Crash when working with node-ffi on Windows and Mac

[rosen-vladimirov]

Hi,

One of the popular npm modules is node-ffi. We are using it in our code for several different calls, but one of them leads to assertion error in node 3.x or later. As the same code of the node-ffi is working fine with iojs 2.x, node 0.12.x and node 0.10.x and after discussion with node-ffi owners, I came to opening this issue.

In our code we are trying to call methods from CoreFoundation.dll, which is part of iTunes installation. When we have some short calls and our process terminates, everything is working fine. But when our process starts working for some long time, we receive Assertion error:

Assertion failed: (obj_data) != (nullptr), file src\node_buffer.cc,  line 150

It looks like the garbage collector had collected something that we are trying to use later.
So we've tried to simplify the reproduction case and we've found that we fail only when trying to create ForeignFunction for specific method of the dll. Please note - we are not using them in the test script, just using ffi.

You can find the repro script and the output when DEBUG=* in this gist
Please note that the crash is on global.gc() call.

I believe the problem is not in the dll itself, as the same code is working fine with node 0.10, node 0.12, iojs 2.x

We suspect that this change had not fixed all issues related to buffers and garbage collection.

The original issue in node-ffi is here. After some time of debugging, @unbornchikken stated:

The Windows one is interesting. I've tested our latest ffi with many-many DLL-s referenced, including OpenCL from various vendors, and everything works properly. However if I reference that iTunes DLL then I get a crash. I think iTunes is doing some library initialization logic that somehow corrupts io.js 3+'s memory. I can see in the debugger that the error is about GC thinks that memory is occupied but instead it gets nullified somehow.

Could you please advise what's going wrong here? I know that it is related to specific npm module, not to the node itself, but the same module is working fine in all other cases and node versions, only the call to this specific method is failing on iojs 3.x and node 4.0.0.

Thanks in advance for your help!

[unbornchikken]

I think we're having this very same error on MAC too there: node-ffi/node-ffi#239 This is the most simplest repro case that we have. Interesting that this only happens on MAC, instead of the above Windows only case.

[unbornchikken]

@indutny What do you think? Does it related to #2484?

[indutny]

@unbornchikken all buffer memory should be aligned now, except the slices that are created by user code. In other words:

var a = new Buffer(8);  // aligned
var b = a.slice(1); // not-aligned, pointing to the interior of `a`

Do you have any test case to reproduce the problem?

[indutny]

I mean for mac?

[unbornchikken]

Please find the intro comment there: node-ffi/node-ffi#239 Very easy to reproduce.

[indutny]

Confirmed:

    frame #0: 0x000000010021765c node`v8::internal::Map::instance_type(this=0x535641e5894855c1) + 12 at objects-inl.h:4173
  * frame #1: 0x000000010061a0c2 node`v8::internal::ShortCircuitConsString(p=0x0000317ae71581e8) + 50 at mark-compact.cc:1198
    frame #2: 0x000000010061ddd6 node`v8::internal::MarkCompactMarkingVisitor::MarkObjectByPointer(collector=0x0000000103006240, anchor_slot=0x0000317ae71581d8, p=0x0000317ae71581e8) + 54 at mark-compact.cc:1274
    frame #3: 0x000000010061f6e0 node`v8::internal::MarkCompactMarkingVisitor::VisitPointers(heap=0x0000000103000020, start=0x0000317ae71581d8, end=0x0000317ae7158210) + 144 at mark-compact.cc:1249
    frame #4: 0x000000010062428b node`v8::internal::StaticMarkingVisitor<v8::internal::MarkCompactMarkingVisitor>::VisitJSArrayBuffer(map=0x00003f71dd20b219, object=0x0000317ae71581d1) + 91 at objects-visiting-inl.h:506
    frame #5: 0x000000010061ccbd node`v8::internal::StaticMarkingVisitor<v8::internal::MarkCompactMarkingVisitor>::IterateBody(map=0x00003f71dd20b219, obj=0x0000317ae71581d1) + 45 at objects-visiting.h:403
    frame #6: 0x0000000100610f17 node`v8::internal::MarkCompactCollector::EmptyMarkingDeque(this=0x0000000103006240) + 439 at mark-compact.cc:2001
    frame #7: 0x00000001006206d9 node`v8::internal::RootMarkingVisitor::MarkObjectByPointer(this=0x00007fff5fbfe728, p=0x0000000103006aa8) + 265 at mark-compact.cc:1667
    frame #8: 0x00000001006205ad node`v8::internal::RootMarkingVisitor::VisitPointer(this=0x00007fff5fbfe728, p=0x0000000103006aa8) + 29 at mark-compact.cc:1637
    frame #9: 0x0000000100726094 node`v8::internal::Isolate::Iterate(this=0x0000000103000000, v=0x00007fff5fbfe728, thread=0x0000000103006aa0) + 164 at isolate.cc:189
    frame #10: 0x0000000100726265 node`v8::internal::Isolate::Iterate(this=0x0000000103000000, v=0x00007fff5fbfe728) + 53 at isolate.cc:208
    frame #11: 0x00000001005d25c4 node`v8::internal::Heap::IterateStrongRoots(this=0x0000000103000020, v=0x00007fff5fbfe728, mode=VISIT_ONLY_STRONG) + 228 at heap.cc:5300
    frame #12: 0x0000000100610aa1 node`v8::internal::MarkCompactCollector::MarkRoots(this=0x0000000103006240, visitor=0x00007fff5fbfe728) + 49 at mark-compact.cc:1937
    frame #13: 0x000000010060bd90 node`v8::internal::MarkCompactCollector::MarkLiveObjects(this=0x0000000103006240) + 368 at mark-compact.cc:2264
    frame #14: 0x000000010060baa4 node`v8::internal::MarkCompactCollector::CollectGarbage(this=0x0000000103006240) + 100 at mark-compact.cc:338
    frame #15: 0x00000001005c2b2d node`v8::internal::Heap::MarkCompact(this=0x0000000103000020) + 173 at heap.cc:1414
    frame #16: 0x00000001005c1993 node`v8::internal::Heap::PerformGarbageCollection(this=0x0000000103000020, collector=MARK_COMPACTOR, gc_callback_flags=kGCCallbackFlagForced) + 499 at heap.cc:1278
    frame #17: 0x00000001005c134b node`v8::internal::Heap::CollectGarbage(this=0x0000000103000020, collector=MARK_COMPACTOR, gc_reason="Isolate::RequestGarbageCollection", collector_reason="GC in old space requested", gc_callback_flags=kGCCallbackFlagForced) + 763 at heap.cc:949
    frame #18: 0x0000000100264f63 node`v8::internal::Heap::CollectGarbage(this=0x0000000103000020, space=OLD_SPACE, gc_reason="Isolate::RequestGarbageCollection", callbackFlags=kGCCallbackFlagForced) + 83 at heap-inl.h:513
    frame #19: 0x00000001005c0ab5 node`v8::internal::Heap::CollectAllGarbage(this=0x0000000103000020, flags=2, gc_reason="Isolate::RequestGarbageCollection", gc_callback_flags=kGCCallbackFlagForced) + 69 at heap.cc:828
    frame #20: 0x0000000100251609 node`v8::Isolate::RequestGarbageCollectionForTesting(this=0x0000000103000000, type=kFullGarbageCollection) + 313 at api.cc:7086
    frame #21: 0x0000000100543d43 node`v8::internal::GCExtension::GC(args=0x00007fff5fbfeb80) + 131 at gc-extension.cc:20
    frame #22: 0x000000010028f9bc node`v8::internal::FunctionCallbackArguments::Call(this=0x00007fff5fbfecc8, f=(node`v8::internal::GCExtension::GC(v8::FunctionCallbackInfo<v8::Value> const&) at gc-extension.cc:19))(v8::FunctionCallbackInfo<v8::Value> const&)) + 124 at arguments.cc:33
    frame #23: 0x00000001002db722 node`v8::internal::MaybeHandle<v8::internal::Object> v8::internal::HandleApiCallHelper<false>(isolate=0x0000000103000000, args=0x00007fff5fbfee48)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>&) + 1426 at builtins.cc:1092
    frame #24: 0x00000001002e5303 node`v8::internal::Builtin_Impl_HandleApiCall(args=v8::internal::(anonymous namespace)::HandleApiCallArgumentsType @ 0x00007fff5fbfee48, isolate=0x0000000103000000)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) + 131 at builtins.cc:1115
    frame #25: 0x00000001002dbd50 node`v8::internal::Builtin_HandleApiCall(args_length=2, args_object=0x00007fff5fbfeef0, isolate=0x0000000103000000) + 80 at builtins.cc:1111

[unbornchikken]

On Windows with that iTunes DLL I have the very same stack trace, that's why I think those are related. This gotta be some pointer layout issue again, ain't this?

[indutny]

Sorry, but I won't be able to finish it today (it is already 4:28 am), will continue looking tomorrow!

[indutny]

I know what's happening, it tries to treat the internal data as a pointer to the heap value. Namely, BodyDescriptor of JSArrayBuffer needs to skip the backing store pointer when iterating through it. I have a fix, going to finish it up tomorrow.

[unbornchikken]

Yup, makes sense. That was my first guess for #2484 too.

[rosen-vladimirov]

Thanks guys, I'm ready to help with testing of the fix :)

[indutny]

Candidate fix is:

diff --git a/deps/v8/src/heap/objects-visiting-inl.h b/deps/v8/src/heap/objects-visiting-inl.h
index 0103054..09b1d48 100644
--- a/deps/v8/src/heap/objects-visiting-inl.h
+++ b/deps/v8/src/heap/objects-visiting-inl.h
@@ -81,10 +81,8 @@ int StaticNewSpaceVisitor<StaticVisitor>::VisitJSArrayBuffer(
     Map* map, HeapObject* object) {
   Heap* heap = map->GetHeap();

-  VisitPointers(
-      heap,
-      HeapObject::RawField(object, JSArrayBuffer::BodyDescriptor::kStartOffset),
-      HeapObject::RawField(object, JSArrayBuffer::kSizeWithInternalFields));
+  JSArrayBuffer::BodyDescriptor::IterateBody<
+      StaticNewSpaceVisitor<StaticVisitor> >(heap, object);
   if (!JSArrayBuffer::cast(object)->is_external()) {
     heap->RegisterLiveArrayBuffer(true,
                                   JSArrayBuffer::cast(object)->backing_store());
@@ -503,10 +501,7 @@ void StaticMarkingVisitor<StaticVisitor>::VisitJSArrayBuffer(
     Map* map, HeapObject* object) {
   Heap* heap = map->GetHeap();

-  StaticVisitor::VisitPointers(
-      heap,
-      HeapObject::RawField(object, JSArrayBuffer::BodyDescriptor::kStartOffset),
-      HeapObject::RawField(object, JSArrayBuffer::kSizeWithInternalFields));
+  JSArrayBuffer::BodyDescriptor::IterateBody<StaticVisitor>(heap, object);
   if (!JSArrayBuffer::cast(object)->is_external()) {
     heap->RegisterLiveArrayBuffer(false,
                                   JSArrayBuffer::cast(object)->backing_store());
diff --git a/deps/v8/src/objects-inl.h b/deps/v8/src/objects-inl.h
index fbc2c4e..1e2fe15 100644
--- a/deps/v8/src/objects-inl.h
+++ b/deps/v8/src/objects-inl.h
@@ -6091,6 +6091,34 @@ void JSArrayBuffer::set_is_shared(bool value) {
 }


+// static
+template<typename StaticVisitor>
+void JSArrayBuffer::BodyDescriptor::IterateBody(Heap* heap, HeapObject* obj) {
+  StaticVisitor::VisitPointers(
+      heap,
+      HeapObject::RawField(obj, JSArrayBuffer::BodyDescriptor::kStartOffset),
+      HeapObject::RawField(obj, JSArrayBuffer::kBackingStoreOffset));
+  StaticVisitor::VisitPointers(
+      heap,
+      HeapObject::RawField(obj,
+                           JSArrayBuffer::kBackingStoreOffset + kPointerSize),
+      HeapObject::RawField(obj, JSArrayBuffer::kSizeWithInternalFields));
+}
+
+
+void JSArrayBuffer::BodyDescriptor::IterateBody(HeapObject* obj,
+                                                ObjectVisitor* v) {
+  v->VisitPointers(
+      HeapObject::RawField(obj, JSArrayBuffer::BodyDescriptor::kStartOffset),
+      HeapObject::RawField(obj, JSArrayBuffer::kBackingStoreOffset));
+  v->VisitPointers(
+      HeapObject::RawField(obj,
+                           JSArrayBuffer::kBackingStoreOffset + kPointerSize),
+      HeapObject::RawField(obj, JSArrayBuffer::kSizeWithInternalFields));
+}
+
+
+
 Object* JSArrayBufferView::byte_offset() const {
   if (WasNeutered()) return Smi::FromInt(0);
   return Object::cast(READ_FIELD(this, kByteOffsetOffset));
diff --git a/deps/v8/src/objects.cc b/deps/v8/src/objects.cc
index 2b042fd..efc168c 100644
--- a/deps/v8/src/objects.cc
+++ b/deps/v8/src/objects.cc
@@ -1420,7 +1420,6 @@ void HeapObject::IterateBody(InstanceType type, int object_size,
     case JS_VALUE_TYPE:
     case JS_DATE_TYPE:
     case JS_ARRAY_TYPE:
-    case JS_ARRAY_BUFFER_TYPE:
     case JS_TYPED_ARRAY_TYPE:
     case JS_DATA_VIEW_TYPE:
     case JS_SET_TYPE:
@@ -1436,6 +1435,9 @@ void HeapObject::IterateBody(InstanceType type, int object_size,
     case JS_MESSAGE_OBJECT_TYPE:
       JSObject::BodyDescriptor::IterateBody(this, object_size, v);
       break;
+    case JS_ARRAY_BUFFER_TYPE:
+      JSArrayBuffer::BodyDescriptor::IterateBody(this, v);
+      break;
     case JS_FUNCTION_TYPE:
       reinterpret_cast<JSFunction*>(this)
           ->JSFunctionIterateBody(object_size, v);
diff --git a/deps/v8/src/objects.h b/deps/v8/src/objects.h
index 7e4fcba..8b62720 100644
--- a/deps/v8/src/objects.h
+++ b/deps/v8/src/objects.h
@@ -10027,6 +10027,18 @@ class JSArrayBuffer: public JSObject {
   static const int kSizeWithInternalFields =
       kSize + v8::ArrayBuffer::kInternalFieldCount * kPointerSize;

+  class BodyDescriptor {
+   public:
+    static const int kStartOffset = JSObject::BodyDescriptor::kStartOffset;
+    static const int kEndOffset = kSizeWithInternalFields - kPointerSize;
+    static const int kSize = kSizeWithInternalFields;
+
+    template<typename StaticVisitor>
+    static inline void IterateBody(Heap* heap, HeapObject* obj);
+
+    static inline void IterateBody(HeapObject* obj, ObjectVisitor* v);
+  };
+
   class IsExternal : public BitField<bool, 1, 1> {};
   class IsNeuterable : public BitField<bool, 2, 1> {};
   class WasNeutered : public BitField<bool, 3, 1> {};

Working on a test for v8.

[indutny]

CL: https://codereview.chromium.org/1327403002/

[indutny]

Unfortunately, we can't land it as it is, because I'm not sure if the supplied fix covers all edge cases in v8. Let's wait until it will land in upstream, and backport it to our v8.

[unbornchikken]

Cannot we invent a workaround? I'm thinking about that we have a GC callback at Buffer side, so we can hook in some behavior before GC happens. So cannot that poiner be nullified there before v8 could even touch it?

[unbornchikken]

And in we I mean you of course. :)

[indutny]

@unbornchikken this is should land in v8 upstream pretty soon. All of my previous CLs were reviewed and landed in days, depending on how many changes were required before the green light.

Workaround might be to allocate buffers aligned and slice them up to get the proper address... I think it should work?

[indutny]

But again, I wouldn't go too far with this. Perhaps 4.0.1 with this fix will be released fast enough.

[unbornchikken]

In upstream we trust then. Thanks.

[indutny]

Fixed by 2b8a06b. Thanks for report!

[unbornchikken]

Maybe they talks to each other in person, just like other(than us) human beings. :)

[rosen-vladimirov]

Thank you very much guys, I'll test the fix very soon and I'll inform you for the results.
Great work @indutny and thanks for the cooperation guys!!! You are awesome!

[TooTallNate]

@indutny can we backport this to io.js v3 (assuming we're still doing patch releases for it)?

[indutny]

I don't mind, but can't say that I am ready to invest time into it. If someone will open a PR - I will review it.