'Segmentation fault' when converting relatively large buffers

[bnoordhuis]

'Segmentation fault' when converting relatively large buffers

[bnoordhuis]

Confirmed, thanks for reporting.

http://github.com/ry/node/blob/81f5ed5c654c7b06e11563c36c79024265bcb7e7/src/node_buffer.cc#L150

It seems the call to NewInstance() returns a (wrapped) NULL pointer when there is stack pressure. The easy way out is to allocate memory on the heap but this might be a bug in V8 so I'm going to take a closer look.

[stevebest]

I added a test case for this issue:

stevebest/node-iconv@b5ccf1c

[stevebest]

Easy way lead to a great success! The original bug is still puzzling, but as long as we don't put convert() 100.000 calls deep down the call stack, it will probably work. :)

stevebest/node-iconv@776b712

Also, allocating memory in 4K chunks is probably suboptimal in terms of efficiency, especially since we can predict that the size of the output is roughly the same as the size of the input. But for now I think it's OK.

[bnoordhuis]

I'd written a patch already so I didn't use yours (sorry about that) but your unit test is much better than the thingy I whipped up so I added it.

I've created a nice test case on that Node bug (didn't turn out to be V8 after all). I'll keep you posted if you want.

[bnoordhuis]

PS: I bumped the temp buffers from 4 to 32K. It wasn't an issue with alloca() because that function simply bumps the stack pointer but heap allocation is rather more expensive.

we can predict that the size of the output is roughly the same as the size of the input

Hah! I originally wrote the iconv bindings to convert UTF-32 to UTF-8 (yes, there are people that actually use UTF-32. I didn't know either). dst = malloc(strlen(src)) would waste a lot of memory in such cases. But I agree that smarter heuristics won't hurt.