Skip to content

gguf-py: bound array element count by remaining file size - #28131

Draft
wmshearer wants to merge 1 commit into
ggml-org:masterfrom
wmshearer:patch-4
Draft

gguf-py: bound array element count by remaining file size#28131
wmshearer wants to merge 1 commit into
ggml-org:masterfrom
wmshearer:patch-4

Conversation

@wmshearer

@wmshearer wmshearer commented Sep 1, 2026

Copy link
Copy Markdown

The array branch of _get_field_parts validates the declared element count against GGUF_MAX_ARRAY_ELEMENTS but never against the bytes actually remaining in the file. A count below that ceiling still drives a full per-element loop, so a small file that declares a large array costs time proportional to the declared count rather than to the file size.

_get_str already performs the equivalent check for strings:

if offset + 8 + int(slen[0]) > self.data.nbytes:
    raise ValueError(f'String length ... exceeds remaining file size ...')

This adds the matching bound for arrays. Every element occupies at least one byte, so a declared count greater than the remaining byte count is not satisfiable and can be rejected before the loop starts.

Found by fuzzing gguf-py with Atheris. A 101-byte input declaring 13,434,883 elements took 43.9s of CPU to parse on master (2d8d612); with this change it raises immediately. A normal file with a 5,000 element array parses unchanged.

Testing:

  • 101-byte fuzzer input: 43.9s before, immediate ValueError after
  • Valid 74KB file with a 5,000 element array and one tensor: parses correctly with the change applied

Overview

Additional information

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure:
    Yes: AI usage disclosure: AI-assisted. The bug was found by my own fuzzing campaign against gguf-py. I used AI assistance to identify the fix and draft this description, and reviewed and tested the change before submitting.

The array branch of `_get_field_parts` validates the declared element
count against `GGUF_MAX_ARRAY_ELEMENTS` but never against the bytes
actually remaining in the file. A count below that ceiling still drives a
full per-element loop, so a small file that declares a large array costs
time proportional to the declared count rather than to the file size.

`_get_str` already performs the equivalent check for strings:

    if offset + 8 + int(slen[0]) > self.data.nbytes:
        raise ValueError(f'String length ... exceeds remaining file size ...')

This adds the matching bound for arrays. Every element occupies at least
one byte, so a declared count greater than the remaining byte count is
not satisfiable and can be rejected before the loop starts.

Found by fuzzing gguf-py with Atheris. A 101-byte input declaring
13,434,883 elements took 43.9s of CPU to parse on master (2d8d612);
with this change it raises immediately. A normal file with a 5,000
element array parses unchanged.

Testing:
- 101-byte fuzzer input: 43.9s before, immediate ValueError after
- Valid 74KB file with a 5,000 element array and one tensor: parses
  correctly with the change applied

AI usage disclosure: AI-assisted. The bug was found by my own fuzzing
campaign against gguf-py. I used AI assistance to identify the fix and
draft this description, and reviewed and tested the change before
submitting.
@ggml-gh-bot

This comment was marked as resolved.

@ggml-gh-bot ggml-gh-bot Bot added the draft PR will be changed to draft by github-actions bot label Sep 1, 2026
@github-actions
github-actions Bot marked this pull request as draft September 1, 2026 00:53
@github-actions github-actions Bot removed the draft PR will be changed to draft by github-actions bot label Sep 1, 2026
@CISC

CISC commented Sep 1, 2026

Copy link
Copy Markdown
Member

The check is incorrect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants