Automattic / Automattic/node-canvas
Three rare error paths in image decoders leak native resources
- Dominant language
- JavaScript
- Stars
- 10.7k
- Forks
- 1.2k
- Avg merge
- 4d 8h
- Merged PRs (30d)
- 1
Description
## Bug
Three rare error paths in image decoding and pattern construction allocate native resources and return without releasing them. None of them are reachable in normal operation — they require a malformed image, cairo rejecting mime data, or cairo failing to create a context — but each one is a real ownership bug and each is a one- or two-line fix.
### 1. `Image::loadGIFFromBuffer()` — `data` leaks when no color map exists
```cpp
uint8_t *data = new uint8_t[naturalWidth * naturalHeight * 4];
// ...
ColorMapObject *colormap = img->ColorMap ? img->ColorMap : gif->SColorMap;
if (colormap == nullptr) {
GIF_CLOSE_FILE(gif);
return CAIRO_STATUS_READ_ERROR; // <- data leaks
}
```
GIFs without a global color map and without a local color map on the first image descriptor are pathological but parseable up to this point. `data` (the full `width * height * 4` decode buffer) is dropped.
### 2. `Image::assignDataAsMime()` — `mime_data` + `mime_closure` leak when cairo rejects the mime
```cpp
Napi::MemoryManagement::AdjustExternalMemory(env, len);
return cairo_surface_set_mime_data(_surface
, mime_type
, mime_data
, len
, clearMimeData
, mime_closure);
```
`mime_data` is a freshly `malloc`ed copy of the JPEG buffer; `mime_closure` is the destruction state cairo would normally call on cleanup. If `cairo_surface_set_mime_data` returns non-success, cairo does not take ownership and will not call `clearMimeData`, so both allocations live forever. The external-memory accounting for `len` is also never reversed, so `Napi::MemoryManagement` keeps thinking the buffer is alive. Per-occurrence leak is the size of the JPEG buffer.
### 3. `create_transparent_pattern()` — `mask_context` + `mask_surface` leak on cairo OOM
```cpp
cairo_t *mask_context = cairo_create(mask_surface);
if (cairo_status(mask_context) != CAIRO_STATUS_SUCCESS) {
return NULL; // <- mask_context and mask_surface leak
}
```
Only reachable when cairo cannot create a context (typically OOM). Returns `NULL` without destroying either object.
## Reproduction
These don't show measurable RSS slope in normal operation — they all sit behind error/OOM gates that aren't easy to hit deliberately from JS without poking at internals. The bugs are visible by code inspection alone, and the fixes are tiny.
## Fix
1. `loadGIFFromBuffer`: `delete[] data` before returning the no-colormap error.
2. `assignDataAsMime`: capture the status of `cairo_surface_set_mime_data`; on non-success, reverse the `AdjustExternalMemory(env, len)` and `free` both `mime_data` and `mime_closure` before returning.
3. `create_transparent_pattern`: destroy `mask_context` and `mask_surface` before returning `NULL`.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.