Invalid `--experimental-modules` CLI flag parsing (core dumped)

[demurgos]

• Version:

  $ node -v
  v10.6.0
  

• Platform:

  $ uname -a
  Linux red 4.17.4-1-ARCH #1 SMP PREEMPT Tue Jul 3 15:45:09 UTC 2018 x86_64 GNU/Linux
  

• Subsystem: cli

While experimenting with building NODE_OPTIONS by concatenating additional options, I found the following two combinations causing a core dump (exit code 134):

$ NODE_OPTIONS="--abort-on-uncaught-exception --experimental-modules" node
munmap_chunk(): invalid pointer
Aborted (core dumped)

$ NODE_OPTIONS="--experimental-modules --experimental-modules" node
free(): invalid size
Aborted (core dumped)

Node should either run successfully or gracefully terminate. The current behavior seems to indicate some unreliable handling of the memory.
The crashes above happen every time I executed the command with these options. It's maybe not just related to experimental-module, the following command runs successfully: NODE_OPTIONS="--trace-sync-io --experimental-modules" node.

[addaleax]

This would be a lazy fix:

diff --git a/src/node.cc b/src/node.cc
index 005b17c27618..b3a25c09368a 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -2818,7 +2818,8 @@ static void ParseArgs(int* argc,
                       bool is_env) {
   const unsigned int nargs = static_cast<unsigned int>(*argc);
   const char** new_exec_argv = new const char*[nargs];
-  const char** new_v8_argv = new const char*[nargs];
+  // *2 because --experimental-modules adds 2 V8 options.
+  const char** new_v8_argv = new const char*[nargs * 2];
   const char** new_argv = new const char*[nargs];
 #if HAVE_OPENSSL
   bool use_bundled_ca = false;

But maybe it’s a better idea to switch to vectors here, since the V8 argv is not exposed as part of the public API (I think) and usually rather small anyway.

[richardlau]

Out of interest (not on a system with a runnable Node.js at the moment to test) does this only happen with NODE_OPTIONS? What if the combination of options is specified directly as arguments to node?

[demurgos]

I was only able to reproduce it with NODE_OPTIONS. If I pass the flags directly to Node, it runs fine. But it does not prove much since there are bad pointers.

Edit: node --experimental-modules --experimental-modules works fine. But node --abort-on-uncaught-exception --experimental-modules crashes.
Edit2: It's not reliable, sometimes it works, sometimes not.

[demurgos]

I took a look at the ParseArgs function: it manipulates a lot of global variables and manually manages the array sizes and string comparisons. It seems easy to do some mistakes there. The node.cc file is already very large, so I think that it'd be more reliable to refactor this function by moving it to its own file, using vector (eventually string) and avoiding side-effects by parsing it into a struct instead of directly updating globals. But I am not familiar with the guidelines for the C++ parts of the code, if it is part of some public API (C FFI?) or if this kind of refactoring is acceptable or not. Regarding the perfs, I don't think the CLI perf is critical, maintainable code may be more import there due to its complexity.

I'd be happy to submit a PR for this, even if it's just about allocating a larger array at the beginning (lazy solution) or only switching to vector. I'd like more feedback about what should be done.

[targos]

I wonder if we could use a good open-source CLI argument parser instead of our own complex and buggy code. For example, CLI11 seems interesting.

[devsnek]

could also consider getopt... i've been wondering for a while if we could move parts of arg parsing to the js side (we could get rid of setting weird properties on process)

[demurgos]

@targos
Looking into available CLI arg parser, the following seem popular and could support Node's syntax (after a first read): CLI11, cxxopts, args.
These not only handle argument parsing but also printing the help or version, not sure this feature is needed. The main drawback is that it adds another dependency to manage. I'm still not sure what's the best between using a lib or just refactoring Node's parser.

[addaleax]

This has been addressed through 29a71ba.