cheat_manager: fix dangling pointer + double-free in working_cheat desc/code copy path

cheat_manager_copy_working_to_idx had a memcpy-aliasing bug
that could cause a double-free when the user edited cheats
after prior add/delete churn.

=== Pointer aliasing via shallow struct copy ===

struct item_cheat holds 'char *desc' and 'char *code' pointer
fields.  cheat_manager_copy_idx_to_working populates
working_cheat via a shallow byte-copy:

    memcpy(&cheat_st->working_cheat,
           &cheat_st->cheats[idx], sizeof(struct item_cheat));
    if (cheat_st->cheats[idx].desc)
       strlcpy(cheat_st->working_desc, cheat_st->cheats[idx].desc,
               CHEAT_DESC_SCRATCH_SIZE);
    ...

After this memcpy, working_cheat.desc == cheats[idx].desc
(pointer alias, not strdup'd).  The string contents are also
copied into the working_desc / working_code scratch buffers,
but the struct-level pointer fields still alias.

The aliased pointers survive across subsequent menu
operations.  If anything frees cheats[src].desc between the
idx_to_working call and a later copy_working_to_idx call,
working_cheat.desc dangles.

=== How the dangling pointer becomes a double-free ===

cheat_manager_copy_working_to_idx was:

    memcpy(&cheats[idx], &working_cheat, sizeof(struct item_cheat));
    if (cheats[idx].desc) free(cheats[idx].desc);
    cheats[idx].desc = strdup(working_desc);
    if (cheats[idx].code) free(cheats[idx].code);
    cheats[idx].code = strdup(working_code);

After the memcpy, cheats[idx].desc == working_cheat.desc.
This has two failure modes:

  1. If working_cheat.desc aliases cheats[src].desc for some
     src != idx, the subsequent free(cheats[idx].desc) frees
     the string that still logically belongs to cheats[src].
     cheats[src].desc now dangles, and a later menu refresh
     that reads cheats[src].desc use-after-frees.

  2. If working_cheat.desc dangles (cheats[src] has since been
     freed), the free(cheats[idx].desc) is a double-free.
     The classic trigger path is:
       a. User opens cheat edit on cheats[N]
          -> copy_idx_to_working(N)  [working_cheat.desc = P
                                      aliases cheats[N].desc]
       b. User backs out without saving and deletes cheats[N]
          via the delete-cheat menu action
          -> cheats[N].desc freed (P freed), working_cheat.desc
             still = P (dangling)
       c. User opens cheat edit on what is now cheats[N] (the
          shifted-down next entry) and hits cancel, triggering
          menu_cbs_cancel.c:99
              cheat_manager_copy_working_to_idx(
                 working_cheat.idx);
          -> memcpy copies the dangling P into cheats[N].desc
          -> free(cheats[N].desc) double-frees P

The exact menu churn varies and many real-world shuffles
defuse the dangling pointer before it reaches the free; but
the shape is reliably reproducible with enough add/delete
sequences, and the underlying invariant ('cheats[i]'s
desc/code are owned solely by cheats[i]') was broken by the
shallow memcpy pattern.

=== Fix: save-restore pattern ===

Save cheats[idx]'s own desc/code pointers before the memcpy
overwrites them with working_cheat's aliases; restore them
after the memcpy; then free and reassign from the scratch
buffers.  Ownership of each cheats[i]'s desc/code stays
local to that slot regardless of what working_cheat's
pointer fields happen to alias or dangle to.

    saved_desc = cheat_st->cheats[idx].desc;
    saved_code = cheat_st->cheats[idx].code;

    memcpy(&cheat_st->cheats[idx], &cheat_st->working_cheat,
           sizeof(struct item_cheat));

    cheat_st->cheats[idx].desc = saved_desc;
    cheat_st->cheats[idx].code = saved_code;

    if (cheat_st->cheats[idx].desc)
       free(cheat_st->cheats[idx].desc);
    cheat_st->cheats[idx].desc = strdup(cheat_st->working_desc);
    ...

The scalar fields (idx, handler, address, value, memory_search_
size, cheat_type, rumble_*, repeat_*, state, big_endian, etc.)
still come from working_cheat via the memcpy - those are the
edited values the user entered in the menu and they are what
we actually want to copy.  Only the string pointers need the
save-restore.

=== Scope: the underlying aliasing remains ===

cheat_manager_copy_idx_to_working and the menu_cbs_ok.c insert/
copy operations (action_ok_cheat_add_new_before,
action_ok_cheat_copy_before, action_ok_cheat_add_new_after,
etc.) still populate working_cheat via shallow memcpy, so
working_cheat.desc and working_cheat.code can still alias or
dangle.  The fix above defuses the one path that turns that
aliasing into a double-free (copy_working_to_idx) rather than
eliminating the underlying invariant violation.

A deeper fix would strdup desc/code on copy_idx_to_working
and free them on the next copy_idx_to_working / teardown,
making working_cheat fully own its strings.  That's a larger
change: every memcpy-to-working_cheat site across menu_cbs_ok.c
would need a matching strdup, and a new tear-down path would
be needed (e.g. on cheat_manager_free).  Out of scope for
this single-file fix which targets the reachable crash.

=== No direct reads of working_cheat.desc / .code ===

A grep for 'working_cheat.desc' and 'working_cheat.code'
across the whole tree finds zero field accesses - only memcpy
sources and destinations.  That means the dangling pointers
in working_cheat themselves are never directly dereferenced;
the crash only surfaces when they get memcpy'd back into a
cheats[] slot and then free()'d.  This is consistent with
the bug being latent for years and only noticed via careful
static audit.

Thread-safety: all cheat_manager operations run on the main
menu thread.  No concurrency concerns.

Reachability: reachable via normal menu use.  The trigger is
any sequence of 'edit cheat A, cancel, delete cheat A, edit
cheat B, cancel' with enough add/delete churn that the new
cheats[N] slot's desc pointer happens to equal the stale
working_cheat.desc.
This commit is contained in:
U-DESKTOP-SPFP6AQ\twistedtechre committed 2026-04-24 07:29:30 +02:00
1 parent 4d2d997c6e
commit 31103bbd5c
1 file changed
+42
+42
View File
@@ -287,12 +287,54 @@ bool cheat_manager_copy_idx_to_working(unsigned idx)
bool cheat_manager_copy_working_to_idx(unsigned idx)
{
cheat_manager_t *cheat_st = &cheat_manager_state;
char *saved_desc;
char *saved_code;
if (!cheat_st->cheats || (cheat_st->size < idx + 1))
return false;
/* Save cheats[idx]'s own desc/code pointers BEFORE the memcpy
* overwrites them with working_cheat's aliases.
*
* working_cheat.desc / .code hold stale or aliased pointers:
* cheat_manager_copy_idx_to_working does a shallow memcpy
* from cheats[src] into working_cheat that copies the pointer
* values without strdup'ing the strings. After that,
* working_cheat.desc == cheats[src].desc (aliased), or if
* cheats[src].desc has since been freed / reassigned (e.g. by
* the user deleting a cheat, adding a new one at the same idx,
* or by a previous call to this function), working_cheat.desc
* dangles.
*
* The pre-patch code did
*
* memcpy(&cheats[idx], &working_cheat, ...);
* if (cheats[idx].desc) free(cheats[idx].desc);
* cheats[idx].desc = strdup(working_desc);
*
* which freed cheats[idx].desc via the just-memcpy'd pointer.
* If that pointer aliased working_cheat.desc, we lost track of
* the original cheats[idx] string (memory leak). Worse, when
* the user cancelled a cheat edit via menu_cbs_cancel.c:99's
* cheat_manager_copy_working_to_idx(working_cheat.idx) path
* after a prior add/delete churn, the dangling working_cheat.
* desc would be copied into cheats[idx] and then free()'d,
* producing a double-free or a free of freed memory.
*
* Fix: save cheats[idx]'s current desc/code pointers, do the
* memcpy for the scalar fields, restore the saved desc/code,
* then free them and reassign from the scratch buffers. This
* keeps ownership local to cheats[idx] across the whole
* function regardless of what working_cheat's desc/code
* happen to point at. */
saved_desc = cheat_st->cheats[idx].desc;
saved_code = cheat_st->cheats[idx].code;
memcpy(&cheat_st->cheats[idx], &cheat_st->working_cheat,
sizeof(struct item_cheat));
cheat_st->cheats[idx].desc = saved_desc;
cheat_st->cheats[idx].code = saved_code;
if (cheat_st->cheats[idx].desc)
free(cheat_st->cheats[idx].desc);