Add randomFillSync

[yassernasc]

It's hard to write unit tests on random values without mocking, so my approach was using big values and making it easy to change the sample size.

[kasperisager]

Good start! This particular function has some sharp edges though:

1. It should work for plain ArrayBuffer and all array buffer views, including Buffer, TypedArray, and DataView.

2. It shouldn't perform any allocations but should instead write directly to the passed buffer. The way you've implemented randomFill() in terms of randomBytes() is great and if you simply do it the other way around you can make it allocation-less while still keeping the implementation succinct.

[yassernasc]

Sharp tips.

So, the key points of the incremented solution IMO are:

1. I'm using bare-type to detect if the buf argument is an ArrayBuffer, TypedArray (Buffer), or DataView and reading those with js_get_arraybuffer_info, js_get_typedarray_info, or js_get_dataview_info, respectivally.

2. OpenSSL's RAND_bytes function now writes directly to the passed buffer and the offset is applied using Pointer index: &buf[offset] (IDK if this technique is a safe approach).

[kasperisager]

Much better! 👌 It can be further simplified by doing the following:

1. Instead of using bare-type, we just need ArrayBuffer.isView() to tell us if we're dealing with a plain ArrayBuffer or a view, i.e. TypedArray or DataView. If the latter is the case, simply unwrap the inner ArrayBuffer by grabbing buffer.buffer, buffer.byteOffset, and buffer.byteLength.

2. Always pass a plain ArrayBuffer alongside a byteOffset and byteLength to the native layer. This way the binding stays monomorphic and you can therefore always call js_get_arraybuffer_info() on the native side.

[yassernasc]

yes indeed, sweet refactoring. 👍

[kasperisager]

Almost there! 🙌 The native side is Just Right(tm) now, the only bits missing are some devilish details on the JavaScript side:

1. offset and size are relative to the number of bytes per element of the passed view when not passing a plain ArrayBuffer. For example, for a 16 byte view, such as Uint16Array, offset and size are not expressed in 1 byte units but in 2 byte units. The simplest way to handle this is to use the subarray(start, end) method if either offset isn't 0 or size isn't buffer.length.

2. When an ArrayBuffer view is passed, instead of a plain ArrayBuffer, the view may not necessarily be over the entire underlying ArrayBuffer. The offset and size passed to the native side should therefore be buffer.byteLength and buffer.byteOffset || 0 to reflect this. As a plain ArrayBuffer doesn't have a byteOffset, it can be defaulted to 0.

I've added some test cases that exercise both of those points.

[yassernasc]

Tricky changes, I tried not to proliferate many conditionals and keep the flow sane.

My solution was to refine and reshape the values from offset and size according to all contexts. For the "bytes per elements” problem I used multiplication on the value passed by argument only, e.g. offset *= buf.BYTES_PER_ELEMENT.

And to apply the window view I relied on byteOffset and byteLength as default values. A little adjustment needed was to combine the offset argument with the byteOffset from the view: offset += buf.byteOffset (the offset value is already potentially multiplied by buf.BYTES_PER_ELEMENT at this point).

In that way, we still pass all the buffer to the native code regardless of how small a window view could be the case, I was wondering about the subarray(start, end) resolving that and not passing any offset to the native code, just the needed buffer and the amount to bytes to be written, but ArrayBuffer and DataView classes doesn't implement the subarray method and IDK if adding a new Buffer/TypedArray layer would make the optimization be lost.

[kasperisager]

Looks great, and good point about subarray(start, end) not being defined for DataView!

Now for the final touch to avoid surprises and nasty CVEs: Bounds checking. In general, whenever you slice into a piece of memory from C, you must ask yourself: Is this slice within bounds of the underlying memory? If you can either guarantee that or write it off as undefined behaviour, all good! If you can't guarantee that, however, there's risk of causing buffer under- and/or overflow. One such case is...

RAND_bytes(&buf[offset], num)

...where you slice into buf at offset for num bytes. There's currently no guarantee that offset and offset + num actually resides within buf though and so we risk RAND_bytes() writing random bytes outside of buf.

A similar, albeit less severe, situation arises when you have a partial view over an underlying ArrayBuffer. Here we have to make sure that offset and offset + num doesn't cause the partial view to expand.

I've added some initial test cases for both of these points. It'd probably be beneficial with similar tests for views with an element size larger than 1 byte such as Uint16Array and friends.

[yassernasc]

Checks were added, and I cleaned the code style a little bit.

[kasperisager]

Amazing, let's land it! 🤩