Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem with ArrayBuffer bounds checking #581

Open
raystubbs opened this issue Jun 19, 2024 · 4 comments
Open

Problem with ArrayBuffer bounds checking #581

raystubbs opened this issue Jun 19, 2024 · 4 comments
Assignees
Labels

Comments

@raystubbs
Copy link

raystubbs commented Jun 19, 2024

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
Copy link
Collaborator

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
Copy link
Author

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

@ClearScriptLib
Copy link
Collaborator

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
Copy link
Author

raystubbs commented Jun 28, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 participants