Skip to content

Commit ed3d8b1

Browse files
committed
src: fix bad logic in uid/gid checks
Pointed out by Coverity. Introduced in commits 3546383 ("process_wrap: avoid leaking memory when throwing due to invalid arguments") and fa4eb47 ("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: #7374 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
1 parent c50e192 commit ed3d8b1

File tree

3 files changed

+11
-37
lines changed

3 files changed

+11
-37
lines changed

src/process_wrap.cc

+4-10
Original file line numberDiff line numberDiff line change
@@ -123,25 +123,19 @@ class ProcessWrap : public HandleWrap {
123123
// options.uid
124124
Local<Value> uid_v = js_options->Get(env->uid_string());
125125
if (uid_v->IsInt32()) {
126-
int32_t uid = uid_v->Int32Value();
127-
if (uid & ~((uv_uid_t) ~0)) {
128-
return env->ThrowRangeError("options.uid is out of range");
129-
}
126+
const int32_t uid = uid_v->Int32Value(env->context()).FromJust();
130127
options.flags |= UV_PROCESS_SETUID;
131-
options.uid = (uv_uid_t) uid;
128+
options.uid = static_cast<uv_uid_t>(uid);
132129
} else if (!uid_v->IsUndefined() && !uid_v->IsNull()) {
133130
return env->ThrowTypeError("options.uid should be a number");
134131
}
135132

136133
// options.gid
137134
Local<Value> gid_v = js_options->Get(env->gid_string());
138135
if (gid_v->IsInt32()) {
139-
int32_t gid = gid_v->Int32Value();
140-
if (gid & ~((uv_gid_t) ~0)) {
141-
return env->ThrowRangeError("options.gid is out of range");
142-
}
136+
const int32_t gid = gid_v->Int32Value(env->context()).FromJust();
143137
options.flags |= UV_PROCESS_SETGID;
144-
options.gid = (uv_gid_t) gid;
138+
options.gid = static_cast<uv_gid_t>(gid);
145139
} else if (!gid_v->IsUndefined() && !gid_v->IsNull()) {
146140
return env->ThrowTypeError("options.gid should be a number");
147141
}

src/spawn_sync.cc

+7-26
Original file line numberDiff line numberDiff line change
@@ -729,17 +729,19 @@ int SyncProcessRunner::ParseOptions(Local<Value> js_value) {
729729
}
730730
Local<Value> js_uid = js_options->Get(env()->uid_string());
731731
if (IsSet(js_uid)) {
732-
if (!CheckRange<uv_uid_t>(js_uid))
732+
if (!js_uid->IsInt32())
733733
return UV_EINVAL;
734-
uv_process_options_.uid = static_cast<uv_gid_t>(js_uid->Int32Value());
734+
const int32_t uid = js_uid->Int32Value(env()->context()).FromJust();
735+
uv_process_options_.uid = static_cast<uv_uid_t>(uid);
735736
uv_process_options_.flags |= UV_PROCESS_SETUID;
736737
}
737738

738739
Local<Value> js_gid = js_options->Get(env()->gid_string());
739740
if (IsSet(js_gid)) {
740-
if (!CheckRange<uv_gid_t>(js_gid))
741+
if (!js_gid->IsInt32())
741742
return UV_EINVAL;
742-
uv_process_options_.gid = static_cast<uv_gid_t>(js_gid->Int32Value());
743+
const int32_t gid = js_gid->Int32Value(env()->context()).FromJust();
744+
uv_process_options_.gid = static_cast<uv_gid_t>(gid);
743745
uv_process_options_.flags |= UV_PROCESS_SETGID;
744746
}
745747

@@ -763,7 +765,7 @@ int SyncProcessRunner::ParseOptions(Local<Value> js_value) {
763765

764766
Local<Value> js_max_buffer = js_options->Get(env()->max_buffer_string());
765767
if (IsSet(js_max_buffer)) {
766-
if (!CheckRange<uint32_t>(js_max_buffer))
768+
if (!js_max_buffer->IsUint32())
767769
return UV_EINVAL;
768770
max_buffer_ = js_max_buffer->Uint32Value();
769771
}
@@ -915,27 +917,6 @@ bool SyncProcessRunner::IsSet(Local<Value> value) {
915917
}
916918

917919

918-
template <typename t>
919-
bool SyncProcessRunner::CheckRange(Local<Value> js_value) {
920-
if ((t) -1 > 0) {
921-
// Unsigned range check.
922-
if (!js_value->IsUint32())
923-
return false;
924-
if (js_value->Uint32Value() & ~((t) ~0))
925-
return false;
926-
927-
} else {
928-
// Signed range check.
929-
if (!js_value->IsInt32())
930-
return false;
931-
if (js_value->Int32Value() & ~((t) ~0))
932-
return false;
933-
}
934-
935-
return true;
936-
}
937-
938-
939920
int SyncProcessRunner::CopyJsString(Local<Value> js_value,
940921
const char** target) {
941922
Isolate* isolate = env()->isolate();

src/spawn_sync.h

-1
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,6 @@ class SyncProcessRunner {
175175
inline int AddStdioInheritFD(uint32_t child_fd, int inherit_fd);
176176

177177
static bool IsSet(Local<Value> value);
178-
template <typename t> static bool CheckRange(Local<Value> js_value);
179178
int CopyJsString(Local<Value> js_value, const char** target);
180179
int CopyJsStringArray(Local<Value> js_value, char** target);
181180

0 commit comments

Comments
 (0)