Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Commit

Permalink
src: fix segfaults, fix 32 bits integer negation
Browse files Browse the repository at this point in the history
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 #7309 but requires further bug fixes to src/smalloc.cc
that I'll address in a follow-up commit.
  • Loading branch information
bnoordhuis authored and indutny committed Mar 16, 2014
1 parent a3dca9a commit f6ea0c2
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 8 deletions.
8 changes: 4 additions & 4 deletions src/node_zlib.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
9 changes: 6 additions & 3 deletions src/smalloc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion src/string_bytes.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit f6ea0c2

Please sign in to comment.