Odd Buffer behavior that leads to crash in iojs 3.0

[unbornchikken]

Hi,

We're in progress to do a PR to popular ffi module for supporting io.js 3.0. It relies on the ref module to do its kinda pointer stuff, so it is important to make ref module to support io.js 3.0 at first.

There I'm stuck with an issue that is not related to ref, that is related to io.js instead, especially to its new Buffer implementation.

I've isolated a repro case there: https://github.com/unbornchikken/ref/commit/270ca83dc3a85772ec60fba66d58920befd98d01 There is a unit test in test/iojs3issue.js:

describe('iojs3issue', function () {
    it('should not crash', function() {
        for (var i = 0; i < 10; i++) {
            gc()
            var buf = new Buffer(8)
            buf.fill(0)
            var buf2 = ref.ref(buf)
            var buf3 = ref.deref(buf2)
        }
    })
    it('should crash', function() {
        for (var i = 0; i < 10; i++) {
            gc()
            var buf = new Buffer(7)
            buf.fill(0)
            var buf2 = ref.ref(buf)
            var buf3 = ref.deref(buf2)
        }
    })
})

Fist one doesn't crash but the second does.

The explanation of the code:

ref.ref(buf) means that:

• allocate a Buffer of size of a pointer: https://github.com/unbornchikken/ref/blob/master/lib/ref.js#L508, and threat its data as a pointer
• copy address of buf's data to that pointer: https://github.com/unbornchikken/ref/blob/io.js-3.0-support/src/binding.cc#L278

So we have a buffer that has 8 bytes (on x64) that is actually a pointer to address of buff's data.

ref.deref(buf2) means that:

• create a Buffer with its data pointing the location of the pointer created like the above. https://github.com/unbornchikken/ref/blob/io.js-3.0-support/src/binding.cc#L249 (WrapPointer: https://github.com/unbornchikken/ref/blob/io.js-3.0-support/src/binding.cc#L145)

Despite the described magic, in the above tests we end up having two Buffers have exactly the same memory location in those data pointer, and the first one is the owner, it has to free the memory, the other one is just referencing that (because there is an empty free callback at WrapPointer: https://github.com/unbornchikken/ref/blob/io.js-3.0-support/src/binding.cc#L145).

Now, that situation lead to a crash when invoking a GC. It seems somehow the size of the original buffer affecting the crash, because if it 7 or lower it won't happen, only after 8. The other weird thing is that if I modify the loop in the second test to have end condition of 2, the crash won't happen. It seems the trigger is 3. So it have to be some internal optimization that take account and somehow ignores the fact that there is a free callback for the second buffer, and tries to free it memory that is at that time already freed.

I've launched a debugger, and got the stack trace of the crash, but I don't have enough v8 skills to decrypt what happened:

>   iojs.exe!v8::internal::Map::instance_type() Line 4557   C++
    iojs.exe!v8::internal::ShortCircuitConsString(v8::internal::Object * * p) Line 1288 C++
    iojs.exe!v8::internal::MarkCompactMarkingVisitor::MarkObjectByPointer(v8::internal::MarkCompactCollector * collector, v8::internal::Object * * anchor_slot, v8::internal::Object * * p) Line 1364   C++
    iojs.exe!v8::internal::MarkCompactMarkingVisitor::VisitPointers(v8::internal::Heap * heap, v8::internal::Object * * start, v8::internal::Object * * end) Line 1340  C++
    iojs.exe!v8::internal::StaticMarkingVisitor<v8::internal::MarkCompactMarkingVisitor>::VisitJSArrayBuffer(v8::internal::Map * map, v8::internal::HeapObject * object) Line 538   C++
    iojs.exe!v8::internal::StaticMarkingVisitor<v8::internal::MarkCompactMarkingVisitor>::IterateBody(v8::internal::Map * map, v8::internal::HeapObject * obj) Line 405 C++
    iojs.exe!v8::internal::MarkCompactCollector::EmptyMarkingDeque() Line 2113  C++
    iojs.exe!v8::internal::RootMarkingVisitor::MarkObjectByPointer(v8::internal::Object * * p) Line 1763    C++
    iojs.exe!v8::internal::RootMarkingVisitor::VisitPointer(v8::internal::Object * * p) Line 1732   C++
    iojs.exe!v8::internal::GlobalHandles::IterateWeakRoots(v8::internal::ObjectVisitor * v) Line 608    C++
    iojs.exe!v8::internal::MarkCompactCollector::MarkLiveObjects() Line 2381    C++
    iojs.exe!v8::internal::MarkCompactCollector::CollectGarbage() Line 339  C++
    iojs.exe!v8::internal::Heap::MarkCompact() Line 1300    C++
    iojs.exe!v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector collector, const v8::GCCallbackFlags gc_callback_flags) Line 1185  C++
    iojs.exe!v8::internal::Heap::CollectGarbage(v8::internal::GarbageCollector collector, const char * gc_reason, const char * collector_reason, const v8::GCCallbackFlags gc_callback_flags) Line 911  C++
    iojs.exe!v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace space, const char * gc_reason, const v8::GCCallbackFlags callbackFlags) Line 536  C++
    iojs.exe!v8::internal::Heap::CollectAllGarbage(int flags, const char * gc_reason, const v8::GCCallbackFlags gc_callback_flags) Line 796 C++
    iojs.exe!v8::Isolate::RequestGarbageCollectionForTesting(v8::Isolate::GarbageCollectionType type) Line 6747 C++
    iojs.exe!v8::internal::GCExtension::GC(const v8::FunctionCallbackInfo<v8::Value> & args) Line 24    C++
    iojs.exe!v8::internal::FunctionCallbackArguments::Call(void (const v8::FunctionCallbackInfo<v8::Value> &) * f) Line 34  C++
    iojs.exe!v8::internal::HandleApiCallHelper<0>(v8::internal::Isolate * isolate, v8::internal::`anonymous-namespace'::BuiltinArguments<1> & args) Line 1110   C++
    iojs.exe!v8::internal::Builtin_Impl_HandleApiCall(v8::internal::`anonymous-namespace'::BuiltinArguments<1> args, v8::internal::Isolate * isolate) Line 1133 C++
    iojs.exe!v8::internal::Builtin_HandleApiCall(int args_length, v8::internal::Object * * args_object, v8::internal::Isolate * isolate) Line 1128  C++

[bnoordhuis]

What happens with v3.1.0? What happens with a debug build of v3.1.0?

[kkoopa]

Is this related? #2457

[kzc]

Doesn't appear to be related to #2457 which was UCS2 encoding specific - aside from the (literally) odd Buffer memory alignment.

As simple as the test case above appears, it does involve some external native methods behind ref and deref. My first impression is there was something amiss with object scopes and rooting while raw buffer memory was being read or written.

[unbornchikken]

The above code did not crashed on previous versions, and I can see nothing extraordinary in its native side. That should just work that way, just a few assignment. I did not tried it with 3.1. I'm gonna do it and reply.

[kzc]

I appreciate it didn't crash pre 3.0.0, but there's a newer v8 engine. Maybe some behavior changed regarding GC and JS object rooting.

[unbornchikken]

I'm pretty sure about that's the case. That's why I bought it to the professionals. It's very simple stuff, that doesn't do anything complicated, but somehow it kills the garbage collector.

[kzc]

aside from the (literally) odd Buffer memory alignment.

@unbornchikken @kkoopa @indutny - The unaligned memory from the Buffer rewrite appears to be the cause of this ref module crash as well as #2457.

With the patch in #2487 the test above runs cleanly on Linux. Without it, it crashes.

[indutny]

Unaligned memory, no-one likes it ;)

[kzc]

@indutny - your March 11 Buffer align PR fell through the cracks?

[indutny]

@kzc yeah, I believe that the buffer code was rewritten in 3.x and these changes were gone.

[kzc]

@indutny Buffer performance should improve with this align fix.

[trevnorris]

@kzc Don't be hopeful. Buffer performance has only degraded since v0.12. Nothing that comes from our API either.

[kzc]

@trevnorris regarding Buffer performance - I'll take your word for it.

A somewhat related question - Is there a design doc for Buffers?

Please correct my hazy and quite possibly erroneous understanding of how lib/buffer.js works: a series of 8K memory pools are created for Buffer instances to use in a first-come, first-serve fashion controlled by a global current allocPool - with the exception that Buffer instances of size 4096 and larger will each get their own exclusive memory pool. A number of unrelated Buffer instances (of size less than 4096 each) share the current global 8K allocPool. When the current 8K allocPool is filled, another is created for subsequent new Buffer instances. New Buffers will never reuse an old partially filled pool even if there is sufficient space available. The memory for each previous 8K allocPool will only be reclaimed by v8 once all of its individual Buffer instances are no longer referenced. I don't see a buffer pools compaction scheme - is there one?

[indutny]

@kzc I think your understanding of the structure is quite correct.

[trevnorris]

@kzc For reference of Buffer instantiation times: https://gist.github.com/trevnorris/efe36274b0d0a23a78f5

There isn't a compaction scheme. It would be more performance expensive to do so than just allocating a completely new buffer on every request. Also, understand this is only the case with using new Buffer(). Any buffers created from I/O operations, and similar, have their own allocated memory for the given operation. This case is generally the more common.

[unbornchikken]

Gentlemen, thanks for your time. @kzc, has #2784 landed to the master? Which io.js version should I expect to work with my ref and ffi PRs?

[kzc]

@unbornchikken - Yes, @indutny pushed his alignment fix to master. Can only guess the future iojs version number, but this particular PR wouldn't introduce any incompatibilities to 3.1.0.

[unbornchikken]

So there is a big fat +1 for integrate this to the next version. It's a miracle that people haven't started to rage about io.js 3 support at ffi yet. :)

[kzc]

@unbornchikken - did you test your code against master to see if it works on Windows?

[unbornchikken]

@kzc not yet. But on Linux it works. Since the stack trace of the error was almost the same, I'm sure about that resolves it on Windows too. I'll try it as soon as I can, and if the issue still persists, I'm gonna reopen this issue.