Skip to content

fix(qr): bound multipart parser resources - #150

Closed
Jdelg718 wants to merge 2 commits into
odudex:masterfrom
Jdelg718:fix/qr-multipart-resource-limits
Closed

fix(qr): bound multipart parser resources#150
Jdelg718 wants to merge 2 commits into
odudex:masterfrom
Jdelg718:fix/qr-multipart-resource-limits

Conversation

@Jdelg718

Copy link
Copy Markdown
Contributor

Problem

Hostile or inconsistent PMOFN/BBQr sequences could grow retained allocations before final assembly checks.

Changes

  • Add insertion-time limits of 1024 parts and 1 MiB of stored multipart bytes.
  • Strictly validate PMOFN metadata with overflow-safe decimal parsing.
  • Bind multipart totals and BBQr encoding/file type to the first accepted frame.
  • Enter a terminal failure state after metadata or resource-limit violations.
  • Replace duplicate parts atomically with correct retained-byte accounting.
  • Add a real parser host-test harness wired into scripts/test.sh.
  • Exclude host test sources from firmware CMake inputs.

Verification

  • Focused parser host tests, including ASan/UBSan: pass.
  • Full host suite (./scripts/test.sh): pass.
  • wave_43 ESP-IDF v6.0.2 build: pass.
  • Independent fail-closed code review: pass.

Compatibility / risk

Valid PMOFN, BBQr, UR, and plain flows remain tested. Sequences above the new limits fail closed.

@odudex

odudex commented Aug 27, 2026

Copy link
Copy Markdown
Owner

Hi @Jdelg718 . Thank you!
I squashed the two commits of this PR and changed a few nits.
Kept your authorship. Please check final commit (b9a09c9) and let me know if you disagree with any change.

@odudex odudex closed this Aug 27, 2026
@Jdelg718

Copy link
Copy Markdown
Contributor Author

thank you. I reviewed final commit b9a09c9 and agree with your changes. The allocation-failure distinction and scanner message, incomplete-PMOFN compatibility, added fault-injection tests, cleanup target, and documentation all improve the patch.

I reran the focused parser tests, ASan/UBSan, full host suite, formatting checks, and the wave_43 firmware build. Everything passed, and the host test source remains excluded from the firmware.

One non-blocking follow-up observation: alloc_failed currently covers part-storage allocations, but an allocation failure in ur_decoder_new() remains non-terminal, while initial BBQr metadata allocation failure is reported as generic invalid input. This is not caused by your changes and does not change my agreement with the commit. I’m happy to address it separately if you think it would be useful.

Thanks again for preserving the authorship and improving the contribution.

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