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

Address Sanitizer tests failing with ES6 Classes #1388

Closed
2 tasks
Beanyy opened this issue May 2, 2024 · 2 comments
Closed
2 tasks

Address Sanitizer tests failing with ES6 Classes #1388

Beanyy opened this issue May 2, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@Beanyy
Copy link
Contributor

Beanyy commented May 2, 2024

Bug Description

I'm running address sanitization tests on our code base with Hermes and I'm encountering a consistent stack-use-after-scope errors with ES6 classes. I've narrowed down the issue to the code here:

ESTree::NodeList toNodeList() const {
ESTree::NodeList nodeList;
for (auto &node : _storage) {
nodeList.push_back(*node);
}
return nodeList;
}

One of the calling functions makeHermesES6InternalCall calls toNodeList which returned an ESTree::NodeList that was allocated on the stack. Changing the code such that the ESTree::NodeList is created on the stack of makeHermesES6InternalCall instead fixes the ASAN error. I'm not 100% sure why this is happening and it may be due to some compiler specific optimizations.

hermes/lib/AST/ES6Class.cpp

Lines 400 to 412 in 7991309

ESTree::Node *makeHermesES6InternalCall(
ESTree::Node *srcNode,
llvh::StringRef methodName,
const NodeVector &parameters) {
auto hermesInternalIdentifier =
makeIdentifierNode(srcNode, "HermesES6Internal");
auto methodIdentifier = makeIdentifierNode(srcNode, methodName);
auto *getPropertyNode = createTransformedNode<ESTree::MemberExpressionNode>(
srcNode, hermesInternalIdentifier, methodIdentifier, false);
return createTransformedNode<ESTree::CallExpressionNode>(
srcNode, getPropertyNode, nullptr, parameters.toNodeList());
}

It creates a little bit a repetitiveness but removing the toNodeList function (and replacing the calling sites with the appropriate fix) resolved the issue entirely for me.

Let me know what you think.


  • I have run gradle clean and confirmed this bug does not occur with JSC
  • The issue is reproducible with the latest version of React Native.

Hermes git revision (if applicable):
React Native version:
OS:
Platform (most likely one of arm64-v8a, armeabi-v7a, x86, x86_64):

Steps To Reproduce

code example:

The Expected Behavior

@Beanyy Beanyy added the bug Something isn't working label May 2, 2024
@tmikov
Copy link
Contributor

tmikov commented May 2, 2024

Thank you for catching this!

This is a compiler bug in Apple CLang. It was worked around in Static Hermes in b6a791c, but we must have forgotten to backport it to Hermes.

We will backport the fix.

Please note that the ES6 classes AST transform is undocumented and unsupported. But it still shouldn't crash :-)

@Beanyy
Copy link
Contributor Author

Beanyy commented May 2, 2024

Thanks! That fixed the issue.

@Beanyy Beanyy closed this as completed May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants