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
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions