fix(importer): bound runaway loops on corrupt count fields in GP3-5 binary parser#2670
fix(importer): bound runaway loops on corrupt count fields in GP3-5 binary parser#2670kaizenman wants to merge 1 commit intoCoderLine:developfrom
Conversation
9827cb8 to
598fb34
Compare
…inary parser Gp3To5Importer.readBend and readTremoloBarEffect read a pointCount integer from the input stream and then loop pointCount times, reading 9 bytes per iteration. Neither validates that pointCount * 9 fits in the remaining stream. When the parser becomes misaligned mid-stream (corrupt or crafted input) pointCount can be read from random bytes and reach ~2^31. ByteBuffer.readByte returns -1 silently at EOF rather than throwing, so the loop never terminates from the input side; each iteration allocates a BendPoint object and V8 OOM-crashes after ~10s. Reject counts on the basis that count * bytesPerItem must fit in the remaining stream — a mathematical constraint of the format, not a magic cap. Real bends have ~30 points (270 bytes), well below any stream's remaining capacity, so the check never fires on valid input. Fixture: a 3.8 KB GP5 from a real-world corpus where pointCount reads as 587530544 against 1413 bytes remaining (6 orders of magnitude mismatch). Pre-fix Node OOM-crashes; post-fix throws UnsupportedFormatError in <100ms.
598fb34 to
988c629
Compare
Note on scopeThis PR adds bounds checks to the two count-driven loops empirically observed to OOM-crash on real-world corrupted files (
A crafted (vs naturally-corrupted) file declaring A more holistic alternative would be to make I went with targeted bounds checks for this PR because:
Happy to extend to the remaining four loops with the same |
|
Thanks for the fixes, can you open counterpart bug reports following the template so we have proper tracking of these items? Also: the issue and PR templates are not optional, please be sure to update your description accordingly. |
|
@kaizenman On the issues please use the issue template: https://github.com/CoderLine/alphaTab/issues/new?template=bug_report_form.yml I know it can be a bit cumbersome, but it really helps the project to track things properly (problem&background, expectations, how to reproduce, what version etc.) |
Issues
Fixes #2677
Proposed changes
Gp3To5Importer.readBendandreadTremoloBarEffectread apointCountinteger from the input stream and then looppointCounttimes, reading 9 bytes per iteration. Neither function validates thatpointCount * 9fits in the remaining stream:When the parser becomes misaligned mid-stream (e.g. on a corrupted file where an earlier field shifts the cursor onto random data),
pointCountis read from those random bytes and can be up to ~2^31. The loop then iterates hundreds of millions of times.ByteBuffer.readByte()returns-1silently at EOF rather than throwing, so the loop never terminates from the input side; meanwhile each iteration allocates aBendPointobject — V8 heap grows until OOM-crash.This is a practical denial-of-service vector against any browser page that runs AlphaTab on user-supplied input: a single ~3 KB corrupt
.gp5can OOM-crash the tab in 10–30 seconds, taking down all sibling tabs in the same Chromium renderer process.Reproducer
test-data/guitarpro5/corrupted-bend-point-count.gp5— a 3.8 KB GP5 file. The file'sreadBeatEffectsflag byte mid-stream causesreadTremoloBarEffectto be invoked on garbage, where thepointCountfield reads as 587,530,544 against a remaining stream of 1,413 bytes — a 6-orders-of-magnitude mismatch.Pre-fix: Node OOM-crashes after ~10 s with V8 reporting
Reached heap limit.Post-fix: a typed
UnsupportedFormatErroris thrown immediately.How other parsers handle this fixture
For context — the same fixture exposes downstream defects in other parsers as well (out of scope for this PR but useful as a sanity check that the file is genuinely malformed):
GPExceptionannotated with track / measure / voice / beat coordinates. Python'sstruct.unpackraises on insufficient bytes, which acts as an automatic safety net at every read — the equivalent of whichByteBuffer.readBytelacks (it returns-1silently at EOF).ArithmeticException: / by zeroduring playback (likely a separate downstream bug on a zero-valued field that survived the malformed parse).This PR only addresses the AlphaTab parse-time DoS. The other parsers' downstream behaviors are not affected.
Fix
Add a bounds check before entering the loop. The check is not a magic cap (
MAX_BEND_POINTS = 60) but a mathematical constraint of the format:pointCount × bytes_per_iterationmust fit in the remaining stream. Anything larger is impossible by conservation of bytes.A real bend has at most ~30 points (
30 × 9 = 270bytes) — well below any stream's remaining capacity, so the check never fires on valid input. On corrupt or crafted input wherepointCountis millions or negative, the check fires immediately, before any allocation, and surfaces a typed error the caller can handle.Coverage
The same loop pattern appears in two functions; both are fixed:
readBend(Gp3To5Importer.ts:1334)readTremoloBarEffect(Gp3To5Importer.ts:1036)This covers GP3 / GP4 / GP5 (all three formats share
Gp3To5Importer). GP6 (GpxImporter, BCFZ container) and GP7/8 (Gp7To8Importer+GpifParser, XML-based) do not have analogous binary count-driven loops in this importer path; if similar issues exist elsewhere they would be addressed separately.Scope
Four other count-driven loops in
Gp3To5Importerfollow the same pattern and are theoretically vulnerable to the same DoS class (barCount,trackCount,beatCountheaders). They are tracked separately in #2678, where the architectural choice between (a) extending_requireFitsto those loops and (b) makingByteBuffer.readByte()throw at EOF can be made independently of merging this PR.Tests
Gp5ImporterTest > corrupted-bend-point-count— loads the fixture and assertsUnsupportedFormatErroris thrown. Pre-fix the test would never finish (Node OOM); post-fix it completes in <100 ms.All 173 GP importer tests pass (16 GP3 + 21 GP4 + 40 GP5 + 42 GP7 + 26 GP8 + 28 GPX).
Note
One of three independent fixes in the same area. The others address distinct root causes:
Checklist
Further details