microsoft / ClearScript

A library for adding scripting to .NET applications. Supports V8 (Windows, Linux, macOS) and JScript/VBScript (Windows).
https://microsoft.github.io/ClearScript/
MIT License
1.77k stars 148 forks source link

Problem with ArrayBuffer bounds checking #581

Open raystubbs opened 4 months ago

raystubbs commented 4 months ago

The following snippet breaks when contents is an empty array. Seems like bounds checking on sourceIndex and offset aren't correct for 0 length ArrayBuffer.

        public IArrayBuffer CreateJsBuffer(byte[] contents) {
            var buf = (IArrayBuffer)((ScriptObject)engine.Evaluate("ArrayBuffer")).Invoke(true, [contents.Length]);
            buf.WriteBytes(contents, 0, (ulong)contents.Length, 0);
            return buf;
        }
Specified argument was out of the range of valid values. (Parameter 'offset') Error: Specified argument was out of the range of valid values. (Parameter 'offset')
ClearScriptLib commented 4 months ago

Hi @raystubbs,

What behavior would you expect in this scenario? An index of zero is indeed out of range for a zero-length array.

Thanks!

raystubbs commented 4 months ago

I'd expect it not to throw when copying 0 bytes with offset or sourceIndex as 0.

ClearScriptLib commented 4 months ago

I'd expect it not to throw when copying 0 bytes with offset or sourceIndex as 0.

So, if count is zero, the method should just return zero without checking the other arguments? While that would certainly be a simple change, we're not sure it would be more correct from an API perspective (unless there's precedent).

In any case, if that's the behavior you want, would it not be possible to check the count before invoking WriteBytes?

raystubbs commented 4 months ago

Alright, I can't think of a particular example/precedent off the top of my head. But the fact that this behavior was surprising should probably be a hint in itself.

And we did change our code to do an explicit check. Just seems like it shouldn't be necessary.

Anyway thanks for the quick responses. If it isn't obvious to you that this isn't good, then probably no reason to change it. But may be worth noting the behavior in the docs.