indygreg / indygreg/python-zstandard

OOM-path hardening: ~45 sites crash on allocation failure (most critically: `ZstdError` NULL at module init cascades into ~50 error paths)

Open
#298 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
642
Forks
116
Avg merge
1d 14h
Merged PRs (30d)
5

Description

## Summary

~45 sites across module init, type creation, and the decompression state machine either don't check the return value of an allocating API, or check it in a way that returns `NULL` without setting an exception. On OOM (or memory pressure in constrained environments), the resulting behavior ranges from `SystemError: returned NULL without setting an exception` (fatal on debug builds) to dereferencing NULL pointers and crashing.

The most catastrophic of these is **Category 1**: `ZstdError` going NULL at module init. Every subsequent `PyErr_Format(ZstdError, ...)` across ~50 error paths would then pass NULL as the exception class. The failure is silent at import time (see Category 2).

## Impact

- **Severity:** Crashes / `SystemError` on OOM. Latent on programs that don't hit OOM during normal operation.
- **Reachability:** OOM conditions, memory-constrained containers, processes with ulimit, small-VM CI, memory-pressure chaos-testing harnesses.
- **Version:** 0.25.0 (commit `7a77a75`).
- **Platform:** Platform-independent.

## Category 1: `ZstdError` NULL at module init — catastrophic

If `PyErr_NewException("zstandard.ZstdError", ...)` fails during module init (OOM), `ZstdError` stays `NULL`. Every subsequent `PyErr_Format(ZstdError, ...)` across ~50 error sites then passes NULL as the exception class → crash. Coupled with Category 2 (failure doesn't propagate out of module init), import appears to succeed and the crash deferred until the first error surfaces.

**Site:** `c-ext/constants.c:28`.

**Fix:**

```c
ZstdError = PyErr_NewException("zstandard.ZstdError", NULL, NULL);
if (!ZstdError) {
return -1; /* propagate out of constants_module_init */
}
```

Plus every caller of `constants_module_init` up the chain must check and propagate.

## Category 2: `zstd_module_init` continues after sub-init failures

16 type-init sub-calls in `zstd_module_init`; if any fails the module still loads. The `features` set also leaks on early returns from module init.

**Fix pattern:**

```c
if (compressor_module_init(m) < 0) return NULL;
if (decompressor_module_init(m) < 0) return NULL;
/* ... 14 more ... */
```

## Category 3: 19 `PyModule_AddObject` without ref-guard

`PyModule_AddObject` steals the reference on success and *leaks* it on failure. `pythoncapi_compat.h` (already included in the codebase) provides `PyModule_AddObjectRef` which doesn't steal.

**Fix:**

```c
/* was: */ PyModule_AddObject(m, "X", X);
/* use: */ if (PyModule_AddObjectRef(m, "X", X) < 0) { Py_DECREF(X); goto err; }
Py_DECREF(X); /* AddObjectRef doesn't steal */
```

For type-registration sites specifically, `PyModule_AddType(m, &T)` (3.10+) is simpler.

## Category 4: 3 unchecked `PyType_FromSpec` in `bufferutil_module_init`

On OOM, `PyType_FromSpec` returns NULL. On Python < 3.9 the code immediately dereferences NULL (`->tp_as_buffer`); on 3.9+ NULL is passed to `PyType_Ready` which also crashes.

**Sites:** `c-ext/bufferutil.c:531, 544, 557`.

**Fix:**

```c
BufferWithSegments_Type = (PyTypeObject *)PyType_FromSpec(&BufferWithSegments_Spec);
if (!BufferWithSegments_Type) return -1;
```

## Category 5: Unchecked `PyTuple_New` in `constants_module_init`

`PyTuple_New(0)` returns NULL on OOM; `PyTuple_SET_ITEM` then writes to NULL.

**Site:** `c-ext/constants.c`.

**Fix:** NULL-check the return before `PyTuple_SET_ITEM`.

## Category 6: 4 `PyMem_Malloc` / `PyMem_Realloc` without `PyErr_NoMemory`

When `PyMem_Malloc` / `PyMem_Realloc` returns NULL, the code takes `goto finally` which returns NULL without setting an exception → `SystemError`.

**Sites:** `c-ext/decompressor.c:704, 739` (`PyMem_Malloc`), `:800, 839` (`PyMem_Realloc`) — all inside `decompress_content_dict_chain`.

**Fix:**

```c
buf = PyMem_Malloc(size);
if (!buf) { PyErr_NoMemory(); goto finally; }

new_buf = PyMem_Realloc(buf, new_size);
if (!new_buf) { PyErr_NoMemory(); goto finally; }
buf = new_buf;
```

## Category 7: `Py_DECREF(NULL)` via `PyLong_FromSsize_t` OOM in `copy_stream`

`PyLong_FromSsize_t(totalRead)` can return NULL on OOM. Current code stores NULL into a tuple via `PyTuple_SET_ITEM(result, 0, totalRead)`; the subsequent `Py_DECREF(result)` traverses the tuple and dereferences NULL. Same pattern in the decompressor `copy_stream`.

**Sites:** `c-ext/compressor.c:411-418`; `c-ext/decompressor.c:247-251`.

**Fix:**

```c
PyObject *totalReadObj = PyLong_FromSsize_t(totalRead);
PyObject *totalWriteObj = PyLong_FromSsize_t(totalWrite);
if (!totalReadObj || !totalWriteObj) {
Py_XDECREF(totalReadObj);
Py_XDECREF(totalWriteObj);
goto except;
}
result = PyTuple_New(2);
if (!result) {
Py_DECREF(totalReadObj);
Py_DECREF(totalWriteObj);
goto except;
}
PyTuple_SET_ITEM(result, 0, totalReadObj);
PyTuple_SET_ITEM(result, 1, totalWriteObj);
```

## Category 8: Unchecked `PyBytes_FromStringAndSize` for `unused_data`

Silent data loss on OOM.

**Site:** `c-ext/decompressobj.c:97`.

**Fix:** NULL-check the return and propagate `PyErr_NoMemory` / return NULL with exception set.

## Suggested PR shape

Most impactful is Category 1 (it cascades into ~50 other error paths). Categories 2-8 are independent and small. Happy to do either one big hardening PR or one PR per category — probably the big PR reads better because OOM hardening is best evaluated holistically.

## Methodology

Found via [cext-review-toolkit](https://github.com/devdanzin/cext-review-toolkit) (Tree-sitter-based static analysis with structured naive/informed review passes). All 8 categories reviewed statically; faithfully reproducing OOM requires `_testcapi.set_nomemory` or libfiu-style allocator injection, both of which are feasible but out of scope for the live-reproducer pass that covered this analysis. Happy to open a PR — the fixes are mechanical and collectively ~100 lines of diff.

*Discovery, root-cause analysis, and issue drafting were performed by [Claude Code](https://www.anthropic.com/claude-code) and reviewed by a human before filing.*

## Full report

Complete multi-agent analysis (48 FIX findings across 13 categories, plus a reproducer appendix): https://gist.github.com/devdanzin/b86039ac097141579590c1a0f3a43605

Contributor guide

Open the contributing guide

Research direction

Start with c-ext/constants.c and the zstd_module_init chain, then inspect the explicitly listed sites in bufferutil.c, decompressor.c, compressor.c, and decompressobj.c. Trace each allocation failure to its caller and run the repository's existing test suite; done means the listed OOM paths propagate failure without NULL dereferences, leaked references, or unset exceptions.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, python
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.