Skip to content

Fix memory leak of node pairs/items arrays on allocation failure in the loader - #357

Open
bg0d-droid wants to merge 1 commit into
yaml:masterfrom
bg0d-droid:fix-loader-alloc-error-leaks
Open

bg0d-droid wants to merge 1 commit into
yaml:masterfrom
bg0d-droid:fix-loader-alloc-error-leaks

Conversation

@bg0d-droid

Copy link
Copy Markdown

Summary

yaml_parser_load_mapping() and yaml_parser_load_sequence() leak their freshly allocated node pairs/items array when PUSH() fails to grow the document node stack.

Sequence of events:

  1. STACK_INIT() allocates the pairs/items array (16 entries, 128/64 bytes).
  2. MAPPING_NODE_INIT()/SEQUENCE_NODE_INIT() copies the array pointer into a stack-local yaml_node_t.
  3. PUSH(parser, parser->document->nodes, node) attaches the node to the document — but when the node stack is full, PUSH() first calls yaml_stack_extend(), whose realloc() can fail under memory pressure. The node is then never attached.
  4. The error: path frees tag and anchor but not the pairs/items array.

The document cleanup in yaml_parser_load() (yaml_document_delete()) only frees data attached to the document, so the array from step 1 is lost. In the fixed paths the array is still unattached at error:, so freeing it there is safe; yaml_free(NULL) covers the earlier failure points.

Impact

A leak on the out-of-memory path in the composing loader. Long-running processes that parse documents from untrusted input (and whose allocations can be exhausted) lose 64–128 bytes per failed composition at the exact moment memory is already tight.

Reproduction

An allocation-failure sweep over yaml_parser_load(): fail the Nth allocation for every N and run LeakSanitizer. With the test corpus below (mapping/sequence nodes placed on node-stack growth boundaries) the unpatched loader reports:

Direct leak of 128 byte(s) in 1 object(s) allocated from:
    #2 yaml_malloc src/api.c
    #3 yaml_parser_load_mapping src/loader.c
    ...

for the mappings document, and the equivalent 64-byte leak for the sequences document.

Fix

  • src/loader.c: free pairs.start / items.start on the error paths of yaml_parser_load_mapping() / yaml_parser_load_sequence().

Tests

  • New tests/test-load-error (+ test-alloc.c/.h, wired into tests/CMakeLists.txt): compiles the library sources with malloc/realloc/free/strdup renamed to counting, failure-injecting equivalents — portable, no LD_PRELOAD or linker tricks — then loads each corpus document once per allocation failure point and asserts that zero blocks remain allocated.
  • The corpus is built so mapping/sequence nodes land exactly on node-stack growth boundaries (nodes 17 and 33), which is where the leak occurs. Without the fix the test fails with 1 block(s) leaked when allocation 51 failed for both documents; with the fix it passes across 68 failure points per document.
  • Full suite passes: ctest 4/4 (version, reader, nesting, load-error).
  • Independent second check: the same sweep run under ASan/LSan with 771 failure points on a larger 9-document corpus is clean after the fix (it flagged the leak before).

When PUSH() grows the document node stack and the growth allocation
fails, yaml_parser_load_mapping() and yaml_parser_load_sequence() jump
to their error paths before the freshly allocated pairs/items array has
been attached to the document. The document cleanup only frees attached
data, so the array leaks.

Free the arrays on those error paths. They are still unattached there,
and yaml_free(NULL) covers the earlier failure points.

Found with an allocation-failure sweep over yaml_parser_load(): every
allocation is failed in turn (771 failure points under ASan/LSan).
After the fix the sweep is clean.

The new test-load-error reproduces this portably: it compiles the
library sources with the allocator renamed, injects one allocation
failure per run, and asserts that no blocks remain allocated. The
corpus places mapping/sequence nodes on node-stack growth boundaries
(nodes 17 and 33), where the leak occurs; without this fix it reports
"1 block(s) leaked when allocation 51 failed" for both documents.
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.

1 participant