buffer: use-after-free when deleting buffer.parent

[bnoordhuis]

Deleting the .parent property on a Buffer instance may cause a use-after-free error when the buffer is later indexed into.

require('assert')(typeof gc === 'function', 'add --expose-gc');
var SlowBuffer = process.binding('buffer').SlowBuffer;
var slow = new SlowBuffer(1 << 16);
var fast = new Buffer(slow, slow.length, 0);
fast.parent = null;
slow = null;
while (gc());
for (var i = 0; i < fast.length; ++i) fast[i] = 0xAA;
for (;;) new SlowBuffer(1 << 16);

$ out/Release/node --expose-gc tmp/slowbuffer.js
node(20404,0x7fff7e6f3180) malloc: *** error for object 0x101812a08: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug
Abort trap: 6

[trevnorris]

@bnoordhuis the syntax new Buffer(slow, slow.length, 0); isn't officially documented, and has been removed in my PR changes. That a problem?

Also, I don't understand how this is unexpected. The only reference v8 has to keep the SlowBuffer alive is the .parent reference on Buffer. As soon as that's removed then v8 thinks it can be cleaned up. Which calls ~ObjectWrap and ~Buffer, which calls Replace, which then free's the mem. Though I am curious how you're getting that error. Only thing I ever get is a Segmentation Fault.

[bnoordhuis]

the syntax new Buffer(slow, slow.length, 0); isn't officially documented, and has been removed in my PR changes. That a problem?

No, that's only to make it easier to demonstrate the issue.

Also, I don't understand how this is unexpected.

It's not unexpected but 'not unexpected' doesn't imply 'not a bug.'

[trevnorris]

Alright. Understand now.

So it is technically a bug that when .parent == null on a buffer the external array data gets cleaned up by gc. Known about this for a while but didn't consider this a bug since I always saw it as expected functionality (thanks @bnoordhuis for clearing that up =).

Solutions are to make it ReadOnly, or to set it as an internal/hidden field. Unfortunately both these add a substantial performance hit to Buffer instantiation. So for now I'd move on keeping it how it is, and possibly documenting it.

This ticket might be open for a while, but I'll keep it in mind as the v8 api performance improves in these related areas.

There is one other possibility that we may want to consider. Currently malloc only takes ~5% of performance time on instantiation. I had already played with removing buffer pools and the SlabAllocator. The performance hit that prevented this change is from v8 needing to Persist, MakeWeak, etc. It came close, but was still a regression.

Currently I'm looking into a way to allow SetIndexedPropertiesToExternalArrayData to auto-cleanup allocated memory on gc. If this works then it could easily surpass the regression.

[bnoordhuis]

Fixed by 3a2f273.

[trevnorris]

Fwiw this could still happen if a user overrides the parent attribute when the buffer is specifically from a slice.

[bnoordhuis]

Okay. Reopen if you think it's worth fixing (and possible to fix.)

[trevnorris]

It is possible, but right now not without a nontrivial performance regression. I'll continue to investigate a better way.

[bnoordhuis]

@trevnorris Any news?

[trevnorris]

We take the performance hit of attaching this internally or leave as is.
Not much different than if a user decides to screw with buffer.length
pre-my-rewite. Or, in the ridiculously tiny chance, Buffers are rewritten
to inherent from ArrayBuffer then it won't be an issue.

Imo it's important we save all the time we can, because as of a recent v8
update instantiation time has increased 30%.

[trevnorris]

@bnoordhuis this isn't going to be fixed any time soon. Either adding the attribute in C++ and making it READONLY or using Object.defineProperty(), the operation becomes 4x and 10x slower respectively. So we can close this, or leave it open as just a known bug, but won't fix.

[trevnorris]

Just an update. This will be fixed if we move Buffer() to be backed by Uint8Array(), but that can only happen when v8 allows extending of natives.

[jasnell]

@trevnorris @vkurchatkin ... is this still an issue?

[trevnorris]

@jasnell Yes. The only two solutions are to either make the .parent read-only or make it a hidden field. Either of those are very expensive, and you might as well only use SlowBuffer instead (which still doesn't solve the .slice() case). This will be solved in V8 4.4 as the entire API that Buffer currently uses will be removed, and the only solution is to use Uint8Array.

[jasnell]

Ok, leaving a reference to nodejs/node#1810 here. Can hopefully close this once that lands.

[jasnell]

nodejs/node#1810 closed. Will pick up the solution in the converged repo. Possibly could investigate a backport.