This repository has been archived by the owner on Apr 22, 2023. It is now read-only.
Fix to throw an ENOSYS error properly at fs.futimesSync() when futimes(3) is not supported. #2050
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d and HAVE_FUTIMES is false.
|
Thanks for the report, Shigeki. The bug is in libuv so I didn't take your patch but it's fixed in joyent/libuv@92c9e95. |
| @@ -57,6 +57,12 @@ function runTests(atime, mtime, callback) { | |||
| fs.utimesSync(__filename, atime, mtime); | |||
| expect_ok('utimesSync', __filename, undefined, atime, mtime); | |||
| tests_run++; | |||
|
|
|||
| if (is_windows) { | |||
| fd = fs.openSync(__filename, 'r+'); | |||
There was a problem hiding this comment.
@shigeki: Why do you need 'r+' on Windows?
There was a problem hiding this comment.
Actually, I'm not sure its reason because I just reuse the existing test code of test-fs-utimes.js as
https://github.com/joyent/node/blob/master/test/simple/test-fs-utimes.js#L101
To see the comment above, there might have a specific issue of fd only in Windows but I've not confirmed it yet.
|
Okay, joyent/libuv@92c9e95 is better. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When futimes(3) is not supported, Node was crashed by invoking fs.futimesSync() because the fs_req_wrap destructor calls free(req->path) without initializing. Here below is the reproduction code of crash.
var fs = require('fs');
var fd = fs.openSync(__filename, 'r');
fs.futimesSync(fd, new Date(), new Date());
Fix to throw an ENOSYS error properly when fs.futimesSync() is invoked and HAVE_FUTIMES is false by adding NULL initialization of req->path at the fs_req_wrap constructor.
It can also be fixed by another patch if we move uv_fs_req_init() out of #if HAVE_FUTIMES at deps/uv/src/unix/fs.c:524 but this pull request has only the former.
I also found that the async call of fs.uv_fs_futime() causes the assertion error at the same case. Since it seems to have several options to fix, I will submit an another issue ticket.