-
Notifications
You must be signed in to change notification settings - Fork 15k
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: don't export private V8 symbols that can cause native node modules to crash #18281
Conversation
Related to #14617 (comment) |
@miniak that comment is about STL symbols, which this PR doesn't affect. |
Actually this PR is also about STL symbols. std::basic_ostream is in description of this PR from the beginning. I am trying to fix issue @miniak reported. That was originally the main goal of my work. |
Oh huh. How does exporting the |
Exactly! I don't know. I have just tested it fixes our issues and it makes Electron 4 in Teams Desktop App much better. This is the reason I am quite hesitant to publish this change to full PR. First, I would like to know if this will be useful for somebody else. Or, in the meantime, I can find some better solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As written, I think this might impair our ability to run v8 tests in component mode. cc @alexeykuzmin
This could potentially be upstreamable if it were written as something like
#if !defined(HIDE_PRIVATE_SYMBOLS)
#ifdef BUILDING_V8_SHARED
#define V8_EXPORT_PRIVATE __declspec(dllexport)
#elif USING_V8_SHARED
#define V8_EXPORT_PRIVATE __declspec(dllimport)
#else
#define V8_EXPORT_PRIVATE
#endif
#else
#define V8_EXPORT_PRIVATE
#endif
Looks like the build is broken because master is broken. See #18443 |
A maintainer has manually backported this PR to "6-0-x", please check out #18619 |
A maintainer has manually backported this PR to "5-0-x", please check out #18620 |
A maintainer has manually backported this PR to "4-2-x", please check out #18621 |
Description of Change
This change stops private V8 symbols and internal CRT methods being exported. It fixes an issue where native node modules can import incorrect CRT methods and crash on Windows.
V8_EXPORT_PRIVATE
std::basic_ostream
fromnode.lib
which causes crashes if node modules usestd::ostream
v8::internal
from node.libChecklist
npm test
passesRelease Notes
Notes: Removed incorrectly published internal V8 symbols and CRT methods from node.lib, causing heap corruptions with node modules using the dynamic CRT on Windows.