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

lib,test: Use long paths when loading native addons #2965

Closed

Conversation

justinmchase
Copy link

Currently, it's possible for a native addon on windows to throw an exception during module loading if it's path is too long.

This change resolves the issue by converting the path to the addon to a long path before calling process.dlopen.

Fixes #2964

@@ -466,7 +466,9 @@ Module._extensions['.json'] = function(module, filename) {


//Native extension for .node
Module._extensions['.node'] = process.dlopen;
Module._extensions['.node'] = function (module, filename) {
Copy link
Member

Choose a reason for hiding this comment

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

style nit: no space after function keyword

Copy link
Author

Choose a reason for hiding this comment

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

Fixed thanks.

@targos targos added module Issues and PRs related to the module subsystem. windows Issues and PRs related to the Windows platform. labels Sep 19, 2015
@justinmchase
Copy link
Author

I wanted to add that I tried making a test for this, but that would entail actually loading a native addon (I think). And I couldn't find an example of another test doing that so I'm not sure how to effectively make a test for this. I'm open for input on how best to achieve that.

@Fishrock123
Copy link
Contributor

but that would entail actually loading a native addon

I don't see a problem with that. It should be test/addons/ though. An example can be found here, I think: https://github.com/nodejs/node/pull/2830/files

cc @bnoordhuis?

@justinmchase
Copy link
Author

@Fishrock123 Great, thanks. Looking now.

@justinmchase
Copy link
Author

@Fishrock123 I apologize for the delay but based on your feedback I added a unit test that successfully loads the addon from a long path.

@justinmchase
Copy link
Author

I tried running eslint like this:

node tools/eslint/bin/eslint.js lib/module.js --rulesdir tools/eslint-rules --quiet

But I get a ton of existing errors. The CONTRIBUTING.md documentation just says:

Make sure the linter is happy and that all tests pass.

I'm not totally sure that I'm doing it right.

@Fishrock123
Copy link
Contributor

For linting just run make jslint :)

On Sep 23, 2015, at 7:05 PM, Justin Chase notifications@github.com wrote:

I tried running eslint like this:

node tools/eslint/bin/eslint.js lib/module.js --rulesdir tools/eslint-rules --quiet
But I get a ton of existing errors. The CONTRIBUTING.md documentation just says:

Make sure the linter is happy and that all tests pass.

I'm not totally sure that I'm doing it right.


Reply to this email directly or view it on GitHub.

@justinmchase justinmchase force-pushed the use-long-path-to-load-addons branch 2 times, most recently from 066501e to 04e3ff7 Compare September 24, 2015 04:07
@justinmchase
Copy link
Author

@Fishrock123 Again, thank you for your patience and your help :)

I ran the tests and the linter successfully. I rebased my commits into a single commit with a descriptive comment and recently rebased upstream/master into my branch. I believe that this is ready for merging.

@justinmchase justinmchase changed the title Use long paths when loading native addons lib,test: Use long paths when loading native addons Sep 25, 2015

// Attempt to load at long path destination
var addon = require(addonDestinationPath);
assert(addon != null);
Copy link
Member

Choose a reason for hiding this comment

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

Can you assert.equal(addon.hello(), 'world'); here?

Copy link
Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor

Choose a reason for hiding this comment

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

@justinmchase Are you intentionally comparing against both null and undefined here?

Copy link
Author

Choose a reason for hiding this comment

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

@thefourtheye I believe I just copied that line from another test doing something similar. However, I think its most accurate to say that addon should be neither null or undefined. So yes it is intentional.

Also, the error case before this PR actually throws an error instead of returning null or undefined. So it's mostly just a sanity check. Same with the following line which actually calls the addon.hello() function, another sanity check.

@bnoordhuis
Copy link
Member

LGTM but someone from @nodejs/platform-windows should sign off on this, I'm not sure to what extent LoadLibrary() supports UNC paths.

CI: https://ci.nodejs.org/job/node-test-pull-request/401/

@thefourtheye
Copy link
Contributor

Also, you might want to make sure that the commit is reflected in your GitHub profile. https://help.github.com/articles/setting-your-email-in-git/

@piscisaureus
Copy link
Contributor

LGTM but someone from @nodejs/platform-windows should sign off on this, I'm not sure to what extent LoadLibrary() supports UNC paths.

It should work, although a comment on the doc page suggests that it doesn't for 32-bit builds. Did you test this?

When using require to load a native addon the path must be converted
into a long path, otherwise the addon will fail to be loaded on
windows if the path is longer than 260 characters.
@justinmchase
Copy link
Author

@piscisaureus I didn't, I only tested on x64. I don't know if I trust that comment but I will make a VM and try it there, it could take a little while.

@thefourtheye I think the machine I happened to make this branch on hadn't been configured right but commit itself should have the right email and name now. Sorry for that.

@benjamingr
Copy link
Member

LGTM, thanks for this it was a painful issue in the past too :D

@bnoordhuis
Copy link
Member

/cc @nodejs/build - I think our Windows CI bots only test 64 bits builds now?

@orangemocha
Copy link
Contributor

@bnoordhuis : correct (even though I would like to fix that).

@justinmchase
Copy link
Author

@bnoordhuis @piscisaureus I built an x86 win7 vm and tested it there, confirmed it works on x86 as well.

@justinmchase
Copy link
Author

I added a correcting comment to msdn to help prevent confusion in the future:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms684175(v=vs.85).aspx#12

@bnoordhuis
Copy link
Member

@Fishrock123
Copy link
Contributor

CI seems happy.

bnoordhuis pushed a commit that referenced this pull request Oct 5, 2015
When using require to load a native addon the path must be converted
into a long path, otherwise the addon will fail to be loaded on
windows if the path is longer than 260 characters.

PR-URL: #2965
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <inglor@gmail.com>
@bnoordhuis
Copy link
Member

Landed in 8593b3e, thanks Justin. I had to touch up the first line of the commit log to make it fit in <= 50 columns, hope you don't mind.

@bnoordhuis bnoordhuis closed this Oct 5, 2015
@jasnell jasnell mentioned this pull request Oct 8, 2015
29 tasks
jasnell pushed a commit that referenced this pull request Oct 8, 2015
When using require to load a native addon the path must be converted
into a long path, otherwise the addon will fail to be loaded on
windows if the path is longer than 260 characters.

PR-URL: #2965
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <inglor@gmail.com>
@justinmchase justinmchase deleted the use-long-path-to-load-addons branch February 5, 2016 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[windows only] Require fails to load native addons with path longer than 260 characters
9 participants