Use OpenBSD implementation for uv_fs_mkdtemp on Windows #1402

Hinidu · 2014-08-07T07:13:32Z

As stated by @sam-github libuv can't use glibc's modified code because glibc licensed under LGPL, but libuv under MIT. I apologize again, it was my fault because of ignorance.
So in this PR I ported OpenBSD version.
Main differences with original:

tries = TMP_MAX instead of tries = INT_MAX. Though I'm not sure what is the best value.
Used rand instead of arc4random. Though I found portable arc4random.c licensed under 3-clause BSD license. So it is possible to use it if appropriate.

saghul · 2014-08-07T07:28:14Z

src/win/fs.c

-    template_part[4] = letters[v % 62];
-    v /= 62;
-    template_part[5] = letters[v % 62];
+  for (i = 1; i <= num_x; ++i)


Replace this for with the previous "wcsncmp(&req->pathw[len - 6], L"XXXXXX", 6)", and or it in the len < num_x test.

saghul · 2014-08-07T07:33:05Z

I don't think we need arc4random here, there are nough number of tries to make it not collide, I guess. (famous last words)

saghul · 2014-08-07T07:33:42Z

Made some comments, but generally LGTM/ @sam-github, @bnoordhuis, can you PTAL?

Hinidu · 2014-08-07T07:37:38Z

@saghul thanks for review, pushed fixes.

saghul · 2014-08-07T07:39:39Z

LGTM. I'll wait for others to chime in before merging. Thanks for handling this so quickly @Hinidu, much appreciated.

Hinidu · 2014-08-07T07:40:13Z

src/win/fs.c

-    fd = _wmkdir(req->pathw);
+  tries = TMP_MAX;
+  do {
+    v = (((uint64_t)rand()) << 32) | rand();


Should I put a space between (uint64_t) and rand()?

Oh, yeah, good catch.

Hinidu · 2014-08-07T07:43:29Z

Thanks for handling this so quickly @Hinidu, much appreciated.

I'm very glad. It was such a shame for me, so that I actually couldn't do anything else :-)

bnoordhuis · 2014-08-07T09:58:43Z

src/win/fs.c

-    fd = _wmkdir(req->pathw);
+  tries = TMP_MAX;
+  do {
+    v = (((uint64_t) rand()) << 32) | rand();


This has weak entropy. rand() on Windows returns a value between 0 and 32768. That's 15 bits of entropy so that means v will have at most 15 * 2 = 30 bits of entropy.

I assume the original code used arc4random()? I think the Windows equivalent is CryptGenRandom() but that API is somewhat cumbersome to use.

How about using uv_hrtime() ^ _getpid(); as we did before?

The upper bound of entropy in this case is 62^6 (< 2^36). So the best and simplest implementation would be:

v = (((uint64_t) rand()) << 30) | (rand() << 15) | rand();

What do you think?

That looks reasonable to me. Arguably better than time + pid because that's fairly predictable and/or externally observable.

EDIT: Assuming the PRNG has been seeded, of course.

EDIT: Assuming the PRNG has been seeded, of course.

How and when I should do this?

I don't think that should be the responsibility of libuv. Rather, I would add a note to the documentation for uv_fs_mkdtemp() that it's the responsibility of the libuv user to ensure that srand() has been called.

It's either that or:

Switch to a CSPRNG like CryptGenRandom(), or

Make the entropy the responsibility of the user, by means of a user-provided buffer or callback.

Option 1 makes the API easier to use but the user won't have much influence on the quality of the (CS)PRNG.

Option 2 makes the API more cumbersome but is useful in applications like node.js that have access to a cross-platform, high-quality entropy pool (OpenSSL's entropy pool in node's case.)

I don't have a preference, I'm just outlining the possibilities that I can think of.

I don't think that should be the responsibility of libuv. Rather, I would add a note to the documentation for uv_fs_mkdtemp() that it's the responsibility of the libuv user to ensure that srand() has been called.

I'd go with this.

Hinidu · 2014-08-07T14:03:16Z

@bnoordhuis @saghul pushed first solution of the problem with entropy.
In the future if appropriate it can be replaced by any solution like CryptGenRandom or arc4random...

Hinidu · 2014-08-08T05:13:06Z

@sam-github thanks for suggestion. Updated documentation of uv_fs_mkdtemp.

Hinidu · 2014-08-08T13:54:07Z

@bnoordhuis @saghul pushed first solution of the problem with entropy.
In the future if appropriate it can be replaced by any solution like CryptGenRandom or arc4random...

What if we use both uv_hrtime() ^ getpid() and rand()s? General scheme:

uint64_t x = uv_hrtime() ^ getpid();
while (!success) {
  uint64_t y = (((uint64_t) rand()) << 30) | (rand() << 15) | rand();
  success |= try_create_directory(x ^ y);
}

I think it would reduce the need to call srand() before using uv_fs_mkdtemp and accordingly simplify its usage. Though directory name will be possible to predict, I think this is not a problem in the most cases. And when it is a real problem the user can call srand and get what he needs.

What do you think @saghul, @bnoordhuis?

bnoordhuis · 2014-08-08T15:20:20Z

@Hinidu The problem with that approach is if rand() also returns hrtime() ^ getpid(), then you have zero bits of entropy.

Real-world usage won't be that black and white, of course, but if there's some correlation between rand(), hrtime() and getpid(), then instead of strengthening the entropy, you might end up weakening it.

Hinidu · 2014-08-08T16:32:42Z

@bnoordhuis thanks for interesting observation!
I learned a bit about CryptGenRandom. It seems not too hard to use and it seems that this function does not need initialization similiar to srand. So if it has no other problems it looks like ideal approach to me.
I can try to replace rand by CryptGenRandom in this PR in the Monday.

saghul · 2014-08-08T17:08:21Z

Sounds good, thanks Pavel!
On Aug 8, 2014 6:32 PM, "Pavel Platto" notifications@github.com wrote:

@bnoordhuis https://github.com/bnoordhuis thanks for interesting
observation!
I learned a bit about CryptGenRandom. It seems not too hard to use and it
seems that this function does not need initialization similiar to srand. So
if it has no other problems it looks like ideal approach to me.
I can try to replace rand by CryptGenRandom in this PR in the Monday.

—
Reply to this email directly or view it on GitHub
#1402 (comment).

piscisaureus · 2014-08-08T17:20:52Z

src/win/fs.c

      len = strlen(req->path);
-      wcstombs((char*) req->path + len - 6, template_part, 6);
+      wcstombs((char*) req->path + len - num_x, ep - num_x, num_x);
      SET_REQ_RESULT(req, 0);
      return;
    } else if (errno != EEXIST) {
      SET_REQ_RESULT(req, -1);


The request result is supposed to have the actual error code and not just -1

I think this is the prefered way because SET_REQ_RESULT will handle this
https://github.com/Hinidu/libuv/blob/openbsd_mkdtemp/src/win/fs.c#L54

Hinidu · 2014-08-11T07:17:58Z

Pushed commit with CryptGenRandom().

bnoordhuis · 2014-08-11T09:52:52Z

LGTM with the caveat that I'm not really a Windows programmer. If I could make two suggestions:

There are a bunch of CryptReleaseContext() calls strewn around. From a maintenance perspective it would be neater if there was a single clean up (and exit) point.
I would suggest assert()'ing that CryptReleaseContext() returns true (sorry, TRUE) to catch bugs where another thread clobbers the handle.

Hinidu · 2014-08-11T09:56:54Z

There are a bunch of CryptReleaseContext() calls strewn around. From a maintenance perspective it would be neater if there was a single clean up (and exit) point.

It bothers me too. ~~Should I use goto for that? Or some another option?~~ Edit: Possible solution posted below.

I would suggest assert()'ing that CryptReleaseContext() returns true (sorry, TRUE) to catch bugs where another thread clobbers the handle.

How is it possible? The handle is the local variable. I thought that another thread should not be able to clobber it.

Hinidu · 2014-08-11T10:10:31Z

It bothers me too. Should I use goto for that? Or some another option?

I think I found pretty good solution:

diff --git a/src/win/fs.c b/src/win/fs.c
index 511952f..c412db5 100644
--- a/src/win/fs.c
+++ b/src/win/fs.c
@@ -748,10 +748,9 @@ void fs__mkdtemp(uv_fs_t* req) {

   tries = TMP_MAX;
   do {
     if (!CryptGenRandom(hCryptProv, sizeof(v), (BYTE*) &v)) {
-      CryptReleaseContext(hCryptProv, 0);
       SET_REQ_WIN32_ERROR(req, GetLastError());
-      return;
+      break;
     }

     cp = ep - num_x;
@@ -763,18 +762,18 @@ void fs__mkdtemp(uv_fs_t* req) {
     if (_wmkdir(req->pathw) == 0) {
       len = strlen(req->path);
       wcstombs((char*) req->path + len - num_x, ep - num_x, num_x);
-      CryptReleaseContext(hCryptProv, 0);
       SET_REQ_RESULT(req, 0);
-      return;
+      break;
     } else if (errno != EEXIST) {
-      CryptReleaseContext(hCryptProv, 0);
       SET_REQ_RESULT(req, -1);
-      return;
+      break;
     }
   } while (--tries);

   CryptReleaseContext(hCryptProv, 0);
-  SET_REQ_RESULT(req, -1);
+  if (tries == 0) {
+    SET_REQ_RESULT(req, -1);
+  }
 }

saghul · 2014-08-11T11:01:27Z

Looks good, though we need to set some error in case all tries are exhausted.

Hinidu · 2014-08-11T11:05:22Z

Looks good, though we need to set some error in case all tries are exhausted.

In this case errno will be equal to EEXIST because we continue loop only in this case. Do you have other candidate?

saghul · 2014-08-11T11:06:07Z

Ah, right. No, that's OK.

Hinidu · 2014-08-11T11:11:12Z

Force pushed cleanup extraction.
There is left only issue with asserting CryptReleaseContext. Though I think there is no errors which can occur in our simple case according to MSDN. So I'm not sure what to do with that.

saghul · 2014-08-11T11:13:17Z

You can add the assert just to be deffensive :-)

Hinidu · 2014-08-11T11:18:52Z

Ok, force-pushed assert.
~~Just curious what the result would be? Will the program crash in this case or libuv somehow handle asserts?~~

bnoordhuis · 2014-08-11T11:40:01Z

How is it possible? The handle is the local variable. I thought that another thread should not be able to clobber it.

Not normally, no, but a HANDLE is just a pointer to an object. If another thread manages to get a pointer to the same object (accidentally or otherwise), then things can go badly wrong. The assert() is a bit of defensive programming against such bugs.

Hinidu · 2014-08-11T11:44:04Z

Not normally, no, but a HANDLE is just a pointer to an object. If another thread manages to get a pointer to the same object (accidentally or otherwise), then things can go badly wrong. The assert() is a bit of defensive programming against such bugs.

Thanks for explanation! I've already pushed assert as you can see.

saghul · 2014-08-11T11:46:34Z

Thanks @Hinidu, I'll have a final look tonight and land it if everything checks out, which looks that way.

Hinidu · 2014-08-11T11:49:07Z

Thanks @Hinidu, I'll have a final look tonight and land it if everything checks out, which looks that way.

Thank you and all others, your suggestions were very helpful. Will wait landing or other comments.

sam-github · 2014-08-11T15:15:18Z

Looks OK to me, thanks for adding a doc comment.

saghul · 2014-08-12T08:08:14Z

Thanks Pavel! Landed in a669f21.

PS: I added an include for wincrypt.h, otherwise it wouldn't work on MinGW.

Hinidu · 2014-08-12T08:10:41Z

Thanks Saúl!

Nodejs-Jenkins added pr labels Aug 7, 2014

Hinidu mentioned this pull request Aug 7, 2014

Implement uv_fs_mkdtemp #1368

Closed

saghul added this to the v0.12 milestone Aug 7, 2014

saghul mentioned this pull request Aug 7, 2014

fs: replace mkdtemp implementation #1403

Closed

saghul reviewed Aug 7, 2014
View reviewed changes

Hinidu reviewed Aug 7, 2014
View reviewed changes

bnoordhuis reviewed Aug 7, 2014
View reviewed changes

Use OpenBSD implementation for uv_fs_mkdtemp on Windows

94fae68

piscisaureus reviewed Aug 8, 2014
View reviewed changes

Use CryptGenRandom in uv_fs_mkdtemp on Windows

a4fcb07

saghul closed this Aug 12, 2014

Use OpenBSD implementation for uv_fs_mkdtemp on Windows #1402

Use OpenBSD implementation for uv_fs_mkdtemp on Windows #1402

Conversation

Hinidu commented Aug 7, 2014

Choose a reason for hiding this comment

saghul commented Aug 7, 2014

saghul commented Aug 7, 2014

Hinidu commented Aug 7, 2014

saghul commented Aug 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hinidu commented Aug 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hinidu commented Aug 7, 2014

Hinidu commented Aug 8, 2014

Hinidu commented Aug 8, 2014

bnoordhuis commented Aug 8, 2014

Hinidu commented Aug 8, 2014

saghul commented Aug 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hinidu commented Aug 11, 2014

bnoordhuis commented Aug 11, 2014

Hinidu commented Aug 11, 2014

Hinidu commented Aug 11, 2014

saghul commented Aug 11, 2014

Hinidu commented Aug 11, 2014

saghul commented Aug 11, 2014

Hinidu commented Aug 11, 2014

saghul commented Aug 11, 2014

Hinidu commented Aug 11, 2014

bnoordhuis commented Aug 11, 2014

Hinidu commented Aug 11, 2014

saghul commented Aug 11, 2014

Hinidu commented Aug 11, 2014

sam-github commented Aug 11, 2014

saghul commented Aug 12, 2014

Hinidu commented Aug 12, 2014