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

Dynamic import attributes broken with multiple attributes #50700

Closed
targos opened this issue Nov 13, 2023 · 4 comments · Fixed by #50703
Closed

Dynamic import attributes broken with multiple attributes #50700

targos opened this issue Nov 13, 2023 · 4 comments · Fixed by #50703
Labels
confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation.

Comments

@targos
Copy link
Member

targos commented Nov 13, 2023

I was trying to adapt a V8 test to run it in Node.js

The following version fails in a C++ CHECK (it doesn't matter if the json file exists or not):

import assert from 'assert';

var life;
import('./modules-skip-1.json', { with: { type: 'json', notARealAssertion: 'value' } }).then(
    namespace => life = namespace.default.life).then(() => {
      assert.strictEqual(life, 42);
    });

var life2;
import('./modules-skip-1.json', { with: { 0: 'value', type: 'json' } }).then(
    namespace => life2 = namespace.default.life).then(() => {
      assert.strictEqual(life2, 42);
    });
#
# Fatal error in , line 0
# Check failed: i < self->length().
#
#
#
#FailureMessage Object: 0x16d934a68
 1: 0x1025fad74 node::NodePlatform::GetStackTracePrinter()::$_3::__invoke() [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]
 2: 0x1036dbcc4 V8_Fatal(char const*, ...) [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]
 3: 0x10273d8bc v8::FixedArray::Get(v8::Local<v8::Context>, int) const [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]
 4: 0x10255ad78 node::loader::createImportAttributesContainer(node::Environment*, v8::Isolate*, v8::Local<v8::FixedArray>) [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]
 5: 0x10255c3d8 node::loader::ImportModuleDynamically(v8::Local<v8::Context>, v8::Local<v8::Data>, v8::Local<v8::Value>, v8::Local<v8::String>, v8::Local<v8::FixedArray>) [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]
 6: 0x102895ab0 v8::internal::Isolate::RunHostImportModuleDynamicallyCallback(v8::internal::MaybeHandle<v8::internal::Script>, v8::internal::Handle<v8::internal::Object>, v8::internal::MaybeHandle<v8::internal::Object>) [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]
 7: 0x102cc3670 v8::internal::Runtime_DynamicImportCall(int, unsigned long*, v8::internal::Isolate*) [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]
 8: 0x103057768 Builtins_CEntry_Return1_ArgvInRegister_NoBuiltinExit [/Users/mzasso/.volta/tools/image/node/21.1.0/bin/node]

This happens in Node.js 21 and v20.x-staging.

@targos targos added confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation. labels Nov 13, 2023
@targos targos self-assigned this Nov 13, 2023
@targos targos changed the title Dynamic import attributes broken with custom attributes Dynamic import attributes broken with multiple attributes Nov 13, 2023
@targos
Copy link
Member Author

targos commented Nov 13, 2023

I thought this would be easy to fix when I saw

for (int i = 0; i < raw_attributes->Length(); i += 3) {

But this is not just an off-by-one issue.

For import(jsModuleDataUrl, { with: { type: 'json', other: 'unsupported' } }) (test I added), the raw_attributes array contains 4 elements.

For import(`data:text/javascript,import${JSON.stringify(jsModuleDataUrl)}with{type:"json"}`) (existing test), it contains 3 elements.

@targos targos removed their assignment Nov 13, 2023
@targos
Copy link
Member Author

targos commented Nov 13, 2023

I don't understand how it's possible. The array is guaranteed to have a size that's a multiple of two:

constexpr size_t kAssertionEntrySizeForDynamicImport = 2;
import_assertions_array = factory()->NewFixedArray(static_cast<int>(
assertion_keys->length() * kAssertionEntrySizeForDynamicImport));

@targos
Copy link
Member Author

targos commented Nov 13, 2023

Debug build makes it a bit more understandable:

$ ./node_g test/es-module/test-esm-import-attributes-errors.mjs
FATAL ERROR: v8::String::Cast Value is not a String
----- Native stack trace -----

 1: 0x100645944 node::DumpNativeBacktrace(__sFILE*) [/Users/mzasso/git/nodejs/node/out/Debug/node]
 2: 0x100784638 node::Abort() [/Users/mzasso/git/nodejs/node/out/Debug/node]
 3: 0x100784798 node::OOMErrorHandler(char const*, v8::OOMDetails const&) [/Users/mzasso/git/nodejs/node/out/Debug/node]
 4: 0x100b344fc v8::String::CheckCast(v8::Data*) [/Users/mzasso/git/nodejs/node/out/Debug/node]
 5: 0x1005fd248 v8::String::Cast(v8::Data*) [/Users/mzasso/git/nodejs/node/out/Debug/node]
 6: 0x1006ed5e8 v8::Local<v8::String> v8::Local<v8::String>::Cast<v8::Data>(v8::Local<v8::Data>) [/Users/mzasso/git/nodejs/node/out/Debug/node]
 7: 0x1006ed590 v8::Local<v8::String> v8::Local<v8::Data>::As<v8::String>() const [/Users/mzasso/git/nodejs/node/out/Debug/node]
 8: 0x1006ea2dc node::loader::createImportAttributesContainer(node::Environment*, v8::Isolate*, v8::Local<v8::FixedArray>) [/Users/mzasso/git/nodejs/node/out/Debug/node]

targos added a commit to targos/node that referenced this issue Nov 13, 2023
The array's length is supposed to be a multiple of two.

Fixes: nodejs#50700
targos added a commit to targos/node that referenced this issue Nov 13, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: nodejs#50700
@targos
Copy link
Member Author

targos commented Nov 13, 2023

Found the issue: #50703

targos added a commit to targos/node that referenced this issue Nov 15, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: nodejs#50700
nodejs-github-bot pushed a commit that referenced this issue Nov 19, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: #50700
PR-URL: #50703
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos added a commit that referenced this issue Nov 23, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: #50700
PR-URL: #50703
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
martenrichter pushed a commit to martenrichter/node that referenced this issue Nov 26, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: nodejs#50700
PR-URL: nodejs#50703
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
lucshi pushed a commit to lucshi/node that referenced this issue Nov 27, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: nodejs#50700
PR-URL: nodejs#50703
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this issue Nov 29, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: #50700
PR-URL: #50703
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this issue Nov 30, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: #50700
PR-URL: #50703
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
nicolo-ribaudo pushed a commit to nicolo-ribaudo/node that referenced this issue Dec 13, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: nodejs#50700
PR-URL: nodejs#50703
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
nicolo-ribaudo pushed a commit to nicolo-ribaudo/node that referenced this issue Dec 13, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: nodejs#50700
PR-URL: nodejs#50703
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
nicolo-ribaudo pushed a commit to nicolo-ribaudo/node that referenced this issue Dec 13, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: nodejs#50700
PR-URL: nodejs#50703
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos added a commit to targos/node that referenced this issue Dec 13, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: nodejs#50700
PR-URL: nodejs#50703
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos added a commit that referenced this issue Dec 14, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: #50700
PR-URL: #50703
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit that referenced this issue Dec 15, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: #50700
PR-URL: #50703
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit that referenced this issue Dec 20, 2023
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: #50700
PR-URL: #50703
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit that referenced this issue Jan 9, 2024
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: #50700
PR-URL: #50703
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
richardlau pushed a commit that referenced this issue Mar 18, 2024
The array's length is supposed to be a multiple of two for dynamic
import callbacks.

Fixes: #50700
PR-URL: #50703
Backport-PR-URL: #51136
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant