module: node aborts when package.json is a directory

[bnoordhuis]

See discussion in #8277.

$ mkdir -p tmp/package.json && strace -e pread ./out/Release/node -e 'require("./tmp")'
pread(9, 0x1dc2690, 32768, 0)           = -1 EISDIR (Is a directory)
/home/bnoordhuis/src/v1.x/out/Release/node[21915]: ../src/node_file.cc:555:void node::InternalModuleReadFile(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `(numchars) >= (0)' failed.
 1: node::Abort() [./out/Release/node]
 2: node::Assert(char const* const (*) [4]) [./out/Release/node]
 3: 0x10dd222 [./out/Release/node]
 4: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [./out/Release/node]
 5: 0x9df175 [./out/Release/node]
 6: 0x9df967 [./out/Release/node]
 7: 0xadf0a1060c7
--- SIGABRT {si_signo=SIGABRT, si_code=SI_TKILL, si_pid=21915, si_uid=1000} ---
+++ killed by SIGABRT (core dumped) +++
Aborted (core dumped)

Caused by the CHECK_GE(numchars, 0) in InternalModuleReadFile() in src/node_file.cc.

EDIT: It's worth mentioning that the BSDs allow reading from a directory file descriptor so merely squelching the EISDIR error is probably not the best fix.

[cjihrig]

Regarding the BSDs, is it an option to just not support storing your package.json as a directory? Aside from that edge case, I came up with the same solution as @rvagg in #8277.

[bmeck]

I vote we not support loading package.json as a directory

[bnoordhuis]

Regarding the BSDs, is it an option to just not support storing your package.json as a directory?

This was never supported (and never worked) but the BSDs are different from other platforms in that trying to open a directory with the open(2) system call is not an error.

That is, you can't check for an EISDIR error because you won't get one on the BSDs.

Aside from that edge case, I came up with the same solution as @rvagg in #8277.

What in particular are you referring to?

[cjihrig]

More or less the same implementation as #8277, with null checks for the required file name. To be honest, it's been a long time and I don't remember all the details.

[jdalton]

Regardless of being supported it probably shouldn't cause a hard crash.

[bmeck]

PR up, just added a stat instead of trying to juggle things after open.