Closed Bug 1203217 Opened 4 years ago Closed 4 years ago

Extend MP3 demuxer test coverage and logging for files with large ID3v2 tags

Categories

(Core :: Audio/Video: Playback, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: JanH, Assigned: JanH)

References

Details

Attachments

(2 files, 4 obsolete files)

Bug 1197985 fixed the problem whereby ID3v2 tags larger than the parsing buffer weren't properly skipped, leaving the MPEG frame parser vulnerable to erroneously detecting frame sync within the ID3 tag.

We should extend the existing gtest to include an MP3 file which trips up the original implementation and also possibly add a bit of logging when we skip an ID3 tag.
That's an excellent idea. Our team doesn't have time immediately, so setting priority to match until we have immediate feature work out of the way.
Priority: -- → P5
Attachment #8660919 - Flags: review?(kinetik)
Attachment #8660921 - Flags: review?(kinetik)
Sorry, fixed a typo.
Attachment #8660919 - Attachment is obsolete: true
Attachment #8660919 - Flags: review?(kinetik)
Attachment #8660924 - Flags: review?(kinetik)
Attachment #8660924 - Attachment is obsolete: true
Attachment #8660924 - Flags: review?(kinetik)
Attachment #8660925 - Flags: review?(kinetik)
Comment on attachment 8660921 [details] [diff] [review]
Part 2 - Add logging for ID3v2 tag detection.patch

Review of attachment 8660921 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MP3Demuxer.cpp
@@ +630,5 @@
>        if (tagSize > remainingBuffer) {
>          // Skipping across the ID3 tag would take us past the end of the buffer, therefore we
>          // return immediately and let the calling function handle skipping the rest of the tag.
> +        MP3DEMUXER_LOGV("ID3v2 tag detected, size=%d, "
> +                    "needing to skip %d bytes past the current buffer",

Indentation is off.
Attachment #8660921 - Flags: review?(kinetik) → review+
Attachment #8660925 - Flags: review?(kinetik) → review+
(In reply to Matthew Gregan [:kinetik] from comment #7)
> Comment on attachment 8660921 [details] [diff] [review]
> Part 2 - Add logging for ID3v2 tag detection.patch
> 
> Review of attachment 8660921 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MP3Demuxer.cpp
> @@ +630,5 @@
> >        if (tagSize > remainingBuffer) {
> >          // Skipping across the ID3 tag would take us past the end of the buffer, therefore we
> >          // return immediately and let the calling function handle skipping the rest of the tag.
> > +        MP3DEMUXER_LOGV("ID3v2 tag detected, size=%d, "
> > +                    "needing to skip %d bytes past the current buffer",
> 
> Indentation is off.

A few of the other logging macros are suffering from the same problem (which is where I picked it up), I'll fix those as well.
cleans up the indentation for the logging macros, carrying forward r+ from kinetik
Attachment #8660921 - Attachment is obsolete: true
Attachment #8661300 - Flags: review+
Keywords: checkin-needed
fix typo in commit message, carrying forward r+ from kinetik
Attachment #8661300 - Attachment is obsolete: true
Attachment #8661658 - Flags: review+
You need to log in before you can comment on or make changes to this bug.