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

Fix validate exception handling #473

Conversation

sublimator
Copy link
Collaborator

@sublimator sublimator commented Mar 17, 2025

This PR improves exception handling in the QuickJS bindings by addressing two key issues:

  1. Adds handleException helper that extracts Error#message property and frees variadic JSValue args
  2. Adds comments explaining bytecode object lifecycle management:
    • QuickJS transforms bytecode functions into closures during evaluation
    • Internal structures are shared between original object and closure
    • Manual freeing after evaluation causes double-free crashes
    • Context cleanup safely handles these objects

Additionally:

  • Fixes string length calculation in JS_Eval() (using strlen() instead of sizeof())
  • Adds null/undefined checks for better error handling

}

JS_FreeValue(ctx, val);
// We don't manually free the bytecode object (obj) here because
// JS_EvalFunction internally transforms it into a closure and takes
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Based on all the context we've gathered, here's a comprehensive explanation of the bytecode function memory management issue in QuickJS:

The Core Issue

When you create a bytecode function via JS_ReadObject(..., JS_READ_OBJ_BYTECODE) and then execute it with JS_EvalFunction(), a specific memory management pattern occurs that makes manual cleanup problematic.

What Happens During Evaluation

When JS_EvalFunction() is called on a bytecode function object:

  1. The function is identified as having the tag JS_TAG_FUNCTION_BYTECODE
  2. A new function object (closure) is created via js_closure()
  3. Inside js_closure2(), the original bytecode's internal JSFunctionBytecode structure is directly assigned to the new closure: p->u.func.function_bytecode = b
  4. The new closure is executed and then freed by JS_CallFree()
  5. When the closure is freed, it frees the shared JSFunctionBytecode structure

The Problematic Scenarios

This sharing of the internal structure creates two problematic scenarios:

  1. Attempting to evaluate a bytecode function twice:

    • First evaluation works fine
    • During first evaluation, the internal structure is freed when the closure is freed
    • Second evaluation attempts to use the freed structure, causing a crash
  2. Manually freeing a bytecode function after evaluation:

    • Evaluation creates a closure that takes ownership of the internal structure
    • The closure is freed after execution, which frees the internal structure
    • Manual call to JS_FreeValue() on the original bytecode function attempts to free the already freed structure, causing a crash

The Solution

The safe approach is to let QuickJS's garbage collection handle the cleanup:

  1. Create bytecode function
  2. Evaluate it (which internally transforms and executes it)
  3. Don't manually free it
  4. Let the context cleanup handle it when JS_FreeContext() is called

Best Practice

For safe handling of bytecode functions:

  1. Evaluate each bytecode function exactly once
  2. Never manually free a bytecode function after evaluation
  3. Let context destruction handle the cleanup

This pattern avoids the double-free issues while still allowing efficient execution of bytecode functions.

sublimator added a commit that referenced this pull request Mar 18, 2025
@sublimator
Copy link
Collaborator Author

Manually squash merged

@sublimator sublimator closed this Mar 18, 2025
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.

1 participant