Skip to content

Fix memory corruption in headless (nographics) build - #247

Open
laurensWoon wants to merge 1 commit into
jrincayc:masterfrom
laurensWoon:fix-headless-corruption
Open

Fix memory corruption in headless (nographics) build#247
laurensWoon wants to merge 1 commit into
jrincayc:masterfrom
laurensWoon:fix-headless-corruption

Conversation

@laurensWoon

Copy link
Copy Markdown
Contributor

Summary

Any procedure containing 2+ turtle-movement calls (fd/back) crashes a --disable-wx (nographics) build with EXC_BAD_ACCESS.

Root-caused with lldb: nographics.h defines GR_SIZE as 1 ("a dummy graphics header file for computers without graphics"), so graphics.c's char record_buffer[GR_SIZE]; is a single byte -- but safe_to_save() (called from save_line()/save_move(), not gated by any nographics-specific stub) writes a full 8-byte pointer into it on the very first call, silently corrupting whatever global follows record_buffer in memory (observed: AllowGetSet). The corruption doesn't crash immediately -- it surfaces later, on some unrelated evaluator lookup that happens to read the now-garbage pointer, which made this confusing to trace back to its actual cause (looks like the second movement command crashes, not the first).

fd/back are the only movement primitives that go through forward_helper() (and thus save_line()/save_move()); rt/lt don't call it at all, which is why a procedure with only turns never triggered this.

Fix

Make safe_to_save() an unconditional no-op for headless builds, mirroring the existing HAVE_WX-only early-return already in draw_turtle() a few lines above it. There's no window to ever redraw in a headless build, so recording draw operations for later replay serves no purpose there -- bumping GR_SIZE instead would only move the overflow further out, not fix the underlying mismatch.

Also added the equivalent early-return to draw_turtle() itself for headless builds (previously only guarded for HAVE_WX) -- turned out not to be this bug's cause once traced with lldb, but it's the same class of issue (recursing through forward() with nothing to actually draw) and worth closing off regardless.

Test plan

Verified with koch 150 4 (256 nested fd/lt/rt calls from a self-recursive procedure) -- previously crashed immediately, now completes cleanly with a correct final turtle position.

Related to (does not fully explain) #245, which covers a distinct, not-yet-root-caused issue in the wx build.

…overflows the 1-byte record_buffer

Root-caused with lldb: any procedure containing 2+ turtle-movement
commands (any combination including FORWARD/BACK) crashed with
EXC_BAD_ACCESS in varTrue(), called from the core evaluator with a
corrupted AllowGetSet pointer (0x0000000200000001).

nographics.h defines GR_SIZE as 1 ("a dummy graphics header file for
computers without graphics"), so graphics.c's
`char record_buffer[GR_SIZE];` is a single byte. safe_to_save() --
called unconditionally from save_line()/save_move()/etc, not gated by
any backend-specific stub -- writes a full 8-byte pointer into that
1-byte buffer on its very first call (`*(char **)(record) = 0;`),
silently corrupting whatever global follows record_buffer in memory
(AllowGetSet, in this build). The corruption doesn't crash immediately;
it surfaces later, on the next unrelated procedure lookup that happens
to consult that now-garbage pointer -- hence the confusing symptom of a
crash on the *second* movement command, or even later, rather than
the first.

FORWARD/BACK are the only two movement primitives that call
forward_helper() (which calls save_line()/save_move()); RIGHT/LEFT do
not, which is why 'right 90 right 90' alone never triggered this.

Fix: make safe_to_save() an unconditional no-op for headless builds,
mirroring the existing HAVE_WX-only early-out in draw_turtle() a few
lines up. There is no window to ever redraw in a headless build, so
recording draw operations for later replay serves no purpose there --
bumping GR_SIZE instead would only move the overflow further out, not
fix the underlying mismatch.

Verified: 'koch 150 4' (256 nested FD/LT/RT calls, previously crashed
immediately) now completes cleanly with a correct final position.
Comment thread graphics.c
BOOLEAN safe_to_save(void) {
char *newbuf;

#if !defined(HAVE_WX) && !defined(WIN32) && !defined(x_window)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of checking a list of platforms, would it make sense to gate on the memory constraint we care about? In other words

#if GR_SIZE <= GR_SLACK

where GR_SLACK is a sufficient buffer for safe_to_save (in the ballpark of 300)

@benjaminpjones benjaminpjones left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cs (clearscreen) also writes to record_buffer. I believe it would require a similar fix.

record[record_index] = FINISHED;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants