Skip to content

Reject trailing Brotli data to prevent infinite drain loops - #232

Open
illia-v wants to merge 4 commits into
python-hyper:mainfrom
illia-v:trailing-data
Open

illia-v wants to merge 4 commits into
python-hyper:mainfrom
illia-v:trailing-data

Conversation

@illia-v

@illia-v illia-v commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Reject trailing data after a complete stream, matching error check of the reference Brotli. This prevents infinite loops while draining the decoder.

Reproducer:

import brotlicffi as brotli  # or import brotli

data = brotli.compress(b"A" * (2**21)) + b"tail"
decoder = brotli.Decompressor()
decoder.process(data, output_buffer_limit=2**20)

while not decoder.can_accept_more_data():
    decoder.process(b"", output_buffer_limit=2**20)

Unpatched brotlicffi loops indefinitely. Patched brotlicffi and reference Brotli raise an error.

@ngoldbaum

ngoldbaum commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Looks good, this implements a check upstream has had since 2018.

I used an AI model to compare this with upstream and noticed two issues and one test suggestion.

Corner cases worth raising

  1. Changelog understates the change. brotlicffi.decompress(stream + junk) and no-limit process(stream + junk) returned the data for the package's entire history and now raise. That's upstream parity and I'd keep it, but HISTORY should say so explicitly rather than only "fixed infinite loops". Note the whole call's output is discarded on the raise, same as upstream.

  2. Post-error state isn't reset. Upstream marks the decoder unhealthy. The PR raises before re-saving _unconsumed_data, so when the raise happens on a second loop iteration (any no-limit call whose initial 5 * len buffer is too small, e.g. compress(b"A"*100) + b"tail") a stale tail stays behind:

    1st: Decompression error: trailing data after stream.
    unconsumed: b'tail' can_accept: False finished: True
    2nd process(b'x'): brotli: decoder process called with data when 'can_accept_more_data()' is False
    finish(): b''
    

    Setting self._unconsumed_data = b'' before the raise (or adding a healthy flag) would make the follow-up error consistent. Minor.

  3. Tests. Cheap additions that mirror upstream's test_garbage_appended and test_already_finished: one-shot decompress(stream + b'x'), and process(stream) then process(b'x').

@illia-v

illia-v commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

@ngoldbaum thanks for the review! I updated the changelog, fixed the stale buffer, and added the regression tests

This branch has not been deployed

No deployments
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.

2 participants