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

Segfault in test-stringbytes-external.js #7309

Closed
cscott opened this issue Mar 14, 2014 · 7 comments
Closed

Segfault in test-stringbytes-external.js #7309

cscott opened this issue Mar 14, 2014 · 7 comments

Comments

@cscott
Copy link

cscott commented Mar 14, 2014

This test began to fail after commit ce04c72 (the previous commit 1c7bf24 did not build).

There is a compiler error which might be relevant:

  g++ '-DOPENSSL_NO_SSL2=1' '-DNODE_WANT_INTERNALS=1' '-DARCH="ia32"' '-DPLATFORM="linux"' '-DNODE_TAG=""' '-DHAVE_OPENSSL=1' '-D__POSIX__' '-DHTTP_PARSER_STRICT=0' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-D_POSIX_C_SOURCE=200112' -I../src -I../tools/msvs/genfiles -I../deps/uv/src/ares -I/home/cananian/Projects/Wikimedia/node/out/Release/obj/gen -I../deps/openssl/openssl/include -I../deps/v8/include -I../deps/zlib -I../deps/http_parser -I../deps/cares/include -I../deps/uv/include  -pthread -Wall -Wextra -Wno-unused-parameter -m32 -O3 -ffunction-sections -fdata-sections -fno-tree-vrp -fno-omit-frame-pointer -fno-rtti -fno-exceptions -MMD -MF /home/cananian/Projects/Wikimedia/node/out/Release/.deps//home/cananian/Projects/Wikimedia/node/out/Release/obj.target/node/src/string_bytes.o.d.raw  -c -o /home/cananian/Projects/Wikimedia/node/out/Release/obj.target/node/src/string_bytes.o ../src/string_bytes.cc
../src/string_bytes.cc: In static member function ‘static size_t node::StringBytes::Write(v8::Isolate*, char*, size_t, v8::Handle<v8::Value>, node::encoding, int*)’:
../src/string_bytes.cc:250:29: warning: ‘data’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     unsigned a = hex2bin(src[i * 2 + 0]);
                             ^
../src/string_bytes.cc:302:15: note: ‘data’ was declared here
   const char* data;
               ^
@bnoordhuis
Copy link
Member

FWIW, the test passes for me on Fedora 20 and OS X 10.8.5. Valgrind isn't complaining either.

The compiler warning looks like a false positive; gcc doesn't realize that the call to GetExternalParts() updates data.

@cscott
Copy link
Author

cscott commented Mar 14, 2014

It definitely segfaults for me on i386 / debian/unstable / g++ 4.8.2.
And the failures in jenkins look real to me as well.

The segfault occurs inside the // make sure Buffers from externals are the same loop in that testcase:

[...]
// make sure Buffers from externals are the same
console.log('we reach here');
for (var i = 0; i < c_bin.length; i++) {
  console.log(i, c_bin.length);
  assert.equal(c_bin[i], c_ucs[i], c_bin[i] + ' == ' + c_ucs[i] +
               ' : index ' + i);
}
console.log('dead before here');

prints:

we reach here
0 2097152
[...]
378 2097152
Segmentation fault

The final value printed is always near 380, but it varies slightly from run to run.

Stack trace:

373 2097152

Program received signal SIGSEGV, Segmentation fault.
__GI___libc_free (mem=0xb6dff008) at malloc.c:2891
2891    malloc.c: No such file or directory.
(gdb) where
#0  __GI___libc_free (mem=0xb6dff008) at malloc.c:2891
#1  0xb7ef999f in operator delete(void*) () from /usr/lib/i386-linux-gnu/libstdc++.so.6
#2  0xb7ef99eb in operator delete[](void*) () from /usr/lib/i386-linux-gnu/libstdc++.so.6
#3  0x085c674f in node::ExternString<v8::String::ExternalAsciiStringResource, char>::~ExternString() ()
#4  0x083c9c53 in v8::internal::MarkCompactCollector::AfterMarking() ()
#5  0x083d4130 in v8::internal::MarkCompactCollector::MarkLiveObjects() ()
#6  0x083d7a83 in v8::internal::MarkCompactCollector::CollectGarbage() ()
#7  0x082deb3c in v8::internal::Heap::MarkCompact(v8::internal::GCTracer*) ()
#8  0x082f364c in v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::internal::GCTracer*, v8::GCCallbackFlags) ()
#9  0x082f3bb4 in v8::internal::Heap::CollectGarbage(v8::internal::GarbageCollector, char const*, char const*, v8::GCCallbackFlags) ()
#10 0x082f3eb6 in v8::internal::Heap::CollectAllGarbage(int, char const*, v8::GCCallbackFlags) ()
#11 0x0820d16a in v8::Isolate::AdjustAmountOfExternalAllocatedMemory(long long) ()
#12 0x085c676b in node::ExternString<v8::String::ExternalAsciiStringResource, char>::~ExternString() ()
#13 0x083c9c53 in v8::internal::MarkCompactCollector::AfterMarking() ()
#14 0x083d4130 in v8::internal::MarkCompactCollector::MarkLiveObjects() ()
#15 0x083d7a83 in v8::internal::MarkCompactCollector::CollectGarbage() ()
#16 0x082deb3c in v8::internal::Heap::MarkCompact(v8::internal::GCTracer*) ()
#17 0x082f364c in v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::internal::GCTracer*, v8::GCCallbackFlags) ()
#18 0x082f3bb4 in v8::internal::Heap::CollectGarbage(v8::internal::GarbageCollector, char const*, char const*, v8::GCCallbackFlags) ()
#19 0x08480f8e in v8::internal::Runtime::PerformGC(v8::internal::Object*, v8::internal::Isolate*)
    ()
#20 0x5730aade in ?? ()
#21 0x57340b04 in ?? ()
#22 0x42c223cc in ?? ()
#23 0x42c2f440 in ?? ()
#24 0x42c21f67 in ?? ()
#25 0x42c2189d in ?? ()
#26 0x5730a624 in ?? ()
#27 0x57339db0 in ?? ()
#28 0x42c21605 in ?? ()
#29 0x5730a624 in ?? ()
#30 0x42c2e957 in ?? ()
#31 0x5730a624 in ?? ()
#32 0x57337910 in ?? ()
#33 0x573201ea in ?? ()
#34 0x0828a733 in v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, bool*, bool) ()
#35 0x0846873d in v8::internal::Runtime_Apply(int, v8::internal::Object**, v8::internal::Isolate*)
    ()
#36 0x5730a1d6 in ?? ()
#37 0x42c20163 in ?? ()
#38 0x5730a624 in ?? ()
#39 0x573783ab in ?? ()
#40 0x57339db5 in ?? ()
#41 0x573772ac in ?? ()
#42 0x57372e51 in ?? ()
#43 0x5736fe15 in ?? ()
#44 0x5735fe18 in ?? ()
#45 0x5735f97c in ?? ()
#46 0x5733f37d in ?? ()
#47 0x5733e52e in ?? ()
#48 0x57337915 in ?? ()
#49 0x573201ea in ?? ()
#50 0x0828a733 in v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, bool*, bool) ()
#51 0x0820385c in v8::Function::Call(v8::Handle<v8::Value>, int, v8::Handle<v8::Value>*) ()
#52 0x08592fec in node::Load(node::Environment*) ()
#53 0x08594369 in node::CreateEnvironment(v8::Isolate*, int, char const* const*, int, char const* const*) ()
#54 0x085944b6 in node::Start(int, char**) ()
#55 0x081ee22b in main ()

Hope that helps.

@cscott
Copy link
Author

cscott commented Mar 14, 2014

make test-valgrind on 1f17f88 emits lots of errors for me as well. I'll attach a pastebin once the run finishes.

@bnoordhuis
Copy link
Member

Thanks, I've been able to reproduce it on ia32. x64 is unaffected for some reason.

@cscott
Copy link
Author

cscott commented Mar 14, 2014

As with all fun bugs, I'm sure the reason will become obvious once you track down the bug. ;)

I'm still only 37% of the way through the make test-valgrind. I'll assume that I don't need to post the results of that, unless you say otherwise.

@bnoordhuis
Copy link
Member

I suppose most of valgrind's complaints are about memory leaks? They're legitimate but harmless: node doesn't free resources on exit because there's no point, the operating system takes care of it.

@cscott
Copy link
Author

cscott commented Mar 14, 2014

@bnoordhuis many, but not all of them. Here are the other interesting lines from the output (so far):

 Conditional jump or move depends on uninitialised value(s)
 Mismatched free() / delete / delete []
 Syscall param sendmsg(msg.msg_iov[0]) points to uninitialised byte(s)
 Syscall param writev(vector[...]) points to uninitialised byte(s)
 Warning: invalid file descriptor 13337 in syscall readv()
AssertionError: "/usr/bin/valgrind.bin --error-exitcode=1 --leak-check=full --smc-check=all out/Rel
ease/node /home/cananian/Projects/Wikimedia/n == "testme"
AssertionError: "ENOTSOCK" == "EINVAL"
AssertionError: "PONG" == "AssertionError: Data should not be undefined\n    at Domain.<anonymous> 
AssertionError: "upgrade error" == "listen EADDRINUSE"
AssertionError: 0 == 1
AssertionError: false == true

The tests with non-leak errors:

CONDITIONAL JUMP OR MOVE DEPENDS ON UNINITIALIZED...
test/simple/test-buffer.js
test/simple/test-pipe.js
test/simple/test-querystring.js
test/simple/test-readline-interface.js
test/simple/test-smalloc.js

MISMATCHED FREE:
test/simple/test-asynclistener.js
/test/simple/test-asynclistener-error.js
test/simple/test-crypto-certificate.js
test/simple/test-domain.js
test/simple/test-domain-implicit-fs.js
test/simple/test-file-read-noexist.js
test/simple/test-file-write-stream.js
test/simple/test-file-write-stream2.js
test/simple/test-file-write-stream3.js
test/simple/test-fs-append-file.js
[...etc...]

SYSCALL PARAM:
test/simple/test-dgram-msgsize.js
test/simple/test-fs-write-stream-err.js

INVALID FILE DESCRIPTOR:
test/simple/test-fs-read-stream.js

ASSERTION ERROR:
test/simple/test-child-process-execsync.js
test/simple/test-child-process-fork-dgram.js
test/simple/test-child-process-fork-net2.js
test/simple/test-child-process-spawnsync.js
test/simple/test-child-process-spawnsync-timeout.js
test/simple/test-child-process-stdout-flush-exit.js
test/simple/test-cluster-dgram-2.js
test/simple/test-cluster-disconnect.js
test/simple/test-debug-signal-cluster.js
test/simple/test-fs-empty-readStream.js
test/simple/test-http-upgrade-server2.js
[...etc...]

I'm currently at ~79% completion. valgrind is slow!

bnoordhuis added a commit to bnoordhuis/node that referenced this issue Mar 15, 2014
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.
bnoordhuis added a commit to bnoordhuis/node that referenced this issue Mar 15, 2014
Fix a regression that was introduced in commit ce04c72 after the
upgrade to V8 3.24.

The new weak persistent handle API no longer gives you the original
persistent but still requires that you clear it inside your weak
callback.

Rearrange the code in src/smalloc.cc to keep track of the persistent
handle with the least amount of pain and try hard to share as much
code as possible between the 'just free it' and 'invoke my callback'
versions of the smalloc API.

Fixes nodejs#7309.
indutny pushed a commit that referenced this issue Mar 16, 2014
Fix a regression that was introduced in commit ce04c72 after the
upgrade to V8 3.24.

The new weak persistent handle API no longer gives you the original
persistent but still requires that you clear it inside your weak
callback.

Rearrange the code in src/smalloc.cc to keep track of the persistent
handle with the least amount of pain and try hard to share as much
code as possible between the 'just free it' and 'invoke my callback'
versions of the smalloc API.

Fixes #7309.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants