Fix unterminated emsg infinite loop and scheme parsing

[Jaybhade]

This PR will...

Stop parseEmsg() from looping forever when an emsg box ends before the null terminator of its scheme_id_uri or value, and read the version 0 layout from the correct offset.

Why is this Pull Request needed?

1. A truncated or non-conformant emsg box hangs the demuxer permanently

Both string fields are walked a byte at a time:

while (bin2str(data.subarray(offset, offset + 1)) !== '\0') {
  schemeIdUri += bin2str(data.subarray(offset, offset + 1));
  offset += 1;
}

Once offset reaches the end of the box, data.subarray(offset, offset + 1) is an empty Uint8Array, so bin2str() returns '' — which never equals '\0'. The loop has no other exit and spins forever incrementing offset.

findBox() clamps a box to the bytes that are actually present (const boxEnd = Math.min(endbox, end)), so a segment whose delivery stops mid-box produces exactly this input; so does a packager that writes an unterminated string.

parseEmsg() runs from MP4Demuxer.extractID3Track() on every emsg box in an fMP4 segment, with no config gate. enableWorker defaults to true, so the transmuxer worker wedges at 100% CPU — playback stops, no ERROR event is emitted, and there is no state to recover from.

Three inputs that never return on master:

input	master	this PR
v1 box truncated after the fixed fields	hangs	schemeIdUri: ''
v1 box whose scheme_id_uri has no terminator	hangs	schemeIdUri: 'urn:a'
v1 emsg clamped by findBox() to the bytes received	hangs	schemeIdUri: ''

Worth noting for comparison: mux.js's equivalent loop indexes with data[index], so past the end it reads undefined, String.fromCharCode(undefined) is '\0', and the loop terminates. Reading a byte via subarray() instead is what removes that exit.

2. Version 0 has never parsed

The version 1 branch steps over the FullBox version and flags with offset += 4; the version 0 branch does not, so it starts reading scheme_id_uri at the version byte. That byte is 0, so the string terminates immediately and schemeIdUri is always "\0". It then reads timescale, presentation_time_delta, event_duration and id from hardcoded offsets 12/16/20/24, which land inside the URI text for any real box.

A well-formed v0 box carrying https://aomedia.org/emsg/ID3, VALUE=1, timescale 90000, delta 4500, duration 180000, id 7:

master:  schemeIdUri="\0"  value="\0"  timeScale=1634692453
         presentationTimeDelta=1684627758  eventDuration=1869768495  id=1701671783
this PR: schemeIdUri="https://aomedia.org/emsg/ID3\0"  value="1\0"  timeScale=90000
         presentationTimeDelta=4500  eventDuration=180000  id=7

Because "\0" matches neither emsgSchemePattern nor config.emsgKLVSchemaUri, every version 0 emsg box is dropped downstream today, so correcting it cannot regress a stream that currently works.

Are there any points in the code the reviewer needs to double check?

• readCString() returns the terminator as part of the string so callers advance by .length. That preserves the previous output: schemeIdUri and value still carry their trailing \0.
• presentationTime is now left undefined for version 0 rather than defaulting to 0. getEmsgStartTime() branches on Number.isFinite(presentationTime), so a 0 default would time every version 0 event at 0 instead of timeOffset + presentationTimeDelta / timeScale. The field is already optional on IEmsgParsingData.
• Version 1 output is unchanged. I ran 20,000 randomised well-formed version 1 boxes (varying scheme, value, timescale, presentation time, duration, id and payload) through the old and new implementations: 20,000/20,000 returned identical results. The same 20,000 boxes rebuilt as version 0 all now match the spec layout, and agree field-for-field with mux.js on every box it accepts.
• The three regression tests hang rather than fail against the unpatched parseEmsg — that is the bug they cover.
• Happy to split the version 0 offset correction into a separate PR if you would rather keep this one to the loop fix.

Resolves issues:

None open — found while reading the fMP4 in-band metadata path.

Checklist

• changes have been done against master branch, and PR does not conflict
• new unit / functional tests have been added (whenever applicable)
• API or design changes are documented in API.md — no API or config change

[itsjamie]

Thanks for the contribution @Jaybhade.

If you have the time, could you add a test to cover a regression for a properly terminated scheme followed by an improper unterminated value, but the buffer length is matches so readCString ends properly.

I'm open to your thoughts on what the behaviour should be. But as it's technically not a valid value (doesn't end with null termination) I could see an argument for the scheme being present and value being empty, for v1 emsg boxes. For v0 I'd assume empty since it would be a completely malformed box.

If you know what the behaviour was on master compared to the current behaviour in this PR, let me know!

[Jaybhade]

Thanks for looking at it. Test added, and I agree with your read — I've pushed both, as two commits so the behaviour change reverts on its own if you'd rather keep it out.

f2c907a adds the regression you asked for, for both versions: a properly terminated scheme_id_uri followed by a value that runs to the last byte of the box, so readCString stops on the buffer end rather than on a terminator.

What master does with that input. The two versions differ, and neither is what you'd want:

• v1 hangs. The value loop walks past the end, data.subarray(offset, offset + 1) is empty, bin2str returns '', and '' !== '\0' forever. This is the hang the PR is about — a terminated scheme doesn't save it, since the box still ends inside a string.
• v0 never reaches the strings at all. With offset starting at 0, the version byte terminates scheme_id_uri and the first flags byte terminates value, so both come back as "\0" and the loops always exit. The fields then come off the hardcoded offsets 12/16/20/24, which land inside the URI text: for https://aomedia.org/emsg/ID3 that gives timeScale: 1634692453 (0x616f6d65, "aome"), id: 1701671783 ("g/ID"), and a payload of "/ID3\0" + "1". So the unterminated value isn't observable on v0 — it's masked by the bigger bug, which is why the PR moves v0 past the version and flags too.

What I think it should be, and what's now in the branch. Your instinct matches mine, so 7be40ac implements it: readCString returns null when it reaches the end of the buffer instead of a terminator, and an unterminated string reads as '' with the rest of the box consumed.

• v1: the scheme is kept when it is terminated, value: '', empty payload — exactly what you described.
• v0: nothing is kept. Every field of a v0 box follows both strings, so if either one is unterminated there is nothing left to trust.

Before that commit the parser handed back the trailing bytes as a truncated string, and the concrete reason I'd argue against that: a partial scheme_id_uri can still satisfy the scheme test in extractID3Track — emsgSchemePattern is an unanchored /\/emsg[-/]ID3/i, and the KLV path is a startsWith — so a box that never fully arrived could push a zero-length metadata sample. An empty string can't.

One related thing worth your call: when only the value is truncated, v1 still returns a valid scheme with an empty payload, so extractID3Track will push a zero-length sample for a box it never fully received. Dropping that box in the demuxer is a small change on top — happy to add it, or to leave the parser reporting what it found and keep that decision out of mp4-tools.

npm run sanity-check is green: type-check, lint, prettier, build, es-check, and 1163/1163 unit tests.

[itsjamie]

On the truncated value, a warning log and pushing the empty value so as to be sure to not leak any corrupted data from the improper ID3 makes sense to me.

The warn will let developers know their stream is broken.

[robwalch]

Applied milestone is dependent on issue confirmation and review approval. Added it so we don't let such an escape slip.

[robwalch]

It would be good to know, as part of the issue description/analysis, if this issue exists in 1.6.x or if it is a recent regression in master.

[Jaybhade]

@itsjamie Warn added in ff7684e, on both versions, naming which string ran off the end and what got dropped:

Unterminated value in parsing emsg box: version 1 box ends inside a string, dropping its value and payload
Unterminated scheme_id_uri in parsing emsg box: version 1 box ends inside a string, dropping its strings and payload

The empty value was already what the branch pushed; this makes it audible. The two existing unterminated-value tests now assert the warn fires exactly once — and they're load-bearing rather than decorative: with just the two logger.warn calls deleted and nothing else changed, those two tests fail (1161 passing / 2 failed) and pass again with them restored.

@robwalch It exists in 1.6.x — it is not a master regression. It has been there since emsg ID3 support was added in a5c669a (Oct 2021), first shipped in v1.2.0, and the two loops are byte-for-byte identical from that commit through v1.6.17.

Measured rather than read, against the published bundles: I lifted parseEmsg verbatim out of each release's dist/hls.js, ran the same truncated v1 box (terminated scheme_id_uri, then a value that runs to the last byte) through it in a worker thread, and watched for a return.

version	result
hls.js@1.2.0	no return in 5s — never terminates
hls.js@1.4.14	no return in 5s — never terminates
hls.js@1.5.20	no return in 5s — never terminates
hls.js@1.6.0	no return in 5s — never terminates
hls.js@1.6.17	no return in 5s — never terminates

So every supported line hangs on this input. Worth noting for severity: parseEmsg runs from MP4Demuxer.extractID3Track() on every emsg box with no config gate, and enableWorker defaults to true, so the transmuxer worker pins a core indefinitely — playback stops with no ERROR event to react to. findBox clamps a box to the bytes present (Math.min(endbox, end)), so a segment truncated mid-box produces this input without needing a malformed packager.

The second defect in the PR — v0 never doing offset += 4 past the FullBox version and flags — dates from the same commit and is likewise unchanged through 1.6.17. It's separately observable: on master the v0 version byte terminates scheme_id_uri and the first flags byte terminates value, so both read as "\0", and the fields come off the hardcoded offsets 12/16/20/24, which land inside the URI text (timeScale: 1634692453 = "aome"). Since "\0" matches neither emsgSchemePattern nor emsgKLVSchemaUri, every v0 emsg was silently dropped — which is why fixing it carries no regression risk.

Gates after ff7684e: type-check, eslint, prettier, 1163/1163 karma unit tests.