yonlu/yellowjacket
1
1
Fork 0
Code Issues 55 Pull Requests 6 Actions Packages Projects Releases 12 Wiki Activity

tagwriter's RIFF parser allocates whatever size a chunk header claims #216

New Issue
Closed
opened 2026-08-24 09:44:58 +00:00 by logan · 2 comments
logan commented 2026-08-24 09:44:58 +00:00
Collaborator
Copy Link
Copy Source

Finding

Tripped over while giving metadata a RIFF reader for #104.

riff.Parse — which was tagwriter.parseRIFF until that change, and
is still only called by the tag writer — sizes its buffer from the
chunk header:

data := make([]byte, chunkSize)
if _, err := io.ReadFull(r, data); err != nil {
    return nil, fmt.Errorf("read chunk data for %q: %w", id, err)
}

chunkSize is four bytes read off the file, so a truncated or
malformed WAV can declare a 4 GB chunk in a 2 kB file and the parser
allocates 4 GB before discovering there is nothing to read into it.
The error is correct; the allocation happens first.

Reproduction

Write a WAV whose data chunk header declares 0xFFFFFFF0 and then
ends, and call writeWavTags on it. The read fails with
unexpected EOF, having asked the allocator for ~4 GB on the way.

Why it has not bitten

The writer only runs on files the user has asked to tag, and a real
WAV's headers are honest. #104's read path is the one that touches
every file in the library on every scan, and it does not have this
shape: riff.ID3Chunk copies with io.CopyN into a bytes.Buffer, so
it grows with what actually arrives.

Direction

The same treatment in Parse, or a ceiling from the file's own size
(it takes an io.Reader today, so a size is not to hand — ID3Chunk's
io.CopyN is the cheaper answer and needs nothing new).

Priority Low: it needs a malformed file and a deliberate tag write,
and the failure is an allocation rather than a wrong answer. Filing it
because it is the sort of thing that is invisible until a user's disk
has a bad sector in a file they then edit.

**Finding** Tripped over while giving `metadata` a RIFF reader for #104. `riff.Parse` — which was `tagwriter.parseRIFF` until that change, and is still only called by the tag writer — sizes its buffer from the chunk header: ```go data := make([]byte, chunkSize) if _, err := io.ReadFull(r, data); err != nil { return nil, fmt.Errorf("read chunk data for %q: %w", id, err) } ``` `chunkSize` is four bytes read off the file, so a truncated or malformed WAV can declare a 4 GB chunk in a 2 kB file and the parser allocates 4 GB before discovering there is nothing to read into it. The error is correct; the allocation happens first. **Reproduction** Write a WAV whose `data` chunk header declares `0xFFFFFFF0` and then ends, and call `writeWavTags` on it. The read fails with `unexpected EOF`, having asked the allocator for ~4 GB on the way. **Why it has not bitten** The writer only runs on files the user has asked to tag, and a real WAV's headers are honest. #104's *read* path is the one that touches every file in the library on every scan, and it does not have this shape: `riff.ID3Chunk` copies with `io.CopyN` into a `bytes.Buffer`, so it grows with what actually arrives. **Direction** The same treatment in `Parse`, or a ceiling from the file's own size (it takes an `io.Reader` today, so a size is not to hand — `ID3Chunk`'s `io.CopyN` is the cheaper answer and needs nothing new). Priority Low: it needs a malformed file *and* a deliberate tag write, and the failure is an allocation rather than a wrong answer. Filing it because it is the sort of thing that is invisible until a user's disk has a bad sector in a file they then edit.
logan self-assigned this 2026-08-26 10:32:31 +00:00
logan added the
Status
In Progress
 label 2026-08-26 10:32:32 +00:00
logan commented 2026-08-26 10:32:32 +00:00
Author
Collaborator
Copy Link
Copy Source

Picking this up on fix/216-riff-parse-allocation.

Approach is the cheaper of the two the issue names: riff.Parse copies
the chunk body with io.CopyN into a bytes.Buffer, exactly as
riff.ID3Chunk beside it already does, so the buffer grows with what
arrives rather than with what the four-byte header claims. Nothing new
is needed and the reader stays an io.Reader.

The regression test measures rather than asserts the error, because the
error is identical on a build that allocates the gigabyte first —
runtime.MemStats.TotalAlloc across a Parse of a file whose data
chunk declares 1 GiB and then ends.

Picking this up on `fix/216-riff-parse-allocation`. Approach is the cheaper of the two the issue names: `riff.Parse` copies the chunk body with `io.CopyN` into a `bytes.Buffer`, exactly as `riff.ID3Chunk` beside it already does, so the buffer grows with what arrives rather than with what the four-byte header claims. Nothing new is needed and the reader stays an `io.Reader`. The regression test measures rather than asserts the error, because the error is identical on a build that allocates the gigabyte first — `runtime.MemStats.TotalAlloc` across a `Parse` of a file whose `data` chunk declares 1 GiB and then ends.
logan commented 2026-08-26 10:53:11 +00:00
Author
Collaborator
Copy Link
Copy Source

PR #224 is open for this and CI is green (run 18009 on a113b7b: check success, e2e success).

riff.Parse copies the chunk body with io.CopyN into a bytes.Buffer now, matching riff.ID3Chunk beside it, so the buffer grows with what arrives rather than with what the four-byte header claims — the issue's own cheaper answer, needing nothing new.

The regression test measures instead of asserting the error, because the error is the same on a build that allocates first: runtime.MemStats.TotalAlloc across a Parse of a 42-byte container whose data chunk claims 1 GiB read 1,073,750,920 bytes before the fix and under 1 MiB after.

Not merging; leaving Status/In Progress on.

PR #224 is open for this and CI is green (run 18009 on `a113b7b`: `check` success, `e2e` success). `riff.Parse` copies the chunk body with `io.CopyN` into a `bytes.Buffer` now, matching `riff.ID3Chunk` beside it, so the buffer grows with what arrives rather than with what the four-byte header claims — the issue's own cheaper answer, needing nothing new. The regression test measures instead of asserting the error, because the error is the same on a build that allocates first: `runtime.MemStats.TotalAlloc` across a `Parse` of a 42-byte container whose `data` chunk claims 1 GiB read **1,073,750,920 bytes** before the fix and under 1 MiB after. Not merging; leaving `Status/In Progress` on.
logan closed this issue 2026-08-26 16:02:50 +00:00
gitea-actions bot removed the
Status
In Progress
 label 2026-08-26 16:02:59 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Area/Design
Area/Downloads
Area/Explore
Area/Library-UI
Area/Metadata
Area/Packaging
Area/Player
Area/Queue
Area/Settings
Area/Shell-Nav
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
 Platform/Android
Platform/Desktop
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Medium
3
The priority is medium
Priority
Low
4
The priority is low
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
In Progress
Somebody is actively working on this right now
No labels Area/Metadata Kind/Bug Platform/Desktop
Priority
Low
4
Reviewed
Confirmed
1
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logan yonlu
No Assignees logan
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#216