buffer: convert range to numbers before validation

[cjihrig]

Checklist

• make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

buffer

Description of change

Prior to this commit, a carefully crafted object with a Symbol.toPrimitive could bypass range validation. This commit converts the range values to numbers before performing any validation.

Note, this commit converts start and end to a number before later converting to unsigned ints. This is a bit of duplicate work, but maintains backwards compatibility.

Fixes: #9149

[fhinkel]

LGTM.

Looking at this made me wonder though about start = -0.01. If rounded to an integer, it's 0 and within the allowed range. But since we only round to integers after the range check, an exception is thrown.

I'm leaning to rounding before the range check, because the docs say, offset <Integer>. Any thoughts on that?

[cjihrig]

@fhinkel, I originally thought to completely process the input before doing any checks. However, that would have been a breaking change. This seems to be more backwards compatible. I wouldn't be opposed to doing a follow up breaking change for v7 or v8.

[fhinkel]

Sounds good!

[deian]

While you're at it, may I suggest adding a fix to the binding code. The issue is here:

  size_t start = args[2]->Uint32Value();
  size_t end = args[3]->Uint32Value();

I think it's worth checking the types of args[2] and args[3]. Otherwise a hard crash is still possible via:

const buff = Buffer.alloc(1);

const start = {
    [Symbol.toPrimitive](hint) {
      throw "hard crash";
    }
};

process.binding('buffer').fill(buff, start, 1);

[trevnorris]

@deian allowing it to crash in a way that a try/catch will work, vs a segfault are different issues. we'll reserve fixing that in a different PR. since other buffer APIs also have the same problem.

[deian]

@trevnorris that snippet will segfault (or maybe that's what you meant?)