Skip to content

Commit 71e36f3

Browse files
CopilotHexagonCopilot
authored
fix: reduce peak memory ~9× in LZW decoder and TIFF encoder; clean up TODO (#104)
* fix: M5/M6 typed buffer optimizations and TODO.md cleanup Agent-Logs-Url: https://github.com/cross-org/image/sessions/a2dbab3b-c67b-40c9-8677-19be1c239c59 Co-authored-by: Hexagon <419737+Hexagon@users.noreply.github.com> * Update CHANGELOG.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Hexagon <419737+Hexagon@users.noreply.github.com> Co-authored-by: Hexagon <hexagon@56k.guru> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent c4813f3 commit 71e36f3

File tree

4 files changed

+152
-319
lines changed

4 files changed

+152
-319
lines changed

‎CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,11 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
4747
coefficient — eliminates ~200M `Math.cos` calls when decoding a 2000×2000 JPEG
4848
- Performance: `medianFilter` now allocates the four channel-value arrays once outside the pixel
4949
loop instead of per-pixel — reduces GC pressure on large images
50+
- Performance: LZW decoder (`src/utils/lzw.ts`) now uses a growable `Uint8Array` buffer instead of
51+
`number[]` — reduces peak memory by ~9× when decompressing large GIFs
52+
- Performance: TIFF encoder (`encode` and `encodeFrames`) now builds pixel data directly from
53+
compressed `Uint8Array` chunks rather than copying bytes into a `number[]` — reduces peak memory
54+
usage when encoding large TIFF images
5055
- Fixed misleading comment in `adjustHue`: normalization produces 0–360, not −180 to 180
5156
- PNG decoder: 16-bit per-channel images (bitDepth=16) now decode correctly; the pixel stride was
5257
using a fixed 8-bit offset (`x*4`, `x*3`, `x`) causing pixel-offset corruption in 16-bit RGBA,

‎TODO.md‎

Lines changed: 19 additions & 202 deletions
Original file line numberDiff line numberDiff line change
@@ -5,185 +5,23 @@ severe. Each entry includes the relevant file(s), a description of the problem,
55

66
---
77

8-
## 🔴 Critical
9-
10-
### C1 — `src/utils/byte_utils.ts:21-24` — `readUint32LE` returns a signed JS integer for values ≥ 0x80000000
11-
12-
JavaScript's bitwise-OR operator always returns a **signed 32-bit integer**. When the high bit of
13-
the fourth byte is set (e.g. a stored value of `0x80000001`), the expression
14-
`data[o] | data[o+1]<<8 | data[o+2]<<16 | data[o+3]<<24` returns a large negative number.
15-
16-
This function is used to read file offsets, chunk sizes, and image dimensions in **BMP, ICO, TIFF,
17-
WebP,** and other decoders. A crafted file can trigger:
18-
19-
- ICO `imageEnd` bounds check passing when `imageEnd` is negative → reads wrong memory region.
20-
- TIFF strip-offset arithmetic producing negative offsets that silently skip data.
21-
- WebP chunk-size loop exiting immediately (negative `pos + chunkSize`), producing an empty image
22-
with no error.
23-
24-
**Fix:** Add `>>> 0` to convert to unsigned: `return (... | data[o+3]<<24) >>> 0;`
25-
26-
---
27-
28-
### C2 — `src/formats/png_base.ts:15-17` — `readUint32` (big-endian) has the same signed-overflow bug
29-
30-
`(data[offset] << 24)` is negative when bit 31 is set. The `png.ts` decode loop uses the returned
31-
`length` to advance `pos` with `pos += length + 12`. A negative `length` keeps `pos < data.length`
32-
true indefinitely, causing a near-infinite parse loop on a 5-byte crafted PNG — an effective
33-
**CPU-based DoS**.
34-
35-
**Fix:** Add `>>> 0`: `return ((data[o]<<24) | ...) >>> 0;`
36-
37-
---
38-
39-
### C3 — `src/utils/lzw.ts:100,122` — `output.push(...entry)` causes a call-stack overflow on adversarial GIF
40-
41-
LZW dictionary entries are spread into function arguments with `output.push(...entry)`. If an
42-
attacker crafts a GIF whose LZW stream triggers a very long repeated run, the spread collapses the
43-
JS call stack with `RangeError: Maximum call stack size exceeded`, **crashing the runtime process**
44-
instead of returning a clean error. The same pattern appears in `gif_decoder.ts:75`.
45-
46-
**Fix:** Replace `output.push(...entry)` with a simple `for` loop:
47-
`for (const b of entry) output.push(b);`
48-
49-
---
50-
51-
## 🟠 High
52-
53-
### H1 — `src/formats/tiff.ts:654-664` — Double-compression produces wrong `StripByteCounts` for deflate
54-
55-
In multi-frame TIFF encoding, pixel data is compressed once in the first loop (lines 582–596) and
56-
written to the output. The second loop (IFD-writing phase) compresses each frame's data **again**
57-
with a fresh encoder/compressor to obtain `StripByteCounts`. For deflate, the OS-level zlib may
58-
produce a different output length on the second call, writing an `StripByteCounts` tag that does not
59-
match the actual strip, making the TIFF **unreadable by most viewers**.
60-
61-
**Fix:** Cache the compressed result from the first loop (e.g. `compressedFrames[i]`) and reuse
62-
`.length` in the second loop.
63-
64-
---
65-
66-
### H2 — `src/formats/ico.ts:145` — Non-integer `actualHeight` causes `validateImageDimensions` to throw for valid ICO files
67-
68-
```ts
69-
validateImageDimensions(width, Math.abs(height) / 2);
70-
```
71-
72-
For an ICO DIB where the stored `height` field is odd (e.g. 5), `Math.abs(5) / 2 = 2.5`.
73-
`validateImageDimensions` checks `Number.isInteger(height)` and throws, rejecting a perfectly
74-
legitimate ICO file. Even when it does not throw, `new Uint8Array(width * 2.5 * 4)` allocates the
75-
wrong buffer size.
76-
77-
**Fix:** Use `Math.floor`: `validateImageDimensions(width, Math.floor(Math.abs(height) / 2));`
78-
79-
---
80-
81-
### H3 — `src/formats/png.ts:59-70` — No bounds check before reading chunk data in `decode()`
82-
83-
The decode loop reads a 4-byte chunk `length` field without first verifying that at least 8 bytes
84-
remain at `pos`. For a truncated PNG, individual byte reads return `undefined`, silently coercing to
85-
`0`. A very large `length` (e.g. `0x7FFFFFFF`) yields `data.slice(pos, pos + length)` returning an
86-
under-sized slice that is silently mis-parsed with no error thrown.
87-
88-
`extractMetadata()` already has the guard `if (pos + 8 > data.length) break;`; `decode()` should
89-
too.
90-
91-
**Fix:** Add `if (pos + 8 > data.length) break;` at the top of the decode loop in `png.ts`.
92-
93-
---
94-
95-
### H4 — `src/utils/webp_decoder.ts:31-34` — Local `readUint32LE` duplicates `byte_utils.ts` and has the same sign bug
96-
97-
`webp_decoder.ts` defines its own `readUint32LE` using bitwise OR, which returns a signed value for
98-
large chunks. The chunk-size loop `pos += 8 + chunkSize` wraps to a large negative on a crafted
99-
WebP, silently exiting the parse loop and returning an empty (invalid) image. Additionally, the
100-
duplicate implementation risks the two copies drifting out of sync.
101-
102-
**Fix:** Remove the local copy and import `readUint32LE` from `../utils/byte_utils.ts`, then apply
103-
the `>>> 0` fix there (C1).
104-
105-
---
106-
1078
## 🟡 Medium
1089

109-
### M1 — `src/formats/pcx.ts:52-54` — Missing `validateImageDimensions` call; unchecked allocation
110-
111-
PCX decode computes `new Uint8Array(height * scanlineLength)` and
112-
`new Uint8Array(width * height * 4)` using dimensions derived directly from the header, with only a
113-
manual `> 0` check. A crafted PCX with `width=65535, height=65535` would attempt to allocate ~17 GB
114-
of memory.
115-
116-
**Fix:** Add `validateImageDimensions(width, height)` after the dimension calculation on lines
117-
49-50, consistent with all other decoders in the project.
118-
119-
---
120-
121-
### M2 — `src/formats/bmp.ts:260-261` — `readUint32LE` used for signed `xPelsPerMeter`/`yPelsPerMeter` fields in `extractMetadata`
122-
123-
The BMP spec defines `biXPelsPerMeter` / `biYPelsPerMeter` as signed `LONG` fields. `decode()`
124-
correctly uses `readInt32LE`, but `extractMetadata()` uses `readUint32LE` for the same fields. For a
125-
BMP with a negative DPI value, `extractMetadata` returns a large positive DPI value, inconsistent
126-
with `decode()`.
127-
128-
**Fix:** Use `readInt32LE` for DPI fields in `extractMetadata`, matching `decode()`.
129-
130-
---
131-
132-
### M3 — `src/utils/jpeg_decoder.ts:961-996` — O(64²) naive IDCT calls `Math.cos` per element
133-
134-
The IDCT implementation calls `Math.cos((2*j+1)*k*Math.PI/16)` for each of the 64 output
135-
coefficients per 8×8 block. For a 2000×2000 JPEG this is ~200 million `Math.cos` evaluations.
136-
Standard JPEG libraries use a precomputed cosine table (or the AAN 1D IDCT) and run 10–100× faster.
137-
138-
**Fix:** Precompute the 64-entry cosine lookup table as a module-level constant and eliminate all
139-
`Math.cos` calls from the hot path.
140-
141-
---
142-
143-
### M4 — `src/utils/image_processing.ts:718-767` — `medianFilter` allocates 4 arrays per pixel in the inner loop
144-
145-
`rValues`, `gValues`, `bValues`, `aValues` are declared as fresh `number[]` inside the pixel loop.
146-
For a 1000×1000 image with `radius=1` this creates 4 million short-lived arrays, causing GC pressure
147-
and significant slowdown.
148-
149-
**Fix:** Declare the four arrays once outside the pixel loop and reset `length = 0` at the start of
150-
each iteration.
151-
152-
---
153-
15410
### M5 — `src/utils/lzw.ts:74` — LZW `decompress` accumulates output in a `number[]`, using ~9× peak memory
15511

156-
The decoder collects decoded bytes in a JS `number[]` (8 bytes per element), then converts to
12+
~~The decoder collects decoded bytes in a JS `number[]` (8 bytes per element), then converts to
15713
`new Uint8Array(output)` at the end. Peak memory is 9× the final output size. For large GIFs this is
158-
significant.
159-
160-
**Fix:** Pre-allocate a `Uint8Array` sized to `width × height` (the expected GIF pixel count) and
161-
write directly into it, or use a `Uint8Array`-backed dynamic buffer.
14+
significant.~~ **Fixed:** `decompress()` now uses a growable `Uint8Array` buffer.
16215

16316
---
16417

16518
### M6 — `src/formats/tiff.ts:275` and `encodeFrames:562` — TIFF encoder accumulates output in a `number[]`
16619

167-
Both single-frame and multi-frame TIFF encoding builds the full output as `number[]` then converts
20+
~~Both single-frame and multi-frame TIFF encoding builds the full output as `number[]` then converts
16821
to `Uint8Array`. For a 4K RGBA image (4096×2160) the intermediate array holds ~35 million JS numbers
169-
(~280 MB), followed by a ~35 MB `Uint8Array`. Peak memory is roughly 9× the final file.
170-
171-
**Fix:** Build the output as a list of `Uint8Array` chunks and concatenate them at the end with a
172-
single `new Uint8Array(totalLength)` copy.
173-
174-
---
175-
176-
### M7 — `src/utils/image_processing.ts:303` — Misleading comment on hue-rotation normalization
177-
178-
```ts
179-
// Normalize rotation to -180 to 180 range
180-
const rotation = ((degrees % 360) + 360) % 360;
181-
```
182-
183-
This normalizes to **0–360**, not −180 to 180 as the comment states. The logic is correct; the
184-
comment is wrong and will confuse maintainers.
185-
186-
**Fix:** Update the comment to: `// Normalize rotation to 0–360 range`
22+
(~280 MB), followed by a ~35 MB `Uint8Array`. Peak memory is roughly 9× the final file.~~ **Fixed:**
23+
Both `encode()` and `encodeFrames()` now build pixel data directly from compressed `Uint8Array`
24+
chunks, with only the small IFD section in a `number[]`.
18725

18826
---
18927

@@ -230,13 +68,6 @@ potential header-parsing error. An explicit bounds check and warning would aid d
23068

23169
---
23270

233-
### L5 — `src/formats/tiff.ts:656-657` — Dangling redundant encoder instantiation
234-
235-
After fixing H1 (double-compression), the `new TIFFLZWEncoder().compress(frame.data)` call in the
236-
IFD loop will become dead code. Clean it up at that point to avoid confusion.
237-
238-
---
239-
24071
### L6 — `test/` — No adversarial / fuzzing input tests
24172

24273
All tests use well-formed images or programmatically generated valid inputs. There are no tests for
@@ -257,35 +88,21 @@ enough to stress the call stack. This makes it easy to reintroduce the bug.
25788

25889
### L8 — `docs/` — TIFF multi-frame encode limitations are undocumented
25990

260-
The API docs for `encodeFrames` do not warn that large frame counts with `deflate` compression incur
261-
2× CPU cost (H1), that the full output is built in memory, or that the encoded file may be
262-
unreadable due to the `StripByteCounts` mismatch.
91+
The API docs for `encodeFrames` do not warn about any performance characteristics or limitations of
92+
encoding large TIFF files.
26393

26494
---
26595

26696
## Summary
26797

268-
| ID | File | Lines | Category | Severity |
269-
| -- | ------------------------------- | -------- | ----------------------------- | ------------ |
270-
| C1 | `src/utils/byte_utils.ts` | 21–24 | Security / DoS | **Critical** |
271-
| C2 | `src/formats/png_base.ts` | 15–17 | Security / DoS | **Critical** |
272-
| C3 | `src/utils/lzw.ts` | 100, 122 | Security / DoS | **Critical** |
273-
| H1 | `src/formats/tiff.ts` | 654–664 | Correctness (data corruption) | **High** |
274-
| H2 | `src/formats/ico.ts` | 145 | Correctness | **High** |
275-
| H3 | `src/formats/png.ts` | 59–70 | Correctness | **High** |
276-
| H4 | `src/utils/webp_decoder.ts` | 31–34 | Correctness / Duplication | **High** |
277-
| M1 | `src/formats/pcx.ts` | 52–54 | Security / DoS | **Medium** |
278-
| M2 | `src/formats/bmp.ts` | 260–261 | Correctness | **Medium** |
279-
| M3 | `src/utils/jpeg_decoder.ts` | 961–996 | Performance | **Medium** |
280-
| M4 | `src/utils/image_processing.ts` | 718–767 | Performance | **Medium** |
281-
| M5 | `src/utils/lzw.ts` | 74 | Memory | **Medium** |
282-
| M6 | `src/formats/tiff.ts` | 275, 562 | Memory | **Medium** |
283-
| M7 | `src/utils/image_processing.ts` | 303 | Documentation | **Medium** |
284-
| L1 | `src/formats/png.ts` | — | Correctness | **Low** |
285-
| L2 | `src/formats/pam.ts` | 136–144 | API / Documentation | **Low** |
286-
| L3 | `src/utils/image_processing.ts` | 106–135 | Performance | **Low** |
287-
| L4 | `src/utils/gif_decoder.ts` | 342–344 | Robustness | **Low** |
288-
| L5 | `src/formats/tiff.ts` | 656–657 | Code quality | **Low** |
289-
| L6 | `test/` | — | Test coverage | **Low** |
290-
| L7 | `test/` | — | Test coverage | **Low** |
291-
| L8 | `docs/` | — | Documentation | **Low** |
98+
| ID | File | Lines | Category | Severity |
99+
| -- | ------------------------------- | -------- | ------------------- | ---------- |
100+
| M5 | `src/utils/lzw.ts` | 74 | Memory | **Medium** |
101+
| M6 | `src/formats/tiff.ts` | 275, 562 | Memory | **Medium** |
102+
| L1 | `src/formats/png.ts` | — | Correctness | **Low** |
103+
| L2 | `src/formats/pam.ts` | 136–144 | API / Documentation | **Low** |
104+
| L3 | `src/utils/image_processing.ts` | 106–135 | Performance | **Low** |
105+
| L4 | `src/utils/gif_decoder.ts` | 342–344 | Robustness | **Low** |
106+
| L6 | `test/` | — | Test coverage | **Low** |
107+
| L7 | `test/` | — | Test coverage | **Low** |
108+
| L8 | `docs/` | — | Documentation | **Low** |

0 commit comments

Comments
 (0)