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

Fix pool pollution, infinite loop #510

Open
wants to merge 2 commits into
base: v3
Choose a base branch
from

Conversation

myndzi
Copy link
Contributor

@myndzi myndzi commented Nov 26, 2024

When nanoid is called with a fractional value, there were a number of undesirable effects:

  • in browser and non-secure, the code infinite loops on while (size--)
  • in node, the value of poolOffset becomes fractional, causing calls to nanoid to return zeroes until the pool is next filled: when i is initialized to poolOffset, pool[i] & 63 -> undefined & 63 -> 0
  • if the first call in node is a fractional argument, the initial buffer allocation fails with an error

I chose |0 to cast to a signed integer primarily because that has a slightly better outcome in the third case above: if the first call is negative (e.g. nanoid(-1)) then Node will throw an error for an invalid Buffer size, rather than attempting to allocate a buffer of size 2**32-1. It's also more compact than >>>0, which would be necessary to cast to an unsigned integer. I don't think there is a use case for generating ids longer than 2**31-1 :)

The browser code is structured in such a way that casting size in customRandom succinctly isn't readily feasible. I chose to cast it at the line let j = step | 0 since casting defaultSize would not fix the infinite loop in all cases, and the other use of defaultSize is to define the step length which is already shown to be fractional and gets cast to an integer with ~ anyway.

As for the nanoid function, new Uint8Array(size) ignores the fractional part, and size doesn't get used further - the function instead calls reduce over the typed array.

In the Node/native async customAlphabet variant, I chose to convert the id.length === size check to id.length >= size, which handles the fractional case and avoids the infinite loop; size is not used for anything else there.

When nanoid is called with a fractional value, there were a number
of undesirable effects:
- in browser and non-secure, the code infinite loops on `while (size--)`
- in node, the value of poolOffset becomes fractional, causing calls to
  nanoid to return zeroes until the pool is next filled: when `i` is
  initialized to `poolOffset`, `pool[i] & 63` -> `undefined & 63` -> `0`
- if the first call in node is a fractional argument, the initial buffer
  allocation fails with an error

I chose `|0` to cast to a signed integer primarily because that has a
slightly better outcome in the third case above: if the first call is
negative (e.g. `nanoid(-1)`) then Node will throw an error for an
invalid Buffer size, rather than attempting to allocate a buffer of
size `2**32-1`. It's also more compact than `>>>0`, which would be
necessary to cast to an unsigned integer. I don't _think_ there is
a use case for generating ids longer than `2**31-1` :)

The browser code is structured in such a way that casting `size` in
`customRandom` succinctly isn't readily feasible. I chose to cast it
at the line `let j = step | 0` since casting defaultSize would not
fix the infinite loop in all cases, and the other use of defaultSize
is to define the step length which is already shown to be fractional
and gets cast to an integer with `~` anyway.

As for the `nanoid` function, `new Uint8Array(size)` ignores the
fractional part, and `size` doesn't get used further - the function
instead calls reduce over the typed array.

In the Node/native async customAlphabet variant, I chose to convert
the `id.length === size` check to `id.length >= size`, which handles
the fractional case and avoids the infinite loop; `size` is not used
for anything else there.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant