string_bytes: fix unaligned write in UCS2

[indutny]

Support unaligned output buffer when writing out UCS2 in
StringBytes::Write.

Fix: #2457

cc @bnoordhuis

I wasn't able to find a way to re-unify it with your code in node_buffer.cc. No test is included, as I believe there are already different unaligned stores in parallel/test-buffer.

[kzc]

Is dst unaligned here as well?

[indutny]

Hm... this is true. I'll fix it.

[indutny]

@kzc fixed, please take a look.

[kzc]

@indutny - I don't see any obvious issues. Because I don't have the specific problematic compiler in question for Linux I can't test it. Ideally this code should be tested on a big endian machine that faults when an unaligned access is attempted like Sparc or BE PowerPC (AIX?).

[indutny]

Thank you @kzc, all fixed now ;)

[indutny]

I don't mind testing it on some BE machines, but I have none at hand. cc @rvagg

[kzc]

I guess the real question is whether v8, and by extension node, supports any BE architecture. Are the iojs ARM targets BE or LE?

[skomski]

parallel/test-buffer or make check doesn't crash for me.

diff --git test/parallel/test-buffer.js test/parallel/test-buffer.js
index d5013c3..895614b 100644
--- test/parallel/test-buffer.js
+++ test/parallel/test-buffer.js
@@ -1189,3 +1189,12 @@ assert.throws(function() {
 assert.throws(function() {
   new Buffer(null);
 }, /must start with number, buffer, array or string/);
+
+// unaligned access results in a crash with gcc 5.2.0 -O3
+var text = '1234567890123456789012345678901234567890123456789';
+var len = Buffer.byteLength(text, 'ucs2');
+var buf = new Buffer(len);
+var bytesWritten = buf.write(text, 0, 'ucs2');
+
+var text = '12345678901234567890123';
+var buf = new Buffer(44).write(text, 0, 'ucs2');

[kzc]

parallel/test-buffer or make check doesn't crash for me.

@skomski using unmodified iojs 3.1.0 sources did make check crash for you when built with gcc 5.2.0?

Just curious if existing Buffer UCS2 tests would have caught it. If not, here's another test:

var assert = require('assert');
// test unaligned buffer writes with 2 byte encodings
['ucs2', 'ucs-2', 'utf16le', 'utf-16le'].forEach(function(encoding) {
  var b = new Buffer(11);
  b.write('あいうえお', 1, encoding); // unaligned
  var b2 = b.slice(1, 11);
  assert.equal(b2.toString(encoding), 'あいうえお');
});

[kzc]

@skomski the odd thing is that this Buffer write from your test should be aligned:

+var buf = new Buffer(len);
+var bytesWritten = buf.write(text, 0, 'ucs2');

Why is new Buffer(len) returning unaligned memory in the first place?

[skomski]

@kzc as I said make check currently at master or stock 3.1.0 doesn't crash and your test also doesn't trigger it.
Since we know it's some optimization from gcc's vectorizer that triggers this behaviour it could be related to SIMD types which have stricter alignment requirements.
I can only reproduce it with Buffers above length 41 and string length above 20 and if the circumstances are correct:

Crash:

var text = '12345678901234567890123';
var buf = new Buffer(44).write(text, 0, 'ucs2');

No crash:

var text = '12345678901234567890123';

var buf = new Buffer(44).write(text, 0, 'ucs2');

[kzc]

@skomski - Crash and No crash above differ only by an empty line between the statements - is this correct?

[skomski]

Yes

[kzc]

The fix in this PR aside, if the addition of an empty line in JS source can cause a Buffer's memory to be unaligned, then this is worth examining in itself. malloc()s are generally 4 or 8 byte aligned on most platforms. So this Buffer memory must be allocated by another mechanism.

[trevnorris]

Have we figured out what circumstances are necessary to cause an unaligned pointer to be returned?

[indutny]

@trevnorris the truth is harsh: https://github.com/nodejs/node/blob/master/lib/buffer.js#L67 . We do nothing to align them at the moment.

[indutny]

@skomski may I ask you to post diasm output in gdb at the time of crash? I'm particularly interested in how this v8 copy string function is compiled. i r to dump registers would be helpful too.

[trevnorris]

LGTM.

[indutny]

CI: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/124/ , thanks @trevnorris

[indutny]

Landed in 68e53dd, thank you!

[skomski]

@indutny I can do it but it should be easily reproducible for you. Just build with gcc 5.2.0 -O3. (linux x86-64)