Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Incorrect encoding of null character in buffer #394

Closed
brianc opened this Issue · 12 comments

6 participants

@brianc

In node v0.3.0 buffers are not encoding null character '\0' correctly

~$ nvm use v0.2.4
Now using node v0.2.4
~$ node
> Buffer('\0')
<Buffer 00>

~$ nvm use v0.3.0
Now using node v0.3.0
~$ node
> Buffer('\0')
<Buffer >

Totally breaks creation of null terminated strings within buffers

@Sannis

You should use Buffer('\0', 'binary') instead of Buffer('\0') with Node.js v0.3.x, because default encoding is 'utf8' now.

@brianc

In v0.2.x this worked: Buffer('!\0', 'utf8') => <Buffer 33 00> and now same thing results in <Buffer 33>

@mernst-github

I stumbled over this, too. It feels like an arbitrary workaround for a string terminator problem somewhere else. Buffer.byteLength('\u0000') actually does return 1 but new Buffer('\u0000') cuts off a trailing null byte: https://github.com/ry/node/blob/master/src/node_buffer.cc#L430

'binary' encoding works, but "this encoding method is depreciated and should be avoided in favor of Buffer objects where possible. This encoding will be removed in future versions of Node." (http://nodejs.org/docs/v0.3.2/api/buffers.html#buffers)

@mattcg

This is still a problem. In v0.4.7, new Buffer('\0', 'ascii') results in <Buffer 20>.

@bnoordhuis

It's a bug / feature in v8::String::WriteAscii() that translates nul bytes into spaces[1]. V8 has always done that but Node didn't trigger that code path in 0.2.x.

[1] https://github.com/v8/v8/blob/e3319f4/src/api.cc#L3615 (warning, big file)

@koichik koichik referenced this issue from a commit in koichik/node
@koichik koichik Fix Buffer drops last null character in UTF-8
Reproduce:

    $ node
    > buf = new Buffer('\0')
    <Buffer >
    > buf.length
    0
    > buf = new Buffer(1)
    <Buffer 28>
    > buf.write('\0')
    0

This patch fixes a part of #394.
e50a751
@koichik koichik referenced this issue from a commit in koichik/node
@koichik koichik Fix Buffer drops last null character in UTF-8
Reproduce:

    $ node
    > buf = new Buffer('\0')
    <Buffer >
    > buf.length
    0
    > buf = new Buffer(1)
    <Buffer 28>
    > buf.write('\0')
    0

Fixes #394.
Fixes #1210.
6079f1a
@bnoordhuis bnoordhuis referenced this issue from a commit in bnoordhuis/node
@bnoordhuis bnoordhuis Don't let v8::String::Utf8Write() append '\0' to the output buffer.
v8::String::Utf8Write() appends '\0' if there is room left in
the output buffer. Pass in the exact decoded length obtained
with v8::String::Utf8Length() to make it stop doing that.

Fixes #394.
22fabd4
@bnoordhuis bnoordhuis referenced this issue from a commit in bnoordhuis/node
@bnoordhuis bnoordhuis Test cases for #394, graciously stolen from Koichi Kobayash.
Assert that the trailing '\0' is not swallowed in UTF-8 input
to the Buffer constructor.
6c46304
@bnoordhuis

The problem is that V8 appends a nul byte if there is still room left in the output buffer. We can work around that by passing in the exact byte length as returned by v8::String::Utf8Length(). It's slower but correct (and not even that much slower). Ad-hoc benchmark:

buf = Buffer(64);
for (var i = 0; i < 10e6; ++i) {
  buf.write('ö日本語abcdefäëö', 0, 'utf8');
}

Current master:

$ time ./node tmp/utf8.js 
real    0m9.346s
user    0m9.350s
sys     0m0.050s

With 22fabd4 applied:

$ time ./node tmp/utf8.js 
real    0m11.213s
user    0m11.230s
sys     0m0.020s

An alternative take is to patch V8 to not append '\0' if requested. That might be necessary for #297 anyway (0x00 is converted to 0x20 in ASCII input) because there is no way right now to side-step that.

@ry Can you review?

@koichik

with 6079f1a applied?

I have tested using long long string when I wrote my first patch (e50a751), it was twice slower if I called v8::String::Utf8Length() in my environment (v0.4/V8 3.1).

@koichik

long string version:

var a = [];
for (var i = 0; i < 2000; ++i) {
  a.push('蛯原友里'); // 3bytes * 4
}
var s = a.join(''); 
var buf = new Buffer(24000);
for (i = 0; i < 10e4; ++i) {
  buf.write(s, 0, 'utf8');
}

current master:

real    0m9.739s
user    0m10.660s
sys     0m4.480s

with 22fabd4 applied:

real    0m12.709s
user    0m14.390s
sys     0m0.970s

with 6079f1a applied:

real    0m10.170s
user    0m10.580s
sys     0m5.110s
@koichik

I can't wait HINT_NO_NULL_TERMINATOR!
http://code.google.com/p/v8/issues/detail?id=1537

@bnoordhuis

I see a similar ~40% performance drop with Utf8Length() on long strings so that's out the window. But having a workaround for a workaround doesn't strike me as a great idea either, no offense to your fine work, koichik.

I suppose we should lobby the V8 guys to fix it but I suspect the patch from that issue you linked to won't pass muster.

@koichik

Never mind, I agree Ben. My patch is workaround to avoid calling Utf8Lenght().
But many Web application especially using template engine such as EJS makes long string.
So we need this workaround I think.

May I tag "v8" on this issue? :-)

@bnoordhuis

@koichik Go for it. :)

@koichik koichik closed this issue from a commit
@koichik koichik Fix Buffer drops last null character in UTF-8
Reproduce:

    $ node
    > buf = new Buffer('\0')
    <Buffer >
    > buf.length
    0
    > buf = new Buffer(1)
    <Buffer 28>
    > buf.write('\0')
    0

Fixes #394.
Fixes #1210.
5208abe
@koichik koichik closed this in 5208abe
@koichik koichik referenced this issue from a commit in koichik/node
@koichik koichik buffer: use NO_NULL_TERMINATION flag
Refs #394.
ab29da1
@koichik koichik referenced this issue from a commit
@koichik koichik buffer: use NO_NULL_TERMINATION flag
Refs #394.
Fixes #1902.
6a72e52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.