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

mkdtemp doc issue? #26435

Closed
vtjnash opened this issue Mar 4, 2019 · 2 comments · Fixed by #26944
Closed

mkdtemp doc issue? #26435

vtjnash opened this issue Mar 4, 2019 · 2 comments · Fixed by #26944
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.

Comments

@vtjnash
Copy link
Contributor

vtjnash commented Mar 4, 2019

During test-writing / manual fuzzing, I noticed that the doc for mkdtemp added in #6800 states that "Generates six random characters to be appended behind a required prefix to create a unique temporary directory". This is true on Linux and Windows, but misses an edge case on BSDs (including Apple). As seen below, it actually "replaces trailing X's with random characters and then appends six additional random characters to the prefix to create a unique directory name".

// on macOS
> fs.mkdtempSync(path.join(os.tmpdir(), 'foox')) // does as documented
'/tmp/fooxz5Ve7r'

> fs.mkdtempSync(path.join(os.tmpdir(), 'fooX')) // also replaces the X from the prefix
'/tmp/foo1PqEasA'
         ^-- not an 'X'

I think this is just a doc issue, but opening this as an issue to hear what others think.

@sam-github
Copy link
Contributor

@nodejs/libuv Not sure if this is a libuv issue. Its just calling mkdtemp(), should it be shielding users from platform variations in that API?

If not, node docs could say "some systems may replace trailing 'X's. Do not rely on this for portability." or something of the like.

@lpinca lpinca added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Mar 9, 2019
cjihrig added a commit to cjihrig/node that referenced this issue Mar 30, 2019
PR-URL: nodejs#26944
Fixes: nodejs#26435
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@vtjnash
Copy link
Contributor Author

vtjnash commented Mar 30, 2019

Thanks for confirming and clarifying the intended behavior!

BethGriggs pushed a commit that referenced this issue Apr 5, 2019
PR-URL: #26944
Fixes: #26435
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
BethGriggs pushed a commit that referenced this issue Apr 8, 2019
PR-URL: #26944
Fixes: #26435
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Signed-off-by: Beth Griggs <[email protected]>
cjihrig added a commit to cjihrig/node that referenced this issue Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants