From 28ccbf1fbe47ba9d7647f59d35b262cb1f4f60b1 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Tue, 13 Sep 2016 12:57:49 +0200
Subject: [PATCH] fs: fix handling of `uv_stat_t` fields
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`FChown` and `Chown` test that the `uid` and `gid` parameters
they receive are unsigned integers, but `Stat()` and `FStat()`
would return the corresponding fields of `uv_stat_t` as signed
integers. Applications which pass those these values directly
to `Chown` may fail
(e.g. for `nobody` on OS X, who has an `uid` of `-2`, see e.g.
https://github.com/nodejs/node-v0.x-archive/issues/5890).

This patch changes the `Integer::New()` call for `uid` and `gid`
to `Integer::NewFromUnsigned()`.

All other fields are kept as they are, for performance, but
strictly speaking the respective sizes of those
fields aren’t specified, either.

Ref: https://github.com/npm/npm/issues/13918
---
 src/node_file.cc | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/src/node_file.cc b/src/node_file.cc
index 77d11756a2dbe5..d1a12098bbf771 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -433,21 +433,19 @@ Local<Value> BuildStatsObject(Environment* env, const uv_stat_t* s) {
   //     crash();
   //   }
   //
-  // We need to check the return value of Integer::New() and Date::New()
+  // We need to check the return value of Number::New() and Date::New()
   // and make sure that we bail out when V8 returns an empty handle.
 
-  // Integers.
+  // Unsigned integers. It does not actually seem to be specified whether
+  // uid and gid are unsigned or not, but in practice they are unsigned,
+  // and Node’s (F)Chown functions do check their arguments for unsignedness.
 #define X(name)                                                               \
-  Local<Value> name = Integer::New(env->isolate(), s->st_##name);             \
+  Local<Value> name = Integer::NewFromUnsigned(env->isolate(), s->st_##name); \
   if (name.IsEmpty())                                                         \
-    return handle_scope.Escape(Local<Object>());                              \
+    return Local<Object>();                                                   \
 
-  X(dev)
-  X(mode)
-  X(nlink)
   X(uid)
   X(gid)
-  X(rdev)
 # if defined(__POSIX__)
   X(blksize)
 # else
@@ -455,12 +453,24 @@ Local<Value> BuildStatsObject(Environment* env, const uv_stat_t* s) {
 # endif
 #undef X
 
+  // Integers.
+#define X(name)                                                               \
+  Local<Value> name = Integer::NewFromUnsigned(env->isolate(), s->st_##name); \
+  if (name.IsEmpty())                                                         \
+    return Local<Object>();                                                   \
+
+  X(dev)
+  X(mode)
+  X(nlink)
+  X(rdev)
+#undef X
+
   // Numbers.
 #define X(name)                                                               \
   Local<Value> name = Number::New(env->isolate(),                             \
                                   static_cast<double>(s->st_##name));         \
   if (name.IsEmpty())                                                         \
-    return handle_scope.Escape(Local<Object>());                              \
+    return Local<Object>();                                                   \
 
   X(ino)
   X(size)
@@ -479,7 +489,7 @@ Local<Value> BuildStatsObject(Environment* env, const uv_stat_t* s) {
         (static_cast<double>(s->st_##name.tv_nsec / 1000000)));               \
                                                                               \
   if (name##_msec.IsEmpty())                                                  \
-    return handle_scope.Escape(Local<Object>());                              \
+    return Local<Object>();                                                   \
 
   X(atim)
   X(mtim)