Crash / use after free when creating a new PCB within the GTK GUI

[Peter Clifton]

Vaclav Peroutka reported:

"If I want to create new layout after I opened some one before, after I enter layout name, PCB always crashes - I suppose that this is a bug."

Yes - its a bug - technically two bugs, according to the valgrind output...

It doesn't seem to
crash on Linux - but it ought to.. see this Valgrind output:

==22404== Invalid read of size 8
==22404== at 0x4D82B3: ghid_route_style_selector_sync (ghid-route-style-selector.c:594)
==22404== by 0x4BAB28: RouteStylesChanged (gtkhid-main.c:1157)
==22404== by 0x49E0D3: hid_actionv (actions.c:247)
==22404== by 0x447B7B: CreateNewPCB (create.c:194)
==22404== by 0x4273E8: ActionNew (action.c:5902)
==22404== by 0x49E0D3: hid_actionv (actions.c:247)
==22404== by 0x49E483: hid_parse_actionstring (actions.c:331)
==22404== by 0x4CCDE8: ghid_menu_cb (gui-top-window.c:373)
==22404== by 0x6B35253: g_closure_invoke (gclosure.c:774)
==22404== by 0x6B484FA: signal_emit_unlocked_R (gsignal.c:3272)
==22404== by 0x6B51B16: g_signal_emit_valist (gsignal.c:3003)
==22404== by 0x6B51CE1: g_signal_emit (gsignal.c:3060)
==22404== Address 0xd3c4458 is 13,880 bytes inside a block of size 14,120 free'd
==22404== at 0x4C282E0: free (vg_replace_malloc.c:366)
==22404== by 0x4273DE: ActionNew (action.c:5901)
==22404== by 0x49E0D3: hid_actionv (actions.c:247)
==22404== by 0x49E483: hid_parse_actionstring (actions.c:331)
==22404== by 0x4CCDE8: ghid_menu_cb (gui-top-window.c:373)
==22404== by 0x6B35253: g_closure_invoke (gclosure.c:774)
==22404== by 0x6B484FA: signal_emit_unlocked_R (gsignal.c:3272)
==22404== by 0x6B51B16: g_signal_emit_valist (gsignal.c:3003)
==22404== by 0x6B51CE1: g_signal_emit (gsignal.c:3060)
==22404== by 0x5A821D2: _gtk_action_emit_activate (gtkaction.c:794)
==22404== by 0x6B35253: g_closure_invoke (gclosure.c:774)
==22404== by 0x6B47CD6: signal_emit_unlocked_R (gsignal.c:3202)
==22404==
==22404== Invalid read of size 8
==22404== at 0x470944: Parse (parse_l.l:282)
==22404== by 0x471913: ParseFont (parse_l.l:356)
==22404== by 0x447973: CreateDefaultFont (create.c:941)
==22404== by 0x447C58: CreateNewPCB (create.c:211)
==22404== by 0x4273E8: ActionNew (action.c:5902)
==22404== by 0x49E0D3: hid_actionv (actions.c:247)
==22404== by 0x49E483: hid_parse_actionstring (actions.c:331)
==22404== by 0x4CCDE8: ghid_menu_cb (gui-top-window.c:373)
==22404== by 0x6B35253: g_closure_invoke (gclosure.c:774)
==22404== by 0x6B484FA: signal_emit_unlocked_R (gsignal.c:3272)
==22404== by 0x6B51B16: g_signal_emit_valist (gsignal.c:3003)
==22404== by 0x6B51CE1: g_signal_emit (gsignal.c:3060)
==22404== Address 0xd3c4538 is 14,104 bytes inside a block of size 14,120 free'd
==22404== at 0x4C282E0: free (vg_replace_malloc.c:366)
==22404== by 0x4273DE: ActionNew (action.c:5901)
==22404== by 0x49E0D3: hid_actionv (actions.c:247)
==22404== by 0x49E483: hid_parse_actionstring (actions.c:331)
==22404== by 0x4CCDE8: ghid_menu_cb (gui-top-window.c:373)
==22404== by 0x6B35253: g_closure_invoke (gclosure.c:774)
==22404== by 0x6B484FA: signal_emit_unlocked_R (gsignal.c:3272)
==22404== by 0x6B51B16: g_signal_emit_valist (gsignal.c:3003)
==22404== by 0x6B51CE1: g_signal_emit (gsignal.c:3060)
==22404== by 0x5A821D2: _gtk_action_emit_activate (gtkaction.c:794)
==22404== by 0x6B35253: g_closure_invoke (gclosure.c:774)
==22404== by 0x6B47CD6: signal_emit_unlocked_R (gsignal.c:3202)

[gpleda.org commit robot (gpleda-launchpad-robot)]

Bug was fixed by a commit
git master commit bf9aa530be0cd35dcf12e64f5bad6315652bff11
http://git.gpleda.org/?p=pcb.git;a=commit;h=bf9aa530be0cd35dcf12e64f5bad6315652bff11

commit bf9aa530be0cd35dcf12e64f5bad6315652bff11
Author: Peter Clifton <email address hidden>
Commit: Peter Clifton <email address hidden>

    create.c: Don't emit RouteStylesChanged from within CreateNewPCB()

    The route style selector is hanging onto pointers of the
    current PCB's route styles. When the action "RouteStylesChanged"
    is called, these are compared against the current route style
    to identify which one the GUI should show as selected.

    When this call comes from within CreateNewPCB, and the OLD
    PCB has already been free'd, this causes free'd memory to be read,
    resulting in valgrind output such as:

        ==22404== Invalid read of size 8
        ==22404== at 0x4D82B3: ghid_route_style_selector_sync (ghid-route-style-selector.c:594)
        ==22404== by 0x4BAB28: RouteStylesChanged (gtkhid-main.c:1157)
        ==22404== by 0x49E0D3: hid_actionv (actions.c:247)
        ==22404== by 0x447B7B: CreateNewPCB (create.c:194)
        ==22404== by 0x4273E8: ActionNew (action.c:5902)
        ==22404== Address 0xd3c4458 is 13,880 bytes inside a block of size 14,120 free'd
        ==22404== at 0x4C282E0: free (vg_replace_malloc.c:366)
        ==22404== by 0x4273DE: ActionNew (action.c:5901)

    Strictly speaking though - since CreateNewPCB does not directly
    assign to the current PCB variable - it has no business in calling
    the "RouteStylesChanged" action anyway.

    Suitable update is taken care of later on in ActionNew(), as it
    calls the "PCBChanged" action - the GTK implementation of which
    in turn updates the route selector correctly.

    Closes-bug: lp-856909

[gpleda.org commit robot (gpleda-launchpad-robot)]

A commit was made which affects this bug
git master commit cf2586a52565f3306c5c7ca6f54bad0b26f84888
http://git.gpleda.org/?p=pcb.git;a=commit;h=cf2586a52565f3306c5c7ca6f54bad0b26f84888

commit cf2586a52565f3306c5c7ca6f54bad0b26f84888
Author: Peter Clifton <email address hidden>
Commit: Peter Clifton <email address hidden>

    action.c: Set PCB = NULL; after freeing it in ActionNew()

    When we call CreateNewPCB(), it trips over some code which wants
    to access the current PCB in order to determine whether to
    auto-save at exit. The code in question does check if PCB is
    NULL first, so this is a sufficient fix for this case.

    Fixes valgrind output such as:

        ==22404== Invalid read of size 8
        ==22404== at 0x470944: Parse (parse_l.l:282)
        ==22404== by 0x471913: ParseFont (parse_l.l:356)
        ==22404== by 0x447973: CreateDefaultFont (create.c:941)
        ==22404== by 0x447C58: CreateNewPCB (create.c:211)
        ==22404== by 0x4273E8: ActionNew (action.c:5902)
        ==22404== Address 0xd3c4538 is 14,104 bytes inside a block of size 14,120 free'd
        ==22404== at 0x4C282E0: free (vg_replace_malloc.c:366)
        ==22404== by 0x4273DE: ActionNew (action.c:5901)

    Which is seen when starting an new layout from within PCB.

    Affects-bug: lp-856909