Return the class name atom from JS_GetClassName() - #1641
Open
andreasrosdal wants to merge 3 commits into
Open
Conversation
JS_GetClassName() duplicates and returns rt->class_array[class_id].class_id, the numeric JSClassID, where a JSAtom is expected. The two fields are adjacent in JSClass and both 32-bit, so the mistake compiles cleanly, but the value handed to JS_DupAtomRT() is a class id reinterpreted as an atom: for a class id below JS_ATOM_END it bumps the refcount of an unrelated predefined atom and returns it, and above that it indexes into the runtime atom array out of any relation to the class. Return .class_name instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014mv33YvfHz7t9mmkituBnn
Contributor
|
Can you please add a test for this? Looks like something we could test for in api-test.c |
andreasrosdal
force-pushed
the
fix-getclassname-atom
branch
from
August 7, 2026 08:17
44512fc to
4bc3068
Compare
Cover the API in api-test.c: the name of a class registered from C, the names of a spread of built-in classes reached through JS_GetClassID(), and JS_ATOM_NULL for class ids with no class behind them. Both name checks fail against the bug they guard against, where the class id was returned in place of the class name atom. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QbX6zyXo3DgDtS3rRPCxBY
andreasrosdal
force-pushed
the
fix-getclassname-atom
branch
from
August 7, 2026 08:18
4bc3068 to
6dacaf5
Compare
Returning the class id produced a valid-looking atom either way: an unrelated predefined atom for a small id, or the id spelled out in decimal for a large one. Rather than trusting a handful of named built-ins to cover both shapes, walk every registered class id and check the name is neither the id in decimal nor anything else starting with a digit. A few internal classes are deliberately unnamed, so an empty name is allowed. Also checks that two classes sharing a name share the interned atom, and that the name does not depend on which context asks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K6eRbuuuCujKgQkrHgvMrc
|
Test added. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JS_GetClassName()duplicates and returnsrt->class_array[class_id].class_id— the numericJSClassID— where aJSAtomis expected.The two fields are adjacent in
JSClassand both 32-bit, so the mistake compiles cleanly, but the value handed toJS_DupAtomRT()is a class id reinterpreted as an atom: for a class id belowJS_ATOM_ENDit bumps the refcount of an unrelated predefined atom and returns that, and above it indexes the runtime atom array with no relation to the class.Return
.class_nameinstead.🤖 Generated with Claude Code
https://claude.ai/code/session_014mv33YvfHz7t9mmkituBnn
Generated by Claude Code