buffer: make SlowBuffer constructor check that argument is a number

[targos]

Fixes: #2634

[targos]

R=@trevnorris

[thefourtheye]

Will Number.isInteger be better here?

[targos]

this would change the behavior for NaN or Infinity

[kzc]

Shouldn't type be checked first? Would short circuit quicker for invalid types.

  if (typeof length !== 'number' || length < 0)

[targos]

It would, but invalid types are not the common case

[kzc]

Both expressions are evaluated for positive integers anyway. So there is only upside to checking type first.

[targos]

You are right. I changed it, thanks

[thefourtheye]

Semver-major?

[targos]

@thefourtheye It is a bug fix. Before it would make the process abort.

[Fishrock123]

LGTM

[cjihrig]

LGTM

[ChALkeR]

Well, this actually defines the behavior of new SlowBuffer('string') to return a zero-size SlowBuffer without any exceptions. Is this correct?
It does not correspond to Buffer behaviour, it does not correspond to the old behaviour, nor does it make any sense to me. I am not sure about this one. @trevnorris, what do you think?
Note that any change here after this gets merged would be a semver-major.

[targos]

@ChALkeR it is correct.
What is the old behavior you are talking about? 0.12 and 2.5.0 also return a zero-size buffer when called with non-numbers

[ChALkeR]

@targos 2.5.0 and 0.12 returned zero-sized buffers for new SlowBuffer('string')? Sorry, I might have missed it.

If so, LGTM.

[targos]

@ChALkeR exactly. But I did more tests after my last answer and found that in those versions, you can pass the number as a string, a boolean, or some other special object:

Now using io.js v2.5.0 (npm v2.13.2)
~ % node
> buffer.SlowBuffer('4')
<Buffer b0 5f 80 02>
> buffer.SlowBuffer(true)
<Buffer 00>
> var v = { valueOf: function(){ return 5; } };
> buffer.SlowBuffer(v)
<Buffer 00 00 00 00 00>

See: https://github.com/nodejs/node/blob/v2.5.0/lib/buffer.js#L181

Do we want to restore this behavior ?

[trevnorris]

Need to include ../common even though you're not using it.

[targos]

Ah, didn't know about that. What is the reason ?

[trevnorris]

checks for accidentally polluted globals. think there may be another one.

[trevnorris]

Commits need to be squashed and I would like to see in the commit message small amount of information about the segfault that was happening.

This is semver-patch. It's only additive and fixes a bug.

[trevnorris]

FYI: new SlowBuffer('string'); would simply hit CHECK(args[0]->IsNumber()); and abort. So if anything the original change to SlowBuffer was a change in API. This simply fixed an oversight on my part.

[trevnorris]

@targos Regarding being able to pass non-numbers, that would be a good change. So instead I would change the check and call to:

if (+length != length)
  length = 0;
return binding.create(+length);

[targos]

@trevnorris I like it, will making the change.

[trevnorris]

For posterity, I'm going to explain why the conditional check above is important.

• The to-int (e.g. val | 0) and to-uint (e.g. val >>> 0) force numeric wrapping.
• parseInt() will return NaN if the value cannot be resolved. Requiring an additional check.
• parseFloat() fails on numeric strings that aren't base 10.
• The check successfully catches NaN.

@targos That check does remove the < 0 which will cause SlowBuffer() to throw. Which is done in v0.12. I don't have a strong feeling either way whether it's kept around.

[targos]

@trevnorris I think it makes sense to throw when < 0.
PR updated according to your comments, commit message edited. PTAL.

[targos]

btw I don't really like the first line of the commit message, I find it too long. I'd be happy to get an advice from a native English speaker to make it better.

[trevnorris]

@targos possibly "buffer: SlowBuffer only accept valid numeric values"

[kzc]

Maybe I missed it, but is there a test for SlowBuffer(0)? It's a boundary case worth testing.

[trevnorris]

To make sure it doesn't throw? Sounds like an appropriate test.

On that note might be useful to test require('buffer').kMaxLength (upper bound) to make sure it succeeds, and kMaxLength + 1 to make sure it throws. This is useful because that value depends on architecture.

[targos]

All right, I will add those tests tomorrow.

[trevnorris]

Thanks much. Ping me when you do and I'll fire off CI.

[targos]

no worries

[aebabis]

Should we use isNaN? It seems to be faster:

function withCast(length) {
    if(+length != length) {
        length = 0;
    }
    return length * 2;
}

function withIsNaN(length) {
    if(isNaN(length)) {
        length = 0;
    }
    return length * 2;
}

function test() {
    console.time('withCast');
    for(var i = 0; i < 10000000; i++) withCast('apple');
    console.timeEnd('withCast');

    console.time('withIsNaN');
    for(var i = 0; i < 10000000; i++) withIsNaN('apple');
    console.timeEnd('withIsNaN');

    console.time('withCast');
    for(var i = 0; i < 10000000; i++) withCast('5');
    console.timeEnd('withCast');

    console.time('withIsNaN');
    for(var i = 0; i < 10000000; i++) withIsNaN('5');
    console.timeEnd('withIsNaN');

    console.time('withCast');
    for(var i = 0; i < 10000000; i++) withCast(5);
    console.timeEnd('withCast');

    console.time('withIsNaN');
    for(var i = 0; i < 10000000; i++) withIsNaN(5);
    console.timeEnd('withIsNaN');
}

test();

withCast: 593ms
withIsNaN: 296ms
withCast: 796ms
withIsNaN: 515ms
withCast: 62ms
withIsNaN: 109ms

[ChALkeR]

@acbabis But isNaN isn't a check for all objects that are not correct numbers…. It's a check if that object is a number that is equal to NaN.

[aebabis]

@ChALkeR Number.isNaN() checks to see if the value has type 'number' and is equal to NaN. isNaN() does type coersion to see if it is "not a number."

But you raise a valid point. Contrary to what I thought, my suggestion may be less readable.

[ChALkeR]

@acbabis Ok, I was wrong here. It checks that the object is equal to NaN after it's cast to a number — that's more correct.

But the point here is that it can't be used to check if the input is numeric or not.

[ChALkeR]

Owow, pressed the wrong button somehow.
Sorry, @Fishrock123.

[aebabis]

@ChALkeR I'm afraid I don't understand. For what input will isNaN(value) not evaluate to the same thing as +value != value?

[aebabis]

Found it!

function same(value) {
    console.log(isNaN(value) === (+value != value), value);
}

same(0);
same(1);
same(true);
same(false);
same('0');
same('1');
same('apple');
same(new Date());
same(+new Date());
same(NaN);
same(Number(5));
same({});
same([]);

true 0
true 1
true true
true false
true '0'
true '1'
true 'apple'
false Fri Sep 04 2015 13:19:03 GMT-0700 (US Mountain Standard Time)
true 1441397943493
true NaN
true 5
true {}
true []

What does the spec say about using a Date as a buffer length? :)

[ChALkeR]

@acbabis Actually, I didn't mean Date, I just misread the initial comparison :-p.