analogous(-1): how a default hid a heap-exhaustion bug for fifteen years

This is a submission for DEV's Summer Bug Smash: Clear the Lineup powered by Sentry.

Project Overview

TinyColor is a colour library for JavaScript. Parse a colour in basically any format, convert it, lighten it, generate a palette from it. It's fifteen years old, has 5.2k stars, and ships on npm as tinycolor2 underneath a lot of design tooling.

I found this while porting it to Rust. I'd built a differential fuzzer for the port: run the original on V8 and my version side by side, same inputs, compare the outputs bit for bit. Thirty-one million comparisons, zero disagreements in the colour maths.

It found nothing here though. Not because the code is fine, but because for these inputs the original doesn't return a wrong answer. It doesn't return at all, and you can't diff against a process that's died.

Bug Fix or Performance Improvement

analogous(results) and monochromatic(results) loop forever on a negative or fractional count, and take the heap with them.

Both decrement a counter and test it for truthiness:

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

// monochromatic
while (results--) { ... }

A counter that never lands exactly on 0 never stops. From 1.5 the sequence goes 0.5, -0.5, -1.5, -2.5…. From -1 it goes -2, -3, -4…. Every pass pushes another colour object onto an array nobody will ever read.

$ node --max-old-space-size=256 -e "require('tinycolor2')('red').analogous(-1)"

<--- Last few GCs --->
FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory

$ echo $?
134

Six cases, all exit 134: both functions at -1, 1.5 and 0.5.

Here's the part that makes it worth guarding rather than shrugging at. polyad() is the third combination function, sitting in the same file, taking the same shape of argument. It already validates:

if (isNaN(number) || number <= 0) {
  throw new Error("Argument to polyad must be a positive number");
}

So the hazard is recognised in one of the three and not the other two. Any caller passing a user-supplied palette size into analogous() or monochromatic() has an unauthenticated way to kill the process. A number field in a colour picker. A value out of a config file. No exotic input needed, just a minus sign.

Code

Issue: bgrins/TinyColor#280
Pull request: (link)

 function analogous(color, results, slices) {
   results = results || 6;
   slices = slices || 30;

+  if (isNaN(results) || results <= 0) {
+    throw new Error("Argument to analogous must be a positive number");
+  }
+
   var hsl = tinycolor(color).toHsl();
   var part = 360 / slices;
   var ret = [tinycolor(color)];

   for (hsl.h = (hsl.h - ((part * results) >> 1) + 720) % 360; --results; ) {
     hsl.h = (hsl.h + part) % 360;
     ret.push(tinycolor(hsl));
   }
   return ret;
 }

 function monochromatic(color, results) {
   results = results || 6;
+
+  if (isNaN(results) || results <= 0) {
+    throw new Error("Argument to monochromatic must be a positive number");
+  }
+
   var hsv = tinycolor(color).toHsv();

Tests mirroring the ones polyad already has:

assertThrows(() => { tinycolor("red").analogous(-1); });
assertThrows(() => { tinycolor("red").analogous(1.5); });
assertThrows(() => { tinycolor("red").monochromatic(-1); });
assertThrows(() => { tinycolor("red").monochromatic(0.5); });

My Improvements

Why nobody hit this in fifteen years

This is the bit I found genuinely interesting, and it's why I nearly dismissed my own bug report.

My first three probes came back clean. analogous(0) returns six colours. analogous(null) returns six. analogous(NaN) returns six. I assumed I'd fat-fingered the argument and moved on, and only came back to it an hour later because the shape of "some counts are fine" bothered me.

The answer is on the first line of the function:

results = results || 6;

That's a default, not a guard. But it happens to catch almost everything a developer would think to try:

you pass	truthy?	loop actually gets	outcome
0	no	6	fine
null	no	6	fine
undefined	no	6	fine
NaN	no	6	fine
false	no	6	fine
-1	yes	-1	heap death
1.5	yes	1.5	heap death
0.5	yes	0.5	heap death

Every falsy value gets swapped for 6 before the loop ever sees it. The default is accidentally guarding the entire class of input you'd reach for first.

So the function is safe for every value you'd casually try and unsafe for the ones you wouldn't. That's a nasty shape for a bug: it hides from exactly the person looking for it.

Checking the blast radius

analogous takes a second argument, slices, guarded the same lazy way with slices || 30. I assumed it had the same hole and it doesn't. slices only feeds part = 360 / slices, so a bad value gives you a wrong hue or a NaN channel, but the loop is driven entirely by results. slices at -1, 1.5 and NaN all return six colours and terminate.

Worth checking rather than assuming, and it let me narrow the report instead of overclaiming.

Guard, or coerce?

Two ways to fix it:

1. Guard and throw, the way polyad() does.
2. Coerce with results = Math.max(1, Math.floor(results)).

Coercion is friendlier to anyone currently passing 1.5 and getting something back. But it silently changes their result and buries the mistake, and I don't think a colour library should be quietly reinterpreting your arguments. The guard also matches what the same file already does for the same input, so a maintainer can merge it without deciding a new policy. Consistency inside a codebase beats my personal taste in error handling.

Verifying before filing

I checked against current main, not just the copy I'd vendored: byte-identical, both loops still there, all six cases still exit 134. Then I read a hundred existing issues looking for a duplicate. Closest are #116 and #204, neither of which is this.

The PR is open rather than merged, and I'll be straight about why: TinyColor hasn't merged anything since February 2023.

What my port does instead

It returns a finite list. analogous(-1) gives one entry, 1.5 gives two.

That's the single deliberate behavioural divergence in an otherwise bit-exact port, documented as D-022 in the decision log. Reproducing the original faithfully would've meant shipping a denial of service on purpose. And there's nothing to reproduce anyway: the original doesn't return something wrong here, it stops existing.

---

The port, the decision log, and the fuzzer that sent me looking: github.com/BigAchiever/tinycolor-rs