Summary
Beatgrid::parse_id3 fails on the seven-byte payload Serato writes for a track it has not analysed, because the marker count is verified to be greater than zero before any markers are read.
take_non_terminal_marker_count in src/tag/beatgrid.rs:
let (input, count) =
nom::combinator::verify(nom::number::complete::be_u32, |x: &u32| x > &0u32)(input)?;
Ok((input, count - 1))
The verify exists because the stored count includes the terminal marker, so it is decremented to get the non-terminal count and must not underflow. But a stored count of zero is not corruption — it is a well-formed beatgrid with no markers at all, which is what an unanalysed track has.
Seen in the wild on a .wav from a real Serato library:
01 00 00 00 00 00 00 7 bytes
^vers ^count = 0 ^footer
Well-formed, footer present, and rejected. A caller cannot distinguish "this track has no grid" from "this payload is broken", which matters when the two call for different behaviour — here, disabling quantise versus warning the user that a tag failed to read.
Reproduction
use triseratops::tag::{format::id3::ID3Tag, Beatgrid};
fn main() {
let empty: Vec<u8> = vec![0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00];
println!("{:?}", Beatgrid::parse_id3(&empty));
// => Err(ParseError) / "Nom parse error"
}
Suggested fix
Two shapes, depending on how much churn is acceptable.
Minimal — let a zero count through and return no markers, since length_count with a count of zero reads nothing:
fn take_non_terminal_marker_count(input: &[u8]) -> Res<&[u8], u32> {
- let (input, count) =
- nom::combinator::verify(nom::number::complete::be_u32, |x: &u32| x > &0u32)(input)?;
- Ok((input, count - 1))
+ let (input, count) = nom::number::complete::be_u32(input)?;
+ Ok((input, count.saturating_sub(1)))
}
That alone is not sufficient: with a count of zero there is also no terminal marker to read, so take_beatgrid would still fail on the eight bytes it expects next. An empty grid needs recognising before the terminal marker is read — for example by returning early with empty markers and no terminal marker, which would make Beatgrid::terminal_marker an Option.
Given that shape change, it may be preferable to expose the empty case explicitly rather than fold it into the existing type.
Environment
- triseratops at
8e92aae1794c4f02a2405eb88ea72f251b077f0c (repo HEAD)
- rustc 1.98.1
Summary
Beatgrid::parse_id3fails on the seven-byte payload Serato writes for a track it has not analysed, because the marker count is verified to be greater than zero before any markers are read.take_non_terminal_marker_countinsrc/tag/beatgrid.rs:The
verifyexists because the stored count includes the terminal marker, so it is decremented to get the non-terminal count and must not underflow. But a stored count of zero is not corruption — it is a well-formed beatgrid with no markers at all, which is what an unanalysed track has.Seen in the wild on a
.wavfrom a real Serato library:Well-formed, footer present, and rejected. A caller cannot distinguish "this track has no grid" from "this payload is broken", which matters when the two call for different behaviour — here, disabling quantise versus warning the user that a tag failed to read.
Reproduction
Suggested fix
Two shapes, depending on how much churn is acceptable.
Minimal — let a zero count through and return no markers, since
length_countwith a count of zero reads nothing:fn take_non_terminal_marker_count(input: &[u8]) -> Res<&[u8], u32> { - let (input, count) = - nom::combinator::verify(nom::number::complete::be_u32, |x: &u32| x > &0u32)(input)?; - Ok((input, count - 1)) + let (input, count) = nom::number::complete::be_u32(input)?; + Ok((input, count.saturating_sub(1))) }That alone is not sufficient: with a count of zero there is also no terminal marker to read, so
take_beatgridwould still fail on the eight bytes it expects next. An empty grid needs recognising before the terminal marker is read — for example by returning early with empty markers and no terminal marker, which would makeBeatgrid::terminal_markeranOption.Given that shape change, it may be preferable to expose the empty case explicitly rather than fold it into the existing type.
Environment
8e92aae1794c4f02a2405eb88ea72f251b077f0c(repo HEAD)