Repository navigation
Copy the $<name> group name before interning it in getSubstitution - #2255
Open
Medios-lgtm wants to merge 1 commit into
Open
Medios-lgtm wants to merge 1 commit into
Medios-lgtm wants to merge 1 commit into
Conversation
getSubstitution passed the result of StringView::getUTF16Ref, a raw pointer into the replacement StringPrimitive, to IdentifierTable::getSymbolHandle. Creating a new identifier allocates before copying the name, so a GC in that window moved the replacement string and the copy read freed memory. Copy the name into the local SmallVector first.
This branch has not been deployed
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
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.
Summary
getSubstitutionresolves a$<name>reference by slicing the replacement string and callingStringView::getUTF16Ref. For a UTF-16 replacement that call does not copy, it returns a raw pointer straight into the replacementStringPrimitiveon the GC heap. That pointer is handed toIdentifierTable::getSymbolHandle, which, when the name is not already interned, allocates a long-livedDynamicStringPrimitiveand only afterwards copies the characters from the caller's pointer.allocateDynamicStringre-reads its input throughprimHandleafter that allocation for exactly this reason, but a bareUTF16Refgives it no handle to re-read, so a GC in the window moves the replacement string and the copy reads freed memory into the new identifier.Any script reaches it through
String.prototype.replace/replaceAllwith a named-groupRegExp, a UTF-16 replacement string, and a group name not yet interned, e.g.'abc'.replace(new RegExp('(?<' + n + '>b)'), '[$<' + n + '>]'). In the default Hades config a long-lived allocation does not start a young-gen collection so the stale read usually survives; underHERMESVM_SANITIZE_HANDLESa collection runs on every allocation and it faults every time.The fix copies the name into the
SmallVectorthat is already declared, withappendUTF16String, before the lookup, the same shapeCallable.cppuses to build a bound function's name.getUTF16Refhas no other caller that feeds the result into an allocating function.Test Plan
Added
test/hermes/regexp-replace-named-group-gc.js, which runs under-gc-sanitize-handles=1.Before the fix, built with
-DHERMESVM_SANITIZE_HANDLES=ON -DHERMESVM_GCKIND=MALLOC -DHERMES_ENABLE_ADDRESS_SANITIZER=ON, the reproducer aborts:After the fix the same sanitize build prints the expected output and ASan is quiet:
In the default ASan+Debug (Hades) build the new test passes and the full
regexplit subset is green (21 passed, 1 unsupported).clang-format --dry-run --Werroris clean on the changed file.