Skip to content

Guard analogous() and monochromatic() against counts that never terminate - #283

Open
BigAchiever wants to merge 1 commit into
bgrins:masterfrom
BigAchiever:guard-analogous-monochromatic-counts
Open

Guard analogous() and monochromatic() against counts that never terminate#283
BigAchiever wants to merge 1 commit into
bgrins:masterfrom
BigAchiever:guard-analogous-monochromatic-counts

Conversation

@BigAchiever

Copy link
Copy Markdown

Fixes #280.

analogous(results) and monochromatic(results) both decrement their loop counter and test it for truthiness:

for (hsl.h = (hsl.h - ((part * results) >> 1) + 720) % 360; --results; ) { ... }  // analogous
while (results--) { ... }                                                         // monochromatic

A counter that never lands exactly on 0 never stops. From -1 the sequence is -2, -3, -4…; from 1.5 it is 0.5, -0.5, -1.5…. Each pass pushes another colour, so the process exhausts its heap:

$ node --max-old-space-size=256 -e "require('tinycolor2')('red').analogous(-1)"
FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory
$ echo $?
134

Six cases reproduce: both functions at -1, 1.5 and 0.5.

Why it went unnoticed

results = results || 6 reads as a default but behaves as an accidental guard. 0, null, undefined, NaN and false are all falsy, so every value someone would casually reach for gets replaced by 6 before the loop sees it. -1, 1.5 and 0.5 are truthy and pass straight through — so the functions are safe for exactly the inputs you'd test and unsafe for the ones you wouldn't.

The change

polyad() already validates the same shape of argument, so this brings the other two combination functions in line with it rather than introducing a new policy.

The guard additionally rejects non-integers, which polyad doesn't need — its loop is for (i = 1; i < number; i++) and terminates fine on 1.5, whereas --results cannot.

Falsy inputs still fall through to the default of 6, unchanged. analogous(3) still returns 3.

I considered coercing instead (Math.max(1, Math.floor(results))), which would be gentler on any caller currently passing a fraction, but it silently changes their result. Happy to switch if you'd prefer that.

Testing

  • Added a case to test.js covering the six throwing inputs and confirming falsy values still default.
  • The full suite passes on the patched build: 45/45, polyad still ignored as before.

One thing to check

I don't have Deno installed, so I couldn't run deno task build. I applied the identical guard by hand to npm/cjs/tinycolor.js, npm/esm/tinycolor.js and tinycolor.js so the generated files match what the build would produce — the guard is plain ES5 and shouldn't transpile differently. Worth regenerating them properly before merge.


Found while differentially fuzzing a Rust port of TinyColor against the original. The fuzzer never caught this one: for these inputs the original doesn't return a wrong answer, it stops existing, and there's nothing to diff against a dead process.

…nate

Both functions decrement their loop counter and test it for truthiness:

    for (hsl.h = ...; --results; )   // analogous
    while (results--)                // monochromatic

A counter that never lands exactly on 0 never stops. From -1 the sequence is
-2, -3, -4...; from 1.5 it is 0.5, -0.5, -1.5... Each pass pushes another
colour, so the process exhausts its heap and aborts with exit 134.

    $ node --max-old-space-size=256 -e "require('tinycolor2')('red').analogous(-1)"
    FATAL ERROR: Reached heap limit Allocation failed
    $ echo $?
    134

Six cases reproduce: both functions at -1, 1.5 and 0.5.

The reason this survived is that `results = results || 6` looks like a default
but acts as an accidental guard. 0, null, undefined, NaN and false are falsy,
so every value someone would casually test gets replaced by 6. -1, 1.5 and 0.5
are truthy and pass straight through.

polyad() already validates the same shape of argument, so this brings the other
two combination functions in line with it. The guard additionally rejects
non-integers, which polyad does not need: its loop is `for (i = 1; i < number;
i++)` and terminates on fractional counts, whereas these two cannot.

Falsy inputs still fall through to the default of 6, unchanged.

Fixes bgrins#280
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.

analogous() and monochromatic() loop unboundedly on negative or fractional counts, exhausting memory

1 participant