Uint8Array check unreliable

[webmaster128]

Heyho! I came across an issue about the Uint8Array checks that are used in this library. TL;DR: instanceof Uint8Array is unrelyable in many cases.

There is a longer writeup about the problem in https://medium.com/@simonwarta/limitations-of-the-instanceof-operator-f4bcdbe7a400. A simple way to reproduce the problem is:

$ node -e 'require("repl").start().context.a = new Uint8Array([1, 2, 2, 1]);'
> a
Uint8Array(4) [ 1, 2, 2, 1 ]
> a instanceof Uint8Array
false

A more reliable way to check for Uint8Array can be found here: https://github.com/cosmos/cosmjs/blob/79396bfaa49831127ccbbbfdbb1185df14230c63/packages/utils/src/typechecks.ts#L14-L32. Wether Buffer is an Uint8Array is a bit controvesial as you can see in the code there. I think for our use case and for the sake of simplicity, Buffer should be considered an Uint8Array, right?

I'm happy to provide a PR to get this fixed.

[paulmillr]

I've read your article, but it is still unclear to me.

What exact use cases are, when the check is incorrect?

Is it web workers, and chrome background pages? Are they the only ones? (the repl example is completely irrelevant).

[webmaster128]

This happens every time data is passed over a thread/process (not sure) boundary. Then the data is cloned using the structured clone algorithm but you have different instantiations of Uint8Array and probably the whole standard library on each side. Background pages of extensions is the first place where I stumbled upon the problem and debugged it. The REPL server is the second one I found recently. Most likely WebWorkers and the background/Ui boundary in Electron show the same behavior too.

I only made the repl example because it is reproducible and relatively simple. It shows data that is created in one thread/process and then cloned to the REPLServer where the check then fails. In CosmJS we have a TypeScript REPL where we run into this problem when trying to migrate to @noble/hashes.

[paulmillr]

I have tried it but then reverted it. It reduces the performance by 20%, which is not really acceptable.

I have googled the issue and it seems like transferring TypedArrays is incorrect way of using web workers. Instead, array buffers should be transferred.

[paulmillr]

In fact, the https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm page specifically mentions supported types and there is no mention of typed arrays anywhere.

[paulmillr]

https://developer.mozilla.org/en-US/docs/Glossary/Transferable_objects

Note: Typed arrays like Int32Array and Uint8Array, are serializable, but not transferable. However their underlying buffer is an ArrayBuffer, which is a transferable object. We could have sent uInt8Array.buffer in the data parameter, but not uInt8Array in the transfer array.

[webmaster128]

I have tried it but then reverted it. It reduces the performance by 20%, which is not really acceptable.

Yeah, I see. Makes sense.

The structured clone algorithm supports typed arrays. This is mentioned in the row: "ArrayBufferView | Including other typed arrays." of the page you linked. I just tested this and it works in the website-webworker connection in Firefox and Chrome. Transferring (i.e. moving) is a different story.

It seems like it is really edge cases where you run into the problem, like the REPL case or the browser extension issue I had. I'll just clone the data in my code then.

I wonder if the type checks are really needed here. We have the type safety through TypeScript already. Do you create them for JS users?

[paulmillr]

Sure, in fact we had an audit where some function let boolean values through, so the auditors told it was "high urgency" issue

[Muhammad-Altabba]

Hello guy, I faced an error with noble-hashes because of the library not able to detect the Uint8Array type. And I found that, this check seems to work well in detecting if the variable is of Uint8Array:

variable.constructor.name === 'Uint8Array'

So, I would suggest add it to the instanceof like this:

variable instanceof Uint8Array || variable.constructor.name === 'Uint8Array'

Do you like me to open a PR?

---

It also worth noting that I could workaround this by calling Uint8Array.from(variable) before passing it to your methods. This casued your methods to detect the type correctly when checking instanceof Uint8Array.

[paulmillr]

I faced an error with noble-hashes because of the library not able to detect the Uint8Array type.

Can you describe your environment? Because it works good in default case.

[Muhammad-Altabba]

Hi @paulmillr , Sorry for the late reply. To have the error, I run the unit tests of https://github.com/web3/web3.js but after I configured jest to use jsdom for the tests environment. However, when you use jsdom at jest, there will be lots of errors, and only some of them is because of this Uint8Array detection inside noble-hashes. (If I could remember correctly, you may need to use some polyfills before you can see the error value "..." at "/0" must pass "bytes" validation). Thanks,

[paulmillr]

so it's jsdom problem. Not noble-hashes.

[Muhammad-Altabba]

Actually, it is a problem with detecting Uint8Array that happens on some edge cases, which could be simulated when using jsdom with noble-hashes.

A user was facing this issue with Hyperledger Besu blockchain running on a VM: https://github.com/web3/web3.js/issues/6393. So, it a problem on some edge cases. And jsdom is one of those edge cases that we can just use to simulate it.

The point, as this issue creator (@webmaster128) said, is that using instanceof Uint8Array is unreliable. And noble hashes is suggested to use a reliable way (probably as: variable instanceof Uint8Array || variable.constructor.name === 'Uint8Array').

Thanks,

[paulmillr]

The solution you're mentioning reduces performance by 1/5, which is pretty bad.

The issue may happen in jsdom, besu and other shitty / unpopular js engines which do not implement specs properly.

This needs to be reported to Besu. Also we can make uint8array check overridable so that you can adjust it in your code.

[Muhammad-Altabba]

Could you please share how you calculated that the solution that I suggested, which is using variable instanceof Uint8Array || variable.constructor.name === 'Uint8Array', would reduce the performance by 1/5?

Thanks,

[paulmillr]

I've confused your suggestion with suggestion of topicstarter. Yeah, that seems to work without performance penalty.

[Muhammad-Altabba]

Ok, great. Do you like me to open a PR?

[paulmillr]

yes

[paulmillr]

@Muhammad-Altabba heads up -- this is not as simple as that. See d5c624a

[Muhammad-Altabba]

Hi @paulmillr , I drafted the code early at: https://github.com/paulmillr/noble-hashes/commit/a716bab2f8af5cefb38f8005bb6364e1763017fc. And I was planning to build, test and open a PR tonight :smile:... And here it is: https://github.com/paulmillr/noble-hashes/pull/78

I see you already merged and my branch is out of date now. I guess you can close my PR except if you still find something worth merging.

Thanks,