Skip to content

Gp3To5Importer: harden remaining four count-driven loops against DoS (follow-up to #2673) #2674

@kaizenman

Description

@kaizenman

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

This is a follow-up to #2673 / PR #2670, which adds bounds checks to two count-driven loops in Gp3To5Importer (readBend, readTremoloBarEffect). Four additional count-driven loops in the same importer follow the same pattern and are theoretically vulnerable to the same DoS class — a crafted file declaring an oversized count field would skip past the PR #2670 checks and reach one of these:

Loop Location
for i = 0; i < this._barCount; i++ (header bar count) Gp3To5Importer.ts:336, :607
for i = 0; i < this._trackCount; i++ (header track count) Gp3To5Importer.ts:452
for i = 0; i < beatCount; i++ (voice beat count) Gp3To5Importer.ts:654

The original PR #2670 only fixes loops empirically observed to crash on naturally-corrupted files. A crafted file declaring barCount = 2_000_000_000 would still bypass it.

ByteBuffer.readByte() returns -1 silently at EOF rather than throwing, so past-EOF reads cannot terminate the runaway loop on the read side either.

Expected Behavior

Either:

  1. Targeted approach: extend the _requireFits(data, count, bytesPerItem, fieldName) helper introduced in PR fix(importer): bound runaway loops on corrupt count fields in GP3-5 binary parser #2670 to the remaining four loops. Each loop has a known per-iteration byte cost, so the check is straightforward.
  2. Holistic approach: change ByteBuffer.readByte() to throw EndOfReaderError at EOF instead of returning -1. Any past-EOF runaway loop terminates after a single iteration regardless of which count field drove it. Trade-off: contract change; existing callers that rely on the -1 sentinel must be audited.

Either yields the same end-state (no DoS via count-field corruption); they differ in surface area and risk. I'm happy to implement whichever direction you prefer.

Steps To Reproduce

Theoretical — no real-world fixture observed for these four loops (the same DoS class was found and fixed in readBend / readTremoloBarEffect via #2673; these four take the same input shape but were not encountered during reduction of the corrupted-file fixture in #2673). A crafted GP3/4/5 file with deliberately-inflated barCount / trackCount / beatCount header fields would trigger the same OOM-crash class as #2673.

Link to jsFiddle, CodePen, Project

N/A.

Version and Environment

alphaTab: 1.9.0 (commit f8ef24f, current HEAD of `develop`)

The bug is on the importer side; affects every consumer platform (Web, Node.js, .NET, Android, iOS).

Platform

Node.js

Anything else?

This is the follow-up scope identified during PR #2670 review. Filing as a separate issue so the investigation isn't lost while the architectural decision is pending. Related: #2673, PR #2670.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions