Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes printObject() under DEBUG_STRESS_GC. #392

Closed
wants to merge 2 commits into from

Conversation

dkopko
Copy link

@dkopko dkopko commented Feb 23, 2019

Corrects use-after-free issue with OBJ_FUNCTIONs. This issue is able to be evoked from building with DEBUG_STRESS_GC, launching the REPL, and entering the following function:
f(x,y) { return x + y; }

@munificent
Copy link
Owner

Do you have other changes to your fork of Lox? It shouldn't be possible to have a function that doesn't have a name in the vanilla version of Lox used in the book. (There's a challenge to add support for lambdas, but the core language doesn't support them.)

Is there something else I'm missing here?

@dkopko
Copy link
Author

dkopko commented Feb 25, 2019

My mistake(s): the issue is not solely DEBUG_STRESS_GC, I gave a function example with bad syntax, and the patch is incomplete.

For me, all that is needed for me to evoke a coredump on master branch is to uncomment the line which defines DEBUG_TRACE_GC while leaving DEBUG_STRESS_GC enabled., then run clox and enter a simple function: fun f() { return 5; }

gdb on the coredump shows this line as the issue:
232 printf("<fn %s>", AS_FUNCTION(value)->name->chars);

The issue seems to be the continued printing of the objects via this path:
#0 printObject (value=18445618173829766320) at /home/daniel/workspace/craftinginterpreters/c/object.c:232
#1 0x0000000000405f45 in printValue (value=18445618173829766320) at /home/daniel/workspace/craftinginterpreters/c/value.c:48
#2 0x000000000040404e in grayObject (object=0x19cdcb0) at /home/daniel/workspace/craftinginterpreters/c/memory.c:54
#3 0x0000000000403515 in grayCompilerRoots () at /home/daniel/workspace/craftinginterpreters/c/compiler.c:1582
#4 0x0000000000404593 in collectGarbage () at /home/daniel/workspace/craftinginterpreters/c/memory.c:257
#5 0x0000000000403fac in reallocate (previous=0x0, oldSize=0, newSize=2) at /home/daniel/workspace/craftinginterpreters/c/memory.c:28

The issue is that with all this printing, occasionally objects are printed which have been added to the object list but are not yet completely initialized (or are not de-initialized before unlinking from the list, but while still undergoing printing).

  1. If functions are printed after they have become linked to the object list via newFunction(), but before they have their name field re-assigned (and as such dereferencing name will be a NULL dereference), this will catch that circumstance and print them as if they are an "anonymous" function. Perhaps a better name for this would be an "uninitialized function".
  2. The same issue as 1 also exists for ObjClosures, but I missed that code in this patch backport. (Will update this PR.)
  3. ObjStrings which still may exist in the object list, but which have become partially freed (via FREE_ARRAY in the OBJ_STRING case of freeObject()), will now assign their chars field to NULL, so as to not allow the OBJ_STRING case of printObject() to attempt to print their contents (which must now be considered bogus data as may have become re-assigned).

@dkopko dkopko closed this Feb 25, 2019
@dkopko dkopko reopened this Feb 25, 2019
@dkopko dkopko closed this Feb 1, 2020
@dkopko dkopko deleted the fix_debug_stress_gc_print branch February 1, 2020 05:47
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.

None yet

2 participants