[PATCH] fix heap-use-after-free bug when accessing already freed widget object

[and]

heap-use-after-free hits by accessing mc listing mode

accessing widget object (at g_array_index loop) which was freed
already (item->quick_widget->u.input.label before at loop)

found by Clang/AddressSanitizer?

ERROR: AddressSanitizer: heap-use-after-free on address 0x60800000aaa0 at pc 0x7fcaad33ef39 bp 0x7ffc752eabd0 sp 0x7ffc752eabc8
READ of size 4 at 0x60800000aaa0 thread T0
    #0 0x7fcaad33ef38 in quick_dialog_skip /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/quick.c:615:33
    #1 0x5e3434 in panel_listing_box /tmp/portage/app-misc/mc-9999/work/mc-9999/src/filemanager/boxes.c:831:13
    #2 0x5f5630 in change_listing_cmd /tmp/portage/app-misc/mc-9999/work/mc-9999/src/filemanager/cmd.c:1656:17
    #3 0x52f55d in midnight_execute_cmd /tmp/portage/app-misc/mc-9999/work/mc-9999/src/filemanager/midnight.c:1113:9
    #4 0x7fcaad339319 in menubar_execute /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/menu.c:341:9
    #5 0x7fcaad337962 in menubar_handle_key /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/menu.c:539:13
    #6 0x7fcaad3359c0 in menubar_callback /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/menu.c:597:13
    #7 0x7fcaad31f5a3 in dlg_try_hotkey /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/dialog.c:450:19
    #8 0x7fcaad31e950 in dlg_key_event /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/dialog.c:509:19
    #9 0x7fcaad31ee12 in frontend_dlg_run /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/dialog.c:570:9
    #10 0x7fcaad31eb15 in dlg_run /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/dialog.c:1267:5
    #11 0x52d8dd in do_nc /tmp/portage/app-misc/mc-9999/work/mc-9999/src/filemanager/midnight.c:1757:9
    #12 0x4fb287 in main /tmp/portage/app-misc/mc-9999/work/mc-9999/src/main.c:463:21
    #13 0x7fcaab72d9e3 in __libc_start_main (/lib64/libc.so.6+0x209e3)
    #14 0x427248 in _start (/tmp/portage/app-misc/mc-9999/work/mc-9999/src/.libs/mc+0x427248)

0x60800000aaa0 is located 0 bytes inside of 88-byte region [0x60800000aaa0,0x60800000aaf8)
freed by thread T0 here:
    #0 0x4c9a58 in __interceptor_free (/tmp/portage/app-misc/mc-9999/work/mc-9999/src/.libs/mc+0x4c9a58)
    #1 0x7fcaad33e3e7 in quick_dialog_skip /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/quick.c:616:13
    #2 0x5e3434 in panel_listing_box /tmp/portage/app-misc/mc-9999/work/mc-9999/src/filemanager/boxes.c:831:13
    #3 0x5f5630 in change_listing_cmd /tmp/portage/app-misc/mc-9999/work/mc-9999/src/filemanager/cmd.c:1656:17
    #4 0x52f55d in midnight_execute_cmd /tmp/portage/app-misc/mc-9999/work/mc-9999/src/filemanager/midnight.c:1113:9
    #5 0x7fcaad339319 in menubar_execute /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/menu.c:341:9
    #6 0x7fcaad337962 in menubar_handle_key /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/menu.c:539:13
    #7 0x7fcaad3359c0 in menubar_callback /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/menu.c:597:13
    #8 0x7fcaad31f5a3 in dlg_try_hotkey /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/dialog.c:450:19
    #9 0x7fcaad31e950 in dlg_key_event /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/dialog.c:509:19
    #10 0x7fcaad31ee12 in frontend_dlg_run /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/dialog.c:570:9
    #11 0x7fcaad31eb15 in dlg_run /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/dialog.c:1267:5
    #12 0x52d8dd in do_nc /tmp/portage/app-misc/mc-9999/work/mc-9999/src/filemanager/midnight.c:1757:9
    #13 0x4fb287 in main /tmp/portage/app-misc/mc-9999/work/mc-9999/src/main.c:463:21
    #14 0x7fcaab72d9e3 in __libc_start_main (/lib64/libc.so.6+0x209e3)
    #15 0x427248 in _start (/tmp/portage/app-misc/mc-9999/work/mc-9999/src/.libs/mc+0x427248)

previously allocated by thread T0 here:
    #0 0x4c9ef0 in calloc (/tmp/portage/app-misc/mc-9999/work/mc-9999/src/.libs/mc+0x4c9ef0)
    #1 0x7fcaac65a51c in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x6651c)
    #2 0x7fcaad33f02e in quick_create_labeled_input /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/quick.c:90:26
    #3 0x7fcaad33b076 in quick_dialog_skip /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/quick.c:238:17
    #4 0x5e3434 in panel_listing_box /tmp/portage/app-misc/mc-9999/work/mc-9999/src/filemanager/boxes.c:831:13
    #5 0x5f5630 in change_listing_cmd /tmp/portage/app-misc/mc-9999/work/mc-9999/src/filemanager/cmd.c:1656:17
    #6 0x52f55d in midnight_execute_cmd /tmp/portage/app-misc/mc-9999/work/mc-9999/src/filemanager/midnight.c:1113:9
    #7 0x7fcaad339319 in menubar_execute /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/menu.c:341:9
    #8 0x7fcaad337962 in menubar_handle_key /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/menu.c:539:13
    #9 0x7fcaad3359c0 in menubar_callback /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/menu.c:597:13
    #10 0x7fcaad31f5a3 in dlg_try_hotkey /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/dialog.c:450:19
    #11 0x7fcaad31e950 in dlg_key_event /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/dialog.c:509:19
    #12 0x7fcaad31ee12 in frontend_dlg_run /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/dialog.c:570:9
    #13 0x7fcaad31eb15 in dlg_run /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/dialog.c:1267:5
    #14 0x52d8dd in do_nc /tmp/portage/app-misc/mc-9999/work/mc-9999/src/filemanager/midnight.c:1757:9
    #15 0x4fb287 in main /tmp/portage/app-misc/mc-9999/work/mc-9999/src/main.c:463:21
    #16 0x7fcaab72d9e3 in __libc_start_main (/lib64/libc.so.6+0x209e3)
    #17 0x427248 in _start (/tmp/portage/app-misc/mc-9999/work/mc-9999/src/.libs/mc+0x427248)

SUMMARY: AddressSanitizer: heap-use-after-free /tmp/portage/app-misc/mc-9999/work/mc-9999/lib/widget/quick.c:615:33 in quick_dialog_skip

Signed-off-by: Andreas Mohr <and@…>

[andrew_b]

Patch is dirty.

1. Mix of code and declaration is used.
2. C++-style comment is used.
3. g_list_append is worse than g_list_prepend in this case.
4. g_list_free_full should be used to delete list and data.

[and]

Thanks andrew, comments addressed in new patch version.

[andrew_b]

Replying to and:

Comments addressed in new patch version.

Mix of code and declaration is still there. Please be careful.

[and]

fixed in v3

By the way I found more memleaks, should I create new tickets each
or do your want to create a metaticket for that stuff
where I will attach fix memleak patches for review?

I have no problem with create new ticket each.

[mooffie]

I've read the code and the solution is fine.

I think a less cryptic (and less wrong) comment would be preferable. What about "Prevent heap-use-after-free: the label may follow, not precede, its quick input in the 'widgets' array. So we just mark it." ? Or, shorter, "Prevent heap-use-after-free: we may yet to encounter the label in this 'widgets' array." I prefer the former.

[mooffie]

(An alternative solution is to tweak quick_create_labeled_input() to always add the 'label' before the 'input'. But in the future we may forget about the significance of this order and re-introduce the bug.)

[mooffie]

Another way to write this patch is to call g_list_prepend (removelist, ...) right after the call to quick_create_labeled_input. Then we'll be able to delete the whole loop at the end. Saves code. For clarity we could name this list heap_allocated_quick_widgets instead of removelist.

[zaytsev]

How about attaching a cleaner patch? ;-)

[and]

thanks mooffie, your suggestion is even smarter

v4 patch attached plus fix for a second memleak in this context
(quick widget u.input.result can hold strdup value too, yet never freed neither needed)

[mooffie]

Replying to and:

v4 patch attached plus fix for a second memleak in this context

This "fix" prevents dialogs from extracting data from QUICK_INPUT() inputs. E.g., the "Select" and "Unselect" dialogs (+ and - keys) no longer work.

(quick widget u.input.result can hold strdup value too, yet never freed neither needed)

I don't quite understand.

==

Another thing:

How about changing:

GList *heap_allocated_quick_widgets = NULL; 

to:

GList *extra_quick_widgets = NULL; /* Widgets not directly requested by the user. */

I think it'd be clearer. The name I suggested earlier was awful.

[and]

additional memleak fix

[and]

My bad, I tested only "listing mode" where I hit heap+memleak bug
Reverse wrong second memleak fix from heap bugfix
and attach two separate files with proper memleak fix, I think.

[mooffie]

Same as "v5" but with comments tweaked.

[mooffie]

Replying to zaytsev:

How about attaching a cleaner patch? ;-)

"v6" looks fine to me.

[mooffie]

As for mc-3567-fix-memleak-at-configure_panel_listing.patch :

I think it'd be nicer if configure_panel_listing() didn't g_free() any of its arguments: it'd g_strdup() them. The code calling this function (one such place) would g_free() them.

But that's just my personal opinion. Maybe cmd.c already has some convention for such cases. We see that set_panel_filter_to() does use its argument directly --but it prefixes its name with "allocated_" to inform the user of this.

[andrew_b]

mc-3567-fix-heap-use-after-free-bug-widget-object-v6.patch​: applied with renamed variable.

[andrew_b]

Replying to mooffie:

As for mc-3567-fix-memleak-at-configure_panel_listing.patch :

I think it'd be nicer if configure_panel_listing() didn't g_free() any of its arguments:

I agree.

it'd g_strdup() them.

I disagree. There is no need in extra string duplication.

The code calling this function (one such place) would g_free() them.

Attached patch 0001-configure_panel_listing-fix-memory-leak.patch.

[and]

Replying to andrew_b:

Attached patch 0001-configure_panel_listing-fix-memory-leak.patch.

looks fine, I can't reproduce second memleak

[mooffie]

Replying to andrew_b:

Attached patch 0001-configure_panel_listing-fix-memory-leak.patch.

That's a nice technique.

[andrew_b]

mc-3567-fix-heap-use-after-free-bug-widget-object-v6.patch​: [5f076e9e0510e04f1826946356e444ccff16f418].
Fix memleak in configure_panel_listing: [2b1fa34c9affbd0f428cd1ffeaacf69fdeffd5bd].