From 8f7baffee48800be915792d6ab9aff931a71fdc2 Mon Sep 17 00:00:00 2001
From: Ben Noordhuis <info@bnoordhuis.nl>
Date: Wed, 22 Jun 2016 21:57:13 +0200
Subject: [PATCH] src: fix bad logic in uid/gid checks

Pointed out by Coverity.  Introduced in commits 3546383c ("process_wrap:
avoid leaking memory when throwing due to invalid arguments") and
fa4eb47c ("bindings: add spawn_sync bindings").

The return statements inside the if blocks were dead code because their
guard conditions always evaluated to false.  Remove them.

PR-URL: https://github.com/nodejs/node/pull/7374
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
---
 src/process_wrap.cc | 14 ++++----------
 src/spawn_sync.cc   | 33 +++++++--------------------------
 src/spawn_sync.h    |  1 -
 3 files changed, 11 insertions(+), 37 deletions(-)

diff --git a/src/process_wrap.cc b/src/process_wrap.cc
index 2b214d1d47..d574bf2296 100644
--- a/src/process_wrap.cc
+++ b/src/process_wrap.cc
@@ -123,12 +123,9 @@ class ProcessWrap : public HandleWrap {
     // options.uid
     Local<Value> uid_v = js_options->Get(env->uid_string());
     if (uid_v->IsInt32()) {
-      int32_t uid = uid_v->Int32Value();
-      if (uid & ~((uv_uid_t) ~0)) {
-        return env->ThrowRangeError("options.uid is out of range");
-      }
+      const int32_t uid = uid_v->Int32Value(env->context()).FromJust();
       options.flags |= UV_PROCESS_SETUID;
-      options.uid = (uv_uid_t) uid;
+      options.uid = static_cast<uv_uid_t>(uid);
     } else if (!uid_v->IsUndefined() && !uid_v->IsNull()) {
       return env->ThrowTypeError("options.uid should be a number");
     }
@@ -136,12 +133,9 @@ class ProcessWrap : public HandleWrap {
     // options.gid
     Local<Value> gid_v = js_options->Get(env->gid_string());
     if (gid_v->IsInt32()) {
-      int32_t gid = gid_v->Int32Value();
-      if (gid & ~((uv_gid_t) ~0)) {
-        return env->ThrowRangeError("options.gid is out of range");
-      }
+      const int32_t gid = gid_v->Int32Value(env->context()).FromJust();
       options.flags |= UV_PROCESS_SETGID;
-      options.gid = (uv_gid_t) gid;
+      options.gid = static_cast<uv_gid_t>(gid);
     } else if (!gid_v->IsUndefined() && !gid_v->IsNull()) {
       return env->ThrowTypeError("options.gid should be a number");
     }
diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc
index 2d6012761c..79f10a0ea2 100644
--- a/src/spawn_sync.cc
+++ b/src/spawn_sync.cc
@@ -729,17 +729,19 @@ int SyncProcessRunner::ParseOptions(Local<Value> js_value) {
   }
   Local<Value> js_uid = js_options->Get(env()->uid_string());
   if (IsSet(js_uid)) {
-    if (!CheckRange<uv_uid_t>(js_uid))
+    if (!js_uid->IsInt32())
       return UV_EINVAL;
-    uv_process_options_.uid = static_cast<uv_gid_t>(js_uid->Int32Value());
+    const int32_t uid = js_uid->Int32Value(env()->context()).FromJust();
+    uv_process_options_.uid = static_cast<uv_uid_t>(uid);
     uv_process_options_.flags |= UV_PROCESS_SETUID;
   }
 
   Local<Value> js_gid = js_options->Get(env()->gid_string());
   if (IsSet(js_gid)) {
-    if (!CheckRange<uv_gid_t>(js_gid))
+    if (!js_gid->IsInt32())
       return UV_EINVAL;
-    uv_process_options_.gid = static_cast<uv_gid_t>(js_gid->Int32Value());
+    const int32_t gid = js_gid->Int32Value(env()->context()).FromJust();
+    uv_process_options_.gid = static_cast<uv_gid_t>(gid);
     uv_process_options_.flags |= UV_PROCESS_SETGID;
   }
 
@@ -763,7 +765,7 @@ int SyncProcessRunner::ParseOptions(Local<Value> js_value) {
 
   Local<Value> js_max_buffer = js_options->Get(env()->max_buffer_string());
   if (IsSet(js_max_buffer)) {
-    if (!CheckRange<uint32_t>(js_max_buffer))
+    if (!js_max_buffer->IsUint32())
       return UV_EINVAL;
     max_buffer_ = js_max_buffer->Uint32Value();
   }
@@ -915,27 +917,6 @@ bool SyncProcessRunner::IsSet(Local<Value> value) {
 }
 
 
-template <typename t>
-bool SyncProcessRunner::CheckRange(Local<Value> js_value) {
-  if ((t) -1 > 0) {
-    // Unsigned range check.
-    if (!js_value->IsUint32())
-      return false;
-    if (js_value->Uint32Value() & ~((t) ~0))
-      return false;
-
-  } else {
-    // Signed range check.
-    if (!js_value->IsInt32())
-      return false;
-    if (js_value->Int32Value() & ~((t) ~0))
-      return false;
-  }
-
-  return true;
-}
-
-
 int SyncProcessRunner::CopyJsString(Local<Value> js_value,
                                     const char** target) {
   Isolate* isolate = env()->isolate();
diff --git a/src/spawn_sync.h b/src/spawn_sync.h
index 8ddba479f3..8f4c05aa5f 100644
--- a/src/spawn_sync.h
+++ b/src/spawn_sync.h
@@ -175,7 +175,6 @@ class SyncProcessRunner {
   inline int AddStdioInheritFD(uint32_t child_fd, int inherit_fd);
 
   static bool IsSet(Local<Value> value);
-  template <typename t> static bool CheckRange(Local<Value> js_value);
   int CopyJsString(Local<Value> js_value, const char** target);
   int CopyJsStringArray(Local<Value> js_value, char** target);