src: fix segfaults, fix 32 bits integer negation

bnoordhuis · indutny · commit f6ea0c203aa0 · 2014-03-16T16:15:33.000+04:00
Make calls to v8::Isolate::AdjustAmountOfExternalAllocatedMemory() take special care when negating 32 bits unsigned types like size_t. Before this commit, values were negated before they got promoted to 64 bits, meaning that on 32 bits architectures, a value like 42 got cast to 4294967254 instead of -42. That in turn made the garbage collector start scavenging like crazy because it thought the system was out of memory. That's bad enough but calls to AdjustAmountOfExternalAllocatedMemory() were made from weak callbacks, i.e. at a time when the garbage collector was already busy. It triggered asserts in debug builds and caused random crashes and memory corruption in release builds. The behavior in release builds is arguably a V8 bug and should perhaps be reported upstream. Partially fixes nodejs#7309 but requires further bug fixes to src/smalloc.cc that I'll address in a follow-up commit.
diff --git a/src/node_zlib.cc b/src/node_zlib.cc
@@ -110,13 +110,13 @@ class ZCtx : public AsyncWrap {
 
     if (mode_ == DEFLATE || mode_ == GZIP || mode_ == DEFLATERAW) {
       (void)deflateEnd(&strm_);
-      env()->isolate()
-          ->AdjustAmountOfExternalAllocatedMemory(-kDeflateContextSize);
+      int64_t change_in_bytes = -static_cast<int64_t>(kDeflateContextSize);
+      env()->isolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);
     } else if (mode_ == INFLATE || mode_ == GUNZIP || mode_ == INFLATERAW ||
                mode_ == UNZIP) {
       (void)inflateEnd(&strm_);
-      env()->isolate()
-          ->AdjustAmountOfExternalAllocatedMemory(-kInflateContextSize);
+      int64_t change_in_bytes = -static_cast<int64_t>(kInflateContextSize);
+      env()->isolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);
     }
     mode_ = NONE;
 
diff --git a/src/smalloc.cc b/src/smalloc.cc
@@ -291,7 +291,8 @@ void TargetCallback(const WeakCallbackData<Object, char>& data) {
   assert(array_size * len >= len);
   len *= array_size;
   if (info != NULL && len > 0) {
-    data.GetIsolate()->AdjustAmountOfExternalAllocatedMemory(-len);
+    int64_t change_in_bytes = -static_cast<int64_t>(len);
+    data.GetIsolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);
     free(info);
   }
 
@@ -338,7 +339,8 @@ void AllocDispose(Environment* env, Handle<Object> obj) {
     free(data);
   }
   if (length != 0) {
-    env->isolate()->AdjustAmountOfExternalAllocatedMemory(-length);
+    int64_t change_in_bytes = -static_cast<int64_t>(length);
+    env->isolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);
   }
 }
 
@@ -407,7 +409,8 @@ void TargetFreeCallback(Isolate* isolate,
     assert(len * array_size > len);
     len *= array_size;
   }
-  isolate->AdjustAmountOfExternalAllocatedMemory(-(len + sizeof(*cb_info)));
+  int64_t change_in_bytes = -static_cast<int64_t>(len + sizeof(*cb_info));
+  isolate->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);
   cb_info->p_obj.Reset();
   cb_info->cb(data, cb_info->hint);
   delete cb_info;
diff --git a/src/string_bytes.cc b/src/string_bytes.cc
@@ -50,7 +50,8 @@ class ExternString: public ResourceType {
   public:
     ~ExternString() {
       delete[] data_;
-      isolate()->AdjustAmountOfExternalAllocatedMemory(-length_);
+      int64_t change_in_bytes = -static_cast<int64_t>(length_);
+      isolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);
     }
 
     const TypeName* data() const {

Original file line number	Diff line number	Diff line change
`@@ -291,7 +291,8 @@ void TargetCallback(const WeakCallbackData<Object, char>& data) {`
`291`	`291`	`assert(array_size * len >= len);`
`292`	`292`	`len *= array_size;`
`293`	`293`	`if (info != NULL && len > 0) {`
`294`		`- data.GetIsolate()->AdjustAmountOfExternalAllocatedMemory(-len);`
	`294`	`+ int64_t change_in_bytes = -static_cast<int64_t>(len);`
	`295`	`+ data.GetIsolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);`
`295`	`296`	`free(info);`
`296`	`297`	`}`
`297`	`298`
`@@ -338,7 +339,8 @@ void AllocDispose(Environment* env, Handle<Object> obj) {`
`338`	`339`	`free(data);`
`339`	`340`	`}`
`340`	`341`	`if (length != 0) {`
`341`		`- env->isolate()->AdjustAmountOfExternalAllocatedMemory(-length);`
	`342`	`+ int64_t change_in_bytes = -static_cast<int64_t>(length);`
	`343`	`+ env->isolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);`
`342`	`344`	`}`
`343`	`345`	`}`
`344`	`346`
`@@ -407,7 +409,8 @@ void TargetFreeCallback(Isolate* isolate,`
`407`	`409`	`assert(len * array_size > len);`
`408`	`410`	`len *= array_size;`
`409`	`411`	`}`
`410`		`- isolate->AdjustAmountOfExternalAllocatedMemory(-(len + sizeof(*cb_info)));`
	`412`	`+ int64_t change_in_bytes = -static_cast<int64_t>(len + sizeof(*cb_info));`
	`413`	`+ isolate->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);`
`411`	`414`	`cb_info->p_obj.Reset();`
`412`	`415`	`cb_info->cb(data, cb_info->hint);`
`413`	`416`	`delete cb_info;`
Original file line number	Diff line number	Diff line change
`@@ -50,7 +50,8 @@ class ExternString: public ResourceType {`
`50`	`50`	`public:`
`51`	`51`	`~ExternString() {`
`52`	`52`	`delete[] data_;`
`53`		`- isolate()->AdjustAmountOfExternalAllocatedMemory(-length_);`
	`53`	`+ int64_t change_in_bytes = -static_cast<int64_t>(length_);`
	`54`	`+ isolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);`
`54`	`55`	`}`
`55`	`56`
`56`	`57`	`const TypeName* data() const {`