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

Add missing JS_FreeValue calls for early returns #468

Conversation

sublimator
Copy link
Collaborator

@sublimator sublimator commented Mar 14, 2025

📌 Pull Request Overview

🚩 Issue / Context

This PR addresses potential memory leaks and incorrect handling of return values in QuickJS-based transaction serialization (JS_JSONStringify calls), particularly around the correct freeing of JavaScript values and C-string conversions.

What Has Changed?

  • Explicit handling of JS_JSONStringify return values:

    • JS_JSONStringify can return:
      • A valid string (JSValue requiring GC cleanup).
      • An exception (JS_EXCEPTION), represented as a special tagged union value (JS_MKVAL(JS_TAG_EXCEPTION, 0)), which doesn't require cleanup.
      • JavaScript's undefined, represented similarly by a tagged value (JS_MKVAL(JS_TAG_UNDEFINED, 0)), also requiring no cleanup.
    • Added proper checks for both JS_IsException and JS_IsUndefined, handling these special tagged values gracefully and safely.
  • Ensuring correct memory management:

    • Calls to JS_FreeCString and JS_FreeValue are now explicitly placed after every successful conversion to avoid leaks.
    • Added defensive checks to prevent memory leaks upon early returns (such as large string size limits).

🔍 Explanation of JS_MKVAL Values

JS_MKVAL creates lightweight, non-garbage-collected QuickJS "value types":

#define JS_MKVAL(tag, val) (JSValue){ (JSValueUnion){ .int32 = val }, tag }
  • These are value types: they're copied directly on assignment (no pointers involved), meaning they require no explicit cleanup (no JS_FreeValue needed).
  • Examples: JS_EXCEPTION, JS_UNDEFINED, and integer primitives.
  • Confirmed by testing and valgrind analysis, there's no memory leak risk with these special values.

⚠️ Important note about JS_JSONStringify

JS_JSONStringify will explicitly return:

  • JS_EXCEPTION when there's an internal error (e.g., cyclic structures or invalid replacer).
  • JS_UNDEFINED when it can't serialize a given input (e.g., undefined, function, or symbol).

Both are special JS_MKVAL values (value types), hence no GC cleanup required.

🚀 Summary of PR Changes

  • Bugfixes: Memory leak fixes by explicitly cleaning up returned strings and JSValues.
  • Robustness: Proper handling of special QuickJS return values (exception, undefined).
  • Testing: Confirmed correctness via explicit valgrind checks and careful manual reviews.

@sublimator sublimator force-pushed the nd-add-missing-js-freevalue-calls-for-early-returns-2025-03-14 branch from 75c5293 to eea668e Compare March 14, 2025 04:38
@sublimator sublimator closed this Mar 14, 2025
@sublimator sublimator reopened this Mar 14, 2025
}
else
{
// No need to free value types
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe want to handle this error better ....
out += "<failed-to-stringify>" or some such

@@ -5114,15 +5123,20 @@ DEFINE_JS_FUNCTION(JSValue, emit, JSValue raw_tx)
// stringify it
JSValue sdata =
JS_JSONStringify(ctx, raw_tx, JS_UNDEFINED, JS_UNDEFINED);
if (JS_IsException(sdata))
if (JS_IsException(sdata) || JS_IsUndefined(sdata))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe just use !JS_IsString ?

}
else
{
out += "<could not display data>";
Copy link
Collaborator Author

@sublimator sublimator Mar 14, 2025

Choose a reason for hiding this comment

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

matches existing "<could not display hex>"

 if (JS_IsBool(as_hex) && !!JS_ToBool(ctx, as_hex))
    {
        auto in = FromJSIntArrayOrHexString(ctx, data, 64 * 1024);
        if (in.has_value())
        {
            if (in->size() > 1024)
                in->resize(1024);
            out += strHex(*in);
        }
        else
            out += "<could not display hex>";
    }

@sublimator
Copy link
Collaborator Author

merged manually

@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