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.

Sign up for GitHub

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

src: don't abort when package.json is a directory #18270

Closed
wants to merge 1 commit into from
Closed

src: don't abort when package.json is a directory #18270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
# include <io.h>
#endif

#include <memory>
#include <vector>

namespace node {
Expand Down Expand Up @@ -458,6 +459,12 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
return;
}

std::shared_ptr<void> defer_close(nullptr, [fd, loop] (...) {
uv_fs_t close_req;
CHECK_EQ(0, uv_fs_close(loop, &close_req, fd, nullptr));
uv_fs_req_cleanup(&close_req);
});

const size_t kBlockSize = 32 << 10;
std::vector<char> chars;
int64_t offset = 0;
Expand All @@ -474,14 +481,12 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
numchars = uv_fs_read(loop, &read_req, fd, &buf, 1, offset, nullptr);
uv_fs_req_cleanup(&read_req);

CHECK_GE(numchars, 0);
if (numchars < 0)
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please return the empty string so that it gets put into packageMainCache

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't sound right. '' is for a package.json without a main property, see the bottom of this function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm look at

node/lib/module.js

Line 133 in e1c29f2

return packageMainCache[requestPath] = undefined;
which eagerly returns with a falsey value and sets an entry.

node/lib/module.js

Line 148 in e1c29f2

if (!pkg) return false;
then bails out. this also looks like it might have a bug re-reading it.

Copy link
Member Author

@bnoordhuis bnoordhuis Jan 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but that's for when we found (and read) a package.json. That's not the case here, it's closer to an ENOENT or EPERM condition.

edit: to be clear, that's the if (json === undefined) return false a few lines up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, we found a package.json it was a directory though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, not a file we can parse.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of this change is around package.json/ directories. This mimics a package.json that was found but has no relevant "main" entry : https://github.com/bnoordhuis/io.js/blob/ec93af3bd79c89088f832f72d788225abb106910/src/node_file.cc#L496-L497 . We can discern the "main" we want to represent for it without the need to parse, and returning "" causes the packageMainCache to correctly store the falsey value to bail the module system.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kind of done arguing but I'll address your last point: by your logic errors such as EPERM should also be interpreted as meaning "package.json without a main entry" - but we don't do that and rightly so.

Copy link
Member

@bmeck bmeck Jan 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EPERM is different, you can EPERM a directory. We can inspect this entity on the file system.


offset += numchars;
} while (static_cast<size_t>(numchars) == kBlockSize);

uv_fs_t close_req;
CHECK_EQ(0, uv_fs_close(loop, &close_req, fd, nullptr));
uv_fs_req_cleanup(&close_req);

size_t start = 0;
if (offset >= 3 && 0 == memcmp(&chars[0], "\xEF\xBB\xBF", 3)) {
start = 3; // Skip UTF-8 BOM.
Expand Down
Empty file added test/fixtures/packages/is-dir/package.json/.placeholder
View file
Open in desktop
Empty file.
7 changes: 7 additions & 0 deletions test/parallel/test-module-loading-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,10 @@ common.expectsError(
code: 'ERR_ASSERTION',
message: /^path must be a string$/
});

common.expectsError(
() => { require('../fixtures/packages/is-dir'); },
{
code: 'MODULE_NOT_FOUND',
message: 'Cannot find module \'../fixtures/packages/is-dir\''
});