Segfault on "Find file" with "Search for content" on and "Regular expression" off

[zaytsev]

Hi,

I've installed 4.8.14 and realized that now basically any search with "Find file" dialog for normal search strings (non-regexps) leads to a segfault. Interestingly, when run under gdb, the segfault doesn't happen, so I suspected a non-initialized variable first. However, valgrind didn't show anything obvious and I've bisected between 4.8.13 & 4.8.13 with the following result:

933fd255d07f8bdb9ffe020bda259102890e971a is the first bad commit
commit 933fd255d07f8bdb9ffe020bda259102890e971a
Author: Boris Savelev <boris.savelev@gmail.com>
Date:   Tue Feb 24 16:05:38 2015 +0300

    Ticket #2743: File selection by patterns uses bytes instead of (unicode) characters
    
    Using the "?" pattern in the file selection dialog brought up with '+',
    mc uses the file name length in bytes instead of characters.
    
    Signed-off-by: Slava Zanko <slavazanko@gmail.com>

:040000 040000 5e7732736764d6413acf6a0732e5f3349459d532 9e5c4d502f7d1229fb73645a76ee2bfbd4f777fa M	lib
:040000 040000 bd65e3f59c6140ccbc355355a35762e79a344285 95be9c62d5270b041d96a0f72d1c58069e58799d M	tests

Therefore, I suspect that it's a regression introduced by #2743, which effectively removes G_REGEX_RAW for my case (UTF-8 locale), but I'm not sure of what's the right way to fix it.

Backtraces are as follows:

(gdb) bt
#0  0x00007f6be1fe05c3 in _pcre_xclass () from /lib/x86_64-linux-gnu/libpcre.so.3
#1  0x00007f6be1fce762 in ?? () from /lib/x86_64-linux-gnu/libpcre.so.3
#2  0x00007f6be1fcd5e2 in ?? () from /lib/x86_64-linux-gnu/libpcre.so.3
#3  0x00007f6be1fdd221 in pcre_exec () from /lib/x86_64-linux-gnu/libpcre.so.3
#4  0x00007f6be31d7e68 in g_match_info_next () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#5  0x00007f6be31d94df in g_regex_match_full () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#6  0x000000000046161a in mc_search__regex_found_cond_one (search_str=<optimized out>, search_str=<optimized out>, regex=<optimized out>, lc_mc_search=<optimized out>) at regex.c:259
#7  mc_search__regex_found_cond (search_str=<optimized out>, lc_mc_search=<optimized out>) at regex.c:305
#8  mc_search__run_regex (lc_mc_search=0x27a7980, user_data=0x2797be0, start_search=3, end_search=140101329816960, found_len=0x7f6be1fe4b80 <_pcre_ucd_stage2>) at regex.c:880
#9  0x000000000049063f in search_content (filename=<optimized out>, directory=0x27970c0 "/home/zaytsev/opt/clion-eap/bin/cmake/share/cmake-3.1/Modules", h=0x27946e0) at find.c:1034
#10 do_search (h=0x27946e0) at find.c:1305
#11 0x00000000004908dd in find_callback (w=0x27946e0, sender=<optimized out>, msg=<optimized out>, parm=<optimized out>, data=<optimized out>) at find.c:1448
#12 0x00000000004185fc in send_message (data=0x0, parm=0, msg=MSG_IDLE, sender=0x0, w=0x27946e0) at ../../lib/widget/widget-common.h:167
#13 frontend_dlg_run (h=0x27946e0) at dialog.c:556
#14 dlg_run (h=0x27946e0) at dialog.c:1266
#15 0x0000000000491ac7 in run_process () at find.c:1604
#16 do_find (filename=<optimized out>, dirname=<optimized out>, content=<optimized out>, pattern=<optimized out>, ignore_dirs=<optimized out>, start_dir_len=<optimized out>, start_dir=<optimized out>)
    at find.c:1645
#17 find_file () at find.c:1780
#18 0x0000000000486e85 in find_cmd () at cmd.c:930
#19 0x000000000042f365 in midnight_execute_cmd (sender=0x0, command=105) at midnight.c:1210
#20 0x000000000042f92a in midnight_callback (w=<optimized out>, sender=0x0, msg=<optimized out>, parm=8255, data=<optimized out>) at midnight.c:1560
#21 0x0000000000418014 in send_message (data=0x0, parm=8255, msg=MSG_UNHANDLED_KEY, sender=0x0, w=0x2765fb0) at ../../lib/widget/widget-common.h:167
#22 dlg_key_event (d_key=8255, h=0x2765fb0) at dialog.c:518
#23 dlg_process_event (h=0x2765fb0, key=8255, event=<optimized out>) at dialog.c:1235
#24 0x000000000041863f in frontend_dlg_run (h=0x2765fb0) at dialog.c:569
#25 dlg_run (h=0x2765fb0) at dialog.c:1266
#26 0x00000000004307be in create_panels_and_run_mc () at midnight.c:960
#27 do_nc () at midnight.c:1763
#28 0x000000000040b392 in main (argc=1, argv=0x7ffd365bbca8) at main.c:418

(gdb) bt full
#0  0x00007f6be1fe05c3 in _pcre_xclass () from /lib/x86_64-linux-gnu/libpcre.so.3
No symbol table info available.
#1  0x00007f6be1fce762 in ?? () from /lib/x86_64-linux-gnu/libpcre.so.3
No symbol table info available.
#2  0x00007f6be1fcd5e2 in ?? () from /lib/x86_64-linux-gnu/libpcre.so.3
No symbol table info available.
#3  0x00007f6be1fdd221 in pcre_exec () from /lib/x86_64-linux-gnu/libpcre.so.3
No symbol table info available.
#4  0x00007f6be31d7e68 in g_match_info_next () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
No symbol table info available.
#5  0x00007f6be31d94df in g_regex_match_full () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
No symbol table info available.
#6  0x000000000046161a in mc_search__regex_found_cond_one (search_str=<optimized out>, search_str=<optimized out>, regex=<optimized out>, lc_mc_search=<optimized out>) at regex.c:259
        mcerror = 0x0
#7  mc_search__regex_found_cond (search_str=<optimized out>, lc_mc_search=<optimized out>) at regex.c:305
        loop1 = 41581000
#8  mc_search__run_regex (lc_mc_search=0x27a7980, user_data=0x2797be0, start_search=3, end_search=140101329816960, found_len=0x7f6be1fe4b80 <_pcre_ucd_stage2>) at regex.c:880
        ret = 41581000
        start_pos = 0
        end_pos = 0
#9  0x000000000049063f in search_content (filename=<optimized out>, directory=0x27970c0 "/home/zaytsev/opt/clion-eap/bin/cmake/share/cmake-3.1/Modules", h=0x27946e0) at find.c:1034
        ch = 0 '\000'
        i = 85
        line = <optimized out>
        strbuf = 0x27a7f50 "\220\377\a?Vt\213\231\235\234\234\201\235\204\236\200\237\200\236\200\240\005\242\242\241\242\242\243\200\244\002\243\246\246\200\245\200\247,\250\202hYI;2,+''-7:DX`gjhLFD?;64311469<R_p\213\241\255\240\221{]3\217"
        pos = <optimized out>
        n_read = <optimized out>
        found = <optimized out>
        found_len = 41536624
        result = " \237i\002\000\000\000\000\020_w\002\000\000\000\000\t\000\000\000\000\000\000\000 \237i\002\000\000\000\000`\246[6\375\177\000\000\270\266[6\375\177\000\000ȥ[6\375\177\000\000p\351\033\343k\177\000\000\001", '\000' <repeats 15 times>, "\001\b\000\000\000\000\000\000h\031\006\000\000\000\000\000\001\000\000\000\000\000\000\000\200\201\000\000\350\003\000\000\350\003", '\000' <repeats 14 times>, "\367+\000\000\000\000\000\000\000\020\000\000\000\000\000\000\030\000\000\000\000\000\000\000\017\320,U\000\000\000\000\314,x\r\000\000\000\000\017\320,U\000\000\000\000\314,x\r\000\000\000\000\017\320,U\000\000\000\000\314,x\r", '\000' <repeats 28 times>...
        strbuf_size = <optimized out>
        buffer = "icns\000\000\262\253it32\000\000r\233\000\000\000\000\377\377\377\377\232\377\230\021\000\020\203\017\213\020\004\017\020\022\023\022\222\021\001\020\020\220\021\222\377\003HXUX\200Z\201X\213V\220c\200a\022_VJC=81+'%\"##(/>A%%\200(\021*+,-07>GKXccllcbUL\221\000\220\377\a?Vt\213\231\235\234\234\201\235\204\236\200\237\200\236\200\240\005\242\242\241\242\242\243\200\244\002\243\246\246\200\245\200\247,\250\202hYI;2,+''-7:DX`gjhLFD?;64311469<R_p\213\241\255\240\221{]3\217\000\217\377\006\037U\201\242"...
        vpath = <optimized out>
        seconds = <optimized out>
        useconds = <optimized out>
        status_updated = <optimized out>
        file_fd = 101
        ret_val = 0
        s = {st_dev = 2049, st_ino = 542363, st_nlink = 1, st_mode = 33188, st_uid = 1000, st_gid = 1000, __pad0 = 0, st_rdev = 0, st_size = 45739, st_blksize = 4096, st_blocks = 96, st_atim = {
            tv_sec = 1428999873, tv_nsec = 608672654}, st_mtim = {tv_sec = 1427382262, tv_nsec = 0}, st_ctim = {tv_sec = 1427442754, tv_nsec = 28322291}, __glibc_reserved = {0, 0, 0}}
        tv = {tv_sec = 1429000207, tv_usec = 241806}
#10 do_search (h=0x27946e0) at find.c:1305
        search_ok = <optimized out>
        dp = 0x2779430
        dirp = 0x2771740
        directory = 0x27970c0 "/home/zaytsev/opt/clion-eap/bin/cmake/share/cmake-3.1/Modules"
        tmp_stat = {st_dev = 2049, st_ino = 542363, st_nlink = 1, st_mode = 33188, st_uid = 1000, st_gid = 1000, __pad0 = 0, st_rdev = 0, st_size = 45739, st_blksize = 4096, st_blocks = 96, st_atim = {
            tv_sec = 1428999873, tv_nsec = 608672654}, st_mtim = {tv_sec = 1427382262, tv_nsec = 0}, st_ctim = {tv_sec = 1427442754, tv_nsec = 28322291}, __glibc_reserved = {0, 0, 0}}
        bytes_found = 24
        count = <optimized out>
#11 0x00000000004908dd in find_callback (w=0x27946e0, sender=<optimized out>, msg=<optimized out>, parm=<optimized out>, data=<optimized out>) at find.c:1448
        h = 0x27946e0
#12 0x00000000004185fc in send_message (data=0x0, parm=0, msg=MSG_IDLE, sender=0x0, w=0x27946e0) at ../../lib/widget/widget-common.h:167
        ret = MSG_NOT_HANDLED
#13 frontend_dlg_run (h=0x27946e0) at dialog.c:556
        d_key = <optimized out>
        event = {buttons = 0 '\000', modifiers = 0 '\000', vc = 0, dx = 0, dy = 0, x = -1, y = 67, type = (unknown: 0), clicks = 7385856, margin = (unknown: 0), wdx = 2200, wdy = 110}
#14 dlg_run (h=0x27946e0) at dialog.c:1266
No locals.
#15 0x0000000000491ac7 in run_process () at find.c:1604
No locals.
#16 do_find (filename=<optimized out>, dirname=<optimized out>, content=<optimized out>, pattern=<optimized out>, ignore_dirs=<optimized out>, start_dir_len=<optimized out>, start_dir=<optimized out>)
    at find.c:1645
        dir_tmp = 0x0
        file_tmp = 0x0
#17 find_file () at find.c:1780
        start_dir_len = 53
#18 0x0000000000486e85 in find_cmd () at cmd.c:930
No locals.
#19 0x000000000042f365 in midnight_execute_cmd (sender=0x0, command=105) at midnight.c:1210
        res = MSG_HANDLED
#20 0x000000000042f92a in midnight_callback (w=<optimized out>, sender=0x0, msg=<optimized out>, parm=8255, data=<optimized out>) at midnight.c:1560
        v = MSG_NOT_HANDLED
        command = <optimized out>
#21 0x0000000000418014 in send_message (data=0x0, parm=8255, msg=MSG_UNHANDLED_KEY, sender=0x0, w=0x2765fb0) at ../../lib/widget/widget-common.h:167
        ret = MSG_NOT_HANDLED
#22 dlg_key_event (d_key=8255, h=0x2765fb0) at dialog.c:518
        handled = <optimized out>
#23 dlg_process_event (h=0x2765fb0, key=8255, event=<optimized out>) at dialog.c:1235
No locals.
#24 0x000000000041863f in frontend_dlg_run (h=0x2765fb0) at dialog.c:569
        d_key = <optimized out>
        event = {buttons = 0 '\000', modifiers = 6 '\006', vc = 631, dx = 0, dy = 0, x = -1, y = 110, type = (unknown: 0), clicks = 7197696, margin = (unknown: 0), wdx = 14224, wdy = 110}
#25 dlg_run (h=0x2765fb0) at dialog.c:1266
No locals.
#26 0x00000000004307be in create_panels_and_run_mc () at midnight.c:960
No locals.
#27 do_nc () at midnight.c:1763
        ret = <optimized out>
#28 0x000000000040b392 in main (argc=1, argv=0x7ffd365bbca8) at main.c:418
        mcerror = 0x0
        config_migrated = <optimized out>
        config_migrate_msg = 0x7ffd365bbcb8 "\271\302[6\375\177"
        exit_code = 1

Please let me know if there is any more info I can provide to help fixing this ASAP... it basically makes grepping (core functionality!) unusable for me.

Many thanks!

[egmont]

I can't reproduce it right away. Could you please provide more details? What are your locale settings? What's your glib version? What's the exact contents of all the fields under the Find File dialog? What's the directory tree look like (please try to find the simplest one that reproduces the crash)?

[zaytsev]

The locale is en_US.UTF-8, the system is Ubuntu 14.04 LTS x86_64; mc is built from source after apt-get build-dep mc and ./configure --prefix=$HOME/opt/mc. I will check the other details tomorrow at work and report back. Thanks!

[zaytsev-work]

To reproduce, create an empty directory, put there the attached file, invoke "Find File" with the options as below and enjoy the segfault.

╔══════════════════════════ Find File ═══════════════════════════╗
║ Start at:                                                      ║
║ .                                                 [^] [ Tree ] ║
║ [ ] Enable ignore directories:                                 ║
║                                                            [^] ║
╟────────────────────────────────────────────────────────────────╢
║ File name:                      Content:                       ║
║ *                          [^]  test                       [^] ║
║ [x] Find recursively            [x] Search for content         ║
║ [x] Using shell patterns        [ ] Regular expression         ║
║ [ ] Case sensitive              [ ] Case sensitive             ║
║ [ ] All charsets                [ ] All charsets               ║
║ [ ] Skip hidden                 [x] Whole words                ║
║                                 [ ] First hit                  ║
╟────────────────────────────────────────────────────────────────╢
║                      [< OK >] [ Cancel ]                       ║
╚════════════════════════════════════════════════════════════════╝

$ ~/opt/mc-dev/bin/mc -V
GNU Midnight Commander 4.8.13-103-g933fd25
Built with GLib 2.40.2
Using the S-Lang library with terminfo database
With builtin Editor
With subshell support as default
With support for background operations
With mouse support on xterm and Linux console
With support for X11 events
With internationalization support
With multiple codepages support
Virtual File Systems: cpiofs, tarfs, sfs, extfs, ftpfs, sftpfs, fish
Data types: char: 8; int: 32; long: 64; void *: 64; size_t: 64; off_t: 64;

$ locale
LANG=en_US.UTF-8
LANGUAGE=en_US:en
LC_CTYPE="en_US.UTF-8"
LC_NUMERIC=de_DE.UTF-8
LC_TIME=de_DE.UTF-8
LC_COLLATE="en_US.UTF-8"
LC_MONETARY=de_DE.UTF-8
LC_MESSAGES="en_US.UTF-8"
LC_PAPER=de_DE.UTF-8
LC_NAME=de_DE.UTF-8
LC_ADDRESS=de_DE.UTF-8
LC_TELEPHONE=de_DE.UTF-8
LC_MEASUREMENT=de_DE.UTF-8
LC_IDENTIFICATION=de_DE.UTF-8
LC_ALL=

Slightly better backtrace (-O0):

(gdb) bt
#0  0x00007f80c14715c3 in _pcre_xclass () from /lib/x86_64-linux-gnu/libpcre.so.3
#1  0x00007f80c145f762 in ?? () from /lib/x86_64-linux-gnu/libpcre.so.3
#2  0x00007f80c145e5e2 in ?? () from /lib/x86_64-linux-gnu/libpcre.so.3
#3  0x00007f80c146e221 in pcre_exec () from /lib/x86_64-linux-gnu/libpcre.so.3
#4  0x00007f80c2668e68 in g_match_info_next () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#5  0x00007f80c266a4df in g_regex_match_full () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#6  0x000000000047d45b in mc_search__regex_found_cond_one (lc_mc_search=0xc832a0, regex=0xc835d0, search_str=0xc48e00) at regex.c:258
#7  0x000000000047d55a in mc_search__regex_found_cond (lc_mc_search=0xc832a0, search_str=0xc48e00) at regex.c:304
#8  0x000000000047e79a in mc_search__run_regex (lc_mc_search=0xc832a0, user_data=0xc7e040, start_search=0, end_search=8, found_len=0x7ffc1a6bc228) at regex.c:877
#9  0x000000000047b58e in mc_search__run_normal (lc_mc_search=0xc832a0, user_data=0xc7e040, start_search=0, end_search=8, found_len=0x7ffc1a6bc228) at normal.c:112
#10 0x000000000044d509 in mc_search_run (lc_mc_search=0xc832a0, user_data=0xc7e040, start_search=0, end_search=8, found_len=0x7ffc1a6bc228) at search.c:285
#11 0x00000000004bd503 in search_content (h=0xc566e0, directory=0xc70e60 "/home/zaytsev/opt/clion-eap.bak/bin/cmake/share/cmake-3.1/Modules", filename=0xc57e83 "CPack.VolumeIcon.icns.in") at find.c:1091
#12 0x00000000004bde47 in do_search (h=0xc566e0) at find.c:1361
#13 0x00000000004be37d in find_callback (w=0xc566e0, sender=0x0, msg=MSG_IDLE, parm=0, data=0x0) at find.c:1504
#14 0x0000000000419ee5 in send_message (w=0xc566e0, sender=0x0, msg=MSG_IDLE, parm=0, data=0x0) at ../../lib/widget/widget-common.h:167
#15 0x000000000041ab3b in frontend_dlg_run (h=0xc566e0) at dialog.c:556
#16 0x000000000041c0b3 in dlg_run (h=0xc566e0) at dialog.c:1266
#17 0x00000000004beaa1 in run_process () at find.c:1660
#18 0x00000000004bebec in do_find (start_dir=0xc6f120 "/home/zaytsev/opt/clion-eap.bak/bin/cmake/share", start_dir_len=47, ignore_dirs=0x0, pattern=0xc57a70 "*", content=0xc83370 "test", dirname=0x7ffc1a6bdaa8, 
    filename=0x7ffc1a6bdaa0) at find.c:1701
#19 0x00000000004bf2cf in find_file () at find.c:1836
#20 0x00000000004b0957 in find_cmd () at cmd.c:930
#21 0x000000000043b202 in midnight_execute_cmd (sender=0x0, command=105) at midnight.c:1210
#22 0x000000000043bab1 in midnight_callback (w=0xc44410, sender=0x0, msg=MSG_UNHANDLED_KEY, parm=8255, data=0x0) at midnight.c:1560
#23 0x0000000000419ee5 in send_message (w=0xc44410, sender=0x0, msg=MSG_UNHANDLED_KEY, parm=8255, data=0x0) at ../../lib/widget/widget-common.h:167
#24 0x000000000041aa42 in dlg_key_event (h=0xc44410, d_key=8255) at dialog.c:518
#25 0x000000000041c00c in dlg_process_event (h=0xc44410, key=8255, event=0x7ffc1a6bdc30) at dialog.c:1235
#26 0x000000000041abb0 in frontend_dlg_run (h=0xc44410) at dialog.c:569
#27 0x000000000041c0b3 in dlg_run (h=0xc44410) at dialog.c:1266
#28 0x000000000043ac38 in create_panels_and_run_mc () at midnight.c:960
#29 0x000000000043c25c in do_nc () at midnight.c:1763
#30 0x000000000040ae79 in main (argc=1, argv=0x7ffc1a6bde08) at main.c:418

[egmont]

Thanks; segfault confirmed on Ubuntu Vivid. I'll try to look at it later.

[andrew_b]

Looks like some bad friendship between glib2 and external pcre. My glib is built with internal pcre and I don't have any segfaults here.

I think this is bug in glib2.

[zaytsev]

So you don't think it's possible / feasible to work around on the mc side? I can try to build glib with internal pcre, whatever this means tomorrow and see if the bug disappears.

[egmont]

For me it only crashes if "Whole words" is enabled. Same for mcview's search feature.

[zaytsev]

Hi Egmont, you are right; sorry, I didn't realize that it's significant at first.

[onlyjob]

On Debian Jessie I had segfault too, in editor on search and replace in the text document...

[boris]

Hi all,
I'm sorry for introducing this bug by my patches. The problem comes from these lines, in lib/search/normal.c

	    if (lc_mc_search->whole_words)
	    {
	        /* NOTE: \b as word boundary doesn't allow search
	         * whole words with non-ASCII symbols */
	        g_string_prepend (tmp, "(^|[^\\p{L}\\p{N}_])(");
	        g_string_append (tmp, ")([^\\p{L}\\p{N}_]|$)");
	    }

When the current file, which we are grepping, is binary and invalid UTF8, tests for these \p{L} and \p{N} trigger segfault.
I'm not sure how to fix this properly, probably it's best to revert the original patch.

[egmont]

Isn't there a flag we could pass to glib that means searching for whole words?

Anyway, glib should most definitely not segfault, no matter what string you give to it. It's a bug in glib. We should file them a concise bugreport.

That being said, a workaround in mc would be feasible. I'd probably be happier with a simple workaround (if we can find one) rather than reverting the whole patch.

[boris]

Yes, the bug in glib, but I suspect it's wonfix for them. The documentation says that strings must be valid UTF8 in UTF8 mode.

At present, I don't see a simple workaround. I tried to use "\\b" instead of "^|[^\\p{L}\\p{N}_])(", sometimes it works, but on some files still segfaults. Seems that greping must be done in RAW mode, so we need to pass some flag to mc_search__cond_struct_new_init_regex, but I'm afraid of breaking something else here.

By the way, it seems that hex search is also broken, sometimes it gives unexpected results, but it's not quite clear why.

2 onlyjob: can you confirm that you have the segfault on Debian jessie? Jessie has 4.8.13, which should be unaffected by this bug.

[onlyjob]

MC-4.8.13 is not affected but MC-4.8.14 on Jessie sometimes segfaults on search.

[boris]

Ok, this is the same bug.

[egmont]

Replying to boris:

The documentation says that strings must be valid UTF8 in UTF8 mode.

Seems that greping must be done in RAW mode

You're right. This is really unfortunate.

Perhaps glib's regex code is just not able to provide what we're looking for? (Reverting to RAW mode has obvious downsides due to not handling UTF-8 as expected).

Maybe we should look for another library? Ideally the library should be able to work with UTF-8, yet tolerate invalid UTF-8 in the file. (The pattern is okay if expected to be valid UTF-8.) Is maybe pcre good enough for us? It's a pity that the command line 'grep' doesn't expose its engine via an API.

[zaytsev-work]

It seems to me that in addition to crashing with "[x] Whole words" when "[ ] Regular expression" is off, "[x] Whole words" is now simply ignored when "[x] Regular expression" is on (when looking for "tm" I'm getting matches such as "kGetoptMask" where "tM" is part of the word).

At the same time, "[x] Case sensitive" is working as expected, that is, if I turn it on then I don't get matches like "kGetoptMask" anymore, but I do get matches like "tmpEncoding", so "[x] Whole words" is still not working.

Can anyone confirm that?

[ossi]

it's beyond me how you can think that "must be utf-8" is equivalent with "may crash if it's not". people use regexes to validate untrusted user input. this is an absolutely critical security hole in glib or libpcre. make sure that the involved parties (starting with debian) are aware of it.

[egmont]

Replying to ossi:

it's beyond me how you can think that "must be utf-8" is equivalent with "may crash if it's not". [...]

If you're sure that it's such a critical issue, do you perhaps feel like going ahead and notifying the corresponding people yourself? :)

I disagree with you. Even the simplest library functions (e.g. free(), strdup() etc.) can crash your app if you give it parameters that do not respect the requirements stated in their API documentation. There's nothing new in it. You wouldn't write a tool that asks for an integer from the user, free()s that value, and then complain that free() is buggy because it crashed your app; would you? The current story is not any different.

g_regex_* clearly documents that you have to make sure all strings passed to it are valid UTF-8 (unless the RAW flag is enabled). If someone writes an app that calls it with invalid UTF-8 (e.g. untrusted user input) then it's a bug (and potentially a security hole too) in _that_ application. In this case, midnight commander.

Unless, of course, you can make g_regex_stuff crash with valid UTF-8 input too. That'd be a whole lot different situation.

[egmont]

mc supports two regex engines (decided at compile time): glib and pcre. glib uses pcre under the hood.

What's the point in having this option, and carrying two parallel implementations of some stuff? Shouldn't we just choose one and stick to that? Whichever better suits our requirements, including being able to grep UTF-8 in potentially invalid input.

[ossi]

passing an invalid pointer to a function is quite a different calibre than passing a pointer to data that happens to be slightly malformed. it's about expectations. i'd expect it to just not match (the invalid sequence being silently discarded), or throw some error messages, but not crash. such unexpected behavior with far-reaching consequences should be rather clearly documented if it's intentional. yet the glib docu doesn't say anything about it, and doesn't make any suggestions how to validate the data in advance.

pcre does it right: "If you pass an invalid UTF-8 string when PCRE_NO_UTF8_CHECK is set, the result is undefined and your program may crash." - that means that it both clearly documents the issue, *and* provides a solution (at some cost, obviously).

so the bug is in glib; definitely in the poor api doc, and possibly in the restricted api (a built-in solution could be faster than separate validation, though pcre just does a separate pass internally anyway).

anyway, the way to fix it is calling g_utf8_validate() in a loop and replacing invalid characters with something harmless.

[egmont]

Asking them to make it more explicit in the doc sounds reasonable to me. (Do you feel like following up with them?)

Anyway, this won't help us with this bug here. :(

[andrew_b]

What about recompile glib with internal PCRE as officially recommended?

​https://developer.gnome.org/glib/stable/glib-building.html

GRegex uses the PCRE library for regular expression matching. The default is to use the internal version of PCRE that is patched to use GLib for memory management and Unicode handling. If you prefer to use the system-supplied PCRE library you can pass the --with-pcre=system option to, but it is not recommended.

As I wrote upper, my glib is built with internal pcre and I don't have any segfaults disscussed here.

[ossi]

​https://bugzilla.gnome.org/show_bug.cgi?id=748620

the suggested fix referred to mc (it modifies the input, so it's obviously not suitable for glib).

relying on internal pcre is a no-go, as debian will disable it anyway as soon as they notice it (library bundling is a no-go for distributors).

[zaytsev-work]

egmont, could you please tell me if you can reproduce the problem in comment:16 as well, or I'd need to investigate more to see if it's related or not, and possibly open another ticket for it?

[zaytsev-work]

It seems to me that one solution could be to switch to PCRE directly, since it's anyways an indirect dependency via glib anyways and use PCRE_NO_UTF8_CHECK only for non-UTF8 locales. I'm not familiar with the regex engine, so I'm not sure of what advantages are offered by the glib wrapper.

Another solution would be to validate the input provided to glib for UTF8 locales on the mc side and throw away invalid sequences, but that's kind of annoying and probably less efficient than having the validation directly in PCRE. Also, if invalid sequences are replaced naively with some arbitrary placeholder, this can cause spurious matches.

[ossi]

the problem with using just validation (which is provided by pcre) is that it will reject the entire file (or at least the chunk mcview searches at a time), which makes the function utterly useless for the viewer, which must be reasonably tolerant to invalid input.

so i'd strongly advocate using some harmless placeholder (spaces, dots, question marks, ...) even at the cost of spurious matches.

fwiw, the problem of invalid utf-8 must be addressed by the viewer itself as well, obviously. the search function should behave consistently with the solution employed in the ui part.

[egmont]

Replying to ossi:

relying on internal pcre is a no-go, as debian will disable it anyway as soon as they notice it (library bundling is a no-go for distributors).

Yup, definitely. We can't rely on distros (let alone end users!) compiling glib with certain flag purely for the benefit of mc.

Moreover, just because it crashes on some systems with external pcre and doesn't crash on one with internal pcre, we can't immediately draw the conclusion that it depends on this property. There might be other differences too, e.g. version number.

It's clear now that we use the library in a way that its docs say we mustn't. Compiling that lib with different flags that happens not to trigger a crash is not an acceptable fix. We should make sure that whichever API we chose to use (whether it's glib, or pcre directly, or anything else), we use it according to its documented behavior.

[egmont]

Replying to ossi:

so i'd strongly advocate using some harmless placeholder (spaces, dots, question marks, ...) even at the cost of spurious matches.

If there's no library that tolerates invalid UTF-8 (similarly to GNU grep) then I also vote for this approach. I'd probably choose U+FFFD as the replacement character rather something that is potentially present in the pattern too.

[egmont]

Replying to zaytsev-work:

"[x] Whole words" is now simply ignored when "[x] Regular expression" is on (when looking for "tm" I'm getting matches such as "kGetoptMask" where "tM" is part of the word).

I can't reproduce this one, "Whole words" works for me as expected even with "Regular expression".

Maybe it depends on the exact contents? Could you maybe attach your entire file?

[zaytsev-work]

Try this inside the attached directory (mc-4.8.14, all settings as above):

╔══════════════════════════ Find File ═══════════════════════════╗  
║ Start at:                                                      ║  
║ .                                                 [^] [ Tree ] ║  
║ [ ] Enable ignore directories:                                 ║  
║                                                            [^] ║  
╟────────────────────────────────────────────────────────────────╢  
║ File name:                      Content:                       ║  
║ *                          [^]  test                       [^] ║  
║ [x] Find recursively            [x] Search for content         ║  
║ [x] Using shell patterns        [x] Regular expression         ║  
║ [ ] Case sensitive              [ ] Case sensitive             ║  
║ [ ] All charsets                [ ] All charsets               ║  
║ [ ] Skip hidden                 [x] Whole words                ║  
║                                 [ ] First hit                  ║  
╟────────────────────────────────────────────────────────────────╢  
║                      [< OK >] [ Cancel ]                       ║  
╚════════════════════════════════════════════════════════════════╝

Both file1 and file2 will match, but only file1 should match (at least that's what I would expect). If you unset "[ ] Regular expression", then only file1 matches, as expected.

[egmont]

Yup, confirmed. Does the same for me too, matches both files. (First I was testing mcview's search which doesn't match, although the originally reported segfault is reproducible there too.)

[andrew_b]

Replying to ossi:

relying on internal pcre is a no-go,

No-go is use non-recommended way to build glib.

as debian will disable it anyway as soon as they notice it (library bundling is a no-go for distributors).

I don't care debian (and other distros) as it uses non-recommended way.

Does anybody confirm that glib with internal pcre doesn't segfaults mc?

[egmont]

Replying to andrew_b:

No-go is use non-recommended way to build glib.

No-go is using a library function in a way its documentation says you mustn't use.

Non-recommended != forbidden. You have approximately zero chance convincing distributions to build glib differently, especially if you're argument is "we know we're calling it incorrectly but we don't want to fix that, instead we expect that building glib differently will hide this problem".

And even if you hear from multiple people that _currently_ glib with internal pcre doesn't crash mc _for them_, what's the guarantee that it's true for everyone, and what's the guarantee that it won't change over time?

And even if you managed to convince _all_ distributions to build glib with internal pcre, how long would it take until it reaches every computer, and what about those who compile glib for themselves?

You have to admit that the bug is in mc, and not point fingers at others and expect a workaround from them. It needs to be fixed in mc. We must use glib according to its documentation. Period.

[andrew_b]

Replying to egmont:

Non-recommended != forbidden. You have approximately zero chance convincing distributions to build glib differently, especially if you're argument is "we know we're calling it incorrectly but we don't want to fix that, instead we expect that building glib differently will hide this problem".

My argument is in comment:22.

And even if you managed to convince _all_ distributions to build glib with internal pcre, how long would it take until it reaches every computer, and what about those who compile glib for themselves?

And if mc will be fixed, how long would it take until it reaches every computer, and what about those who compile mc for themselves?

[egmont]

Replying to andrew_b:

My argument is in comment:22.

Which part exactly? Maybe "I don't have any segfaults"? Drawing the conclusion that it might not crash for anyone else is quite far-fetched. Especially, again, when you're clearly aware that you're using its API in a forbidden way.

And if mc will be fixed, how long would it take until it reaches every computer, and what about those who compile mc for themselves?

I really don't have the feeling right now that I'm talking to one of the main developers of mc right now. I more have the feeling talking to someone who's a newbie in this area, sorry. Do I really have to explain it?

mc (and any reasonable app) is able to compile/work against years old versions underlying libs. A 7+ year old glib is good enough. There's a good reason for this, it'd be a hell if you forced anyone to the most recent version of everything. Not everyone who wants the newest mc wants the newest of everything underneath too. Even if you managed to make every distribution switch to glib with builtin pcre _today_, I'd basically have to wait for many months until I could compile/run a "fixed" (strictly in quote marks!) mc. Essentially you'd reduce the 7-year safety gap to zero.

And you still wouldn't be sure that it won't crash, since you'd still use the API incorrectly.

Whereas, if you fixed the code mc, you could make a 4.8.15 release, and then there'd be only a window of maybe 2 months where mc release contained this bug. And I could go ahead and upgrade my mc on all my machines, even on ancient ones. Without touching any component that I don't want to touch, and without risking introducing a bug in another application due to an unforeseen change in an underlying library.

I find this debate totally pointless. Why don't you go ahead and try to convince every distribuion to use built-in pcre? Good luck for that! (I'm pretty sure you'll fail big time, but give it a try.) This current bugreport is obviously not the right forum for that.

How about leaving this bugreport for those whose opinion is not "I don't care debian (and other distros)" and who actually want to fix the bug where it is, that is, in mc? :)

It's 100% clear that using an API in a way its documentation forbids is a severe bug which needs to be fixed. Anything along the lines of "but under certain constellation of the stars it happens to work" is irrelevant.

[zaytsev-work]

egmont, could you just offer a patch or maybe boris could do so, since he broke the grep so badly in the first place? I'm sure that andrew_b will be reasonable and will not block a good-looking patch from being integrated. I'm sorry, but I'm afraid I can't offer a patch myself, but I can certainly try to test one if this would help.

[egmont]

I'd be happy to work on a cleanup and thorough robust fix, one that includes the removal of compile-time choice and contains a single implementation that we choose after careful consideration, and - if necessary - also contains ossi's idea of replacing broken utf8 characters. I'll have to begin by familiarizing myself with the choices, as I don't have experience with g_regex or pcre. Unfortunately I can't promise to start working on it right away. Maybe in a month or so I'll have free time.

(I'm personally not a guy of quick-n-dirty fixes where we just work around this particular segfault, but the code becomes even more complicated, even harder to understand, and we'll likely hit the next similar bug soon. I'd like to first fully understand where we are before moving forward. But I don't mind if someone comes up with such a quick-aid fix. :))

[andrew_b]

Ticket #3501 has been marked as a duplicate of this ticket.

[egmont]

Could you guys please try (and stress-test) the attached patch?

[egmont]

Fix

[zaytsev-work]

Just tried to build the latest master with this new patch, and the new binary doesn't segfault where the old one did. I haven't had time for any more thorough testing though, but I will now switch to this version as the main one and report back if I see any more problems in the future.

[egmont]

I did some more testing with various system and file charsets involved, and I could not crash mc.

Could you guys please go ahead and apply this patch?

[slavazanko]

The branch '3449_search_segfault' was created, initial changeset:0278c11a93b78ef958d669aa3585aa5977d04a48

Egmont, Yuri, Andrew, review please and put your nickname into 'Votes for changeset' field please. It will be enough for apply the patch onto master branch.

[slavazanko]

Merged into master. Merge changeset: e8b883deb6277904add3cffd2c338400b0e618af

Commits:

8524a4f Ticket #3449: Code refactoring: move sanitisation into different function
0278c11 Ticket #3449: Segfault on "Find file" with "Search for content" on and "Regular expression" off

[slavazanko]

Sorry, changes had reverted because an issue was raised while running tests:

make[4]: Entering directory `<https://qa.nest-initiative.org/job/mc-master/ws/build-default/tests/src/editor'>
Running suite(s): /src/editor
**
ERROR:../../../../tests/src/editor/editcmd__edit_complete_word_cmd.c:353:test_autocomplete_single: assertion failed (actual_completed_str->str == data->expected_completed_word): ("\306" == "\306\331\327\301")
33%: Checks: 3, Failures: 1, Errors: 1
../../../../tests/src/editor/editcmd__edit_complete_word_cmd.c:257:F:Core:test_autocomplete:1: editcmd_dialog_completion_show__edit((nil)) pointer should be equal to test_edit(0xa0dac00)

../../../../tests/src/editor/editcmd__edit_complete_word_cmd.c:319:E:Core:test_autocomplete_single:0: (after this point) Received signal 6 (Aborted)
FAIL: editcmd__edit_complete_word_cmd
==================
1 of 1 test failed
==================
make[4]: *** [check-TESTS] Error 1

The branch for the ticket still alive here: '3449_search_segfault', initial changeset:93926565b7030d9edf5ecc5af89e044cb706d1a7

[zaytsev-work]

Can you maybe just force push changeset:738a4d4 instead of reverting the branch merge? There was nothing committed on top of it so far, I would have done it, but I don't have access to the repo from work.

[egmont]

Could you please remind me how to run the tests locally? The jenkins output suggests it's "make check-TESTS" but there's no such target, not even after a "./configure --enable-tests".

[andrew_b]

Replying to egmont:

Could you please remind me how to run the tests locally? The jenkins output suggests it's "make check-TESTS" but there's no such target, not even after a "./configure --enable-tests".

make check

[zaytsev]

Hi, for your information, I've just done

$ git reset --hard HEAD~~
$ git push -f origin master

to roll-back the botched merge.

I suspect the test failure has to do with the fact that the patch replaces invalid characters with \0 (if I understand it correctly), and so auto-completion gets a truncated string. I'm not sure if there is a "clean" solution other than making a new string as the comment suggests, but then, this will probably make it a lot slower? :(

[egmont]

I've just did a brand new checkout, followed by ./autogen --enable-tests and ./configure --enable-tests. Then "make check" just traverses the directory tree, claiming "Nothing to be done for 'check'" in each of them, and doesn't run any tests.

[zaytsev]

Very strange; do you have check and check-devel installed (Fedora)? The CI system does it as follows:

# Prepare for build
./autogen.sh
# Build default configuration

mkdir -p build-default
cd build-default

../configure \
    --prefix="$(pwd)/INSTALL_ROOT" \
    --enable-maintainer-mode \
    --enable-mclib \
    --enable-charset \
    --enable-tests \
    --enable-werror

make
make check
make install

[egmont]

Thanks, I should've paid more attention to configure's output. I didn't have "check" installed.

[egmont]

Fix, v2

[egmont]

Please check this patch.

I've moved the wrapper to be right above the glib method, and to be transparent to the caller (that is, doesn't permanenty mangle the string; instead creates a short-lived temporary copy and modifies that one. It's a small price we pay for nicer and more robust code).

[egmont]

Fix, v3

[egmont]

v2 didn't handle strings containing NULs correctly. I fixed it now. I also clarified the method's comment.

Please review.

[zaytsev]

Hi Egmont, I will try to test the new patch tomorrow at work by compiling a patched version and seeing it it searches without crashing.

Regarding the implementation, I wonder if embedded NULs are handled correctly elsewhere though... anyways it would be nice if you could leave a comment next to memcpy that g_strndup will scramble the string, I didn't think of it at first.

Thanks for your efforts!

[egmont]

I've tested both in current mc and in patch v1 that matches that occur after a NUL within a line are found correctly. This means that they are handled correctly elsewhere in the code.

Then I refactored the code and forgot to check it again in v2. Of course, according to Murphy's law, it didn't work :D It was totally my fault, I got confused about what strndup() does. I see no point in adding a comment next to the current solution explaining why another solution would be broken, repeating the doc of that other method that we don't use. Just forget v2 of this patch ever existed! ;)

[zaytsev]

I see no point in adding a comment

Ok, I'll do it then, if I get to it. My knee-jerk reflex to v3 was like "wat, isn't there something like g_strndup?", but then I checked it out, and found that it uses strncpy, and so swallows embedded NULs. At this point, it became clear why you did what you did...

[zaytsev]

Branch: 3449_search_segfault.
Changeset: [7cdb3e47c0acf27f5e20f7a279ed9a87a9300bbb].

Travis CI build is successful, related tests now seem to pass: ​https://travis-ci.org/MidnightCommander/mc/builds/86055461 .

[zaytsev]

Merged to master: [8382dfb09d0d9fe0c814632a1525a7dabe411bca].

Thanks Egmont for your help and patience!

[zaytsev]

Merged to master: [65fcc798ab1040666d4986d2c5705761aed01cdb].

Sorry, Egmont, you are right, the right glib2 version was in your email, but I overlooked it (yet, I wasn't drunk).

[zaytsev]

Merged to master: [52fd328042a426e885da891c8ce8218cda3a1cf7].