Closed Bug 520500 Opened 10 years ago Closed 10 years ago

Vorbis files rejected when fishsound fails to parse comments


(Core :: Audio/Video, defect)

Not set



Tracking Status
status1.9.2 --- beta2-fixed
status1.9.1 --- .8-fixed


(Reporter: kinetik, Assigned: cpearce)




(Keywords: regression, testcase, verified1.9.2)


(1 file)

The following files from the bug URL fail to play:

The reason is the same for every file: they contain what looks like an invalid Vorbis comment, so fish_sound_comments_decode returns FISH_SOUND_ERR_OUT_OF_MEMORY, which causes fs_vorbis_decode to bail early with the same error.

I think it makes more sense to just ignore the invalid comments and try to treat the rest of the file as valid than to reject the entire file due to some bad optional metadata.
The invalid comment is a plain string, not a KV pair as required by the spec.  It looks like fishsound tries to handle this case, because fish_sound_comments_decode has logic for handling a key without a value, but it calls fs_comment_new passing an explicit NULL, and that is rejected by fs_comment_validate_byname.

#0  fs_comment_validate_byname (name=0x1d0b1680 "Track encoded by encoder_example.c", value=0x0) at /Users/kinetik/work/mozilla-central/media/libfishsound/src/libfishsound/fishsound_comments.c:141
#1  0x127af5cf in fs_comment_new (name=0x1d0b1680 "Track encoded by encoder_example.c", value=0x0) at /Users/kinetik/work/mozilla-central/media/libfishsound/src/libfishsound/fishsound_comments.c:160
#2  0x127afe5c in fish_sound_comments_decode (fsound=0x1c9f75d0, comments=0x1298c07 " ", length=79) at /Users/kinetik/work/mozilla-central/media/libfishsound/src/libfishsound/fishsound_comments.c:515
(More stack frames follow...)

(gdb) fr
#0  fs_comment_validate_byname (name=0x1d0b1680 "Track encoded by encoder_example.c", value=0x0) at /Users/kinetik/work/mozilla-central/media/libfishsound/src/libfishsound/fishsound_comments.c:141
141	  if (!name || !value) return 0;

It seems like this case could be fixed easily by passing an empty string, rather than NULL, to fs_comment_new.  But that doesn't fix the general problem of treating comment decoding as a hard error, which I suspect we should treat as non-fatal.
Duplicate of this bug: 496326
There's a work-in-progress patch in bug 521863 which fixes libfishsound's comment parsing. We should move it here. Additionally bug 496326 is a dupe of this. It's test cases: (Crashes with WIP patch from bug
Assignee: nobody → chris
Attached patch Patch v1Splinter Review
Patch to libfishsound to not reject vorbis tracks with comments with name or value length 0. No whitespace changes for all you Emacs lovers out there.
Attachment #406148 - Flags: review?(chris.double)
Attachment #406148 - Flags: review?(chris.double) → review+
We should take this on 1.9.2, it ensures that some files with valid vorbis comments are not incorrectly rejected as invalid.
Flags: blocking1.9.2?
Closed: 10 years ago
Resolution: --- → FIXED
It's working fine on 1.9.1. So do we know which fix it has been regressed?

Verified fixed on trunk with builds on OS X and Windows like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091014 Minefield/3.7a1pre ID:20091014030825
Flags: in-testsuite?
Keywords: regression, testcase
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a1
Marking in-testsuite+, as the patch included content/media/test/bug520500.ogg which test this condition.

(In reply to comment #7)
> It's working fine on 1.9.1. So do we know which fix it has been regressed?

I was unable to play bug520500.ogg on Windows Firefox 3.5.3 and current 191 [Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv: Gecko/20091015 Shiretoko/3.5.5pre (.NET CLR 3.5.30729)]. FF3.5.3 fails to load the file, whereas 3.5.5pre can load it, but fails to play it.

However is another example of a valid ogg file which exhibits this problem, and it does work in 3.5.3 but not 3.5.5pre. So we've regressed between 3.5.3 and 3.5.5pre.

I can only assume one of the Annodex ogg library updates that we recently pushed to 191 caused this behaviour change between 3.5.3 and 3.5.5pre. Regardless, the 191 branch still can only play some of these valid files and has regressed since 3.5.3.

dveditz: Do you want to take this patch on 3.5.3 as well to fix this regression? It merely means we don't sometimes reject some valid ogg files with 0 length vorbis "comment" meta-data. I don't know how common this type of file is, but they're valid files.
Flags: in-testsuite? → in-testsuite+
Flags: blocking1.9.2? → blocking1.9.2+
Applied to upstream libfishsound commit ac2015dbd72d59208a482de5b0e69e74774e7987
Updated in upstream libfishsound commit 46faa6b42a47efed66be99569291f0153f50aae8

(earlier applied patch from 520500, this commit includes additional change in comment #4 patch)
(In reply to comment #9)

Turns out we've already branched of 1.9.2 for beta 1, so this fix won't show up on 1.9.2 until a release after the beta. status-1.9.2 -> final-fixed.
Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b2pre) Gecko/20091019 Namoroka/3.6b2pre ID:20091019052445

Lets ask for 1.9.1 status so we don't loose track on 1.9.1.
status1.9.1: --- → ?
Keywords: verified1.9.2
Blocks: 529632
Duplicate of this bug: 529632
Comment on attachment 406148 [details] [diff] [review]
Patch v1

dveditz: can we take this patch bug on 1.9.1? It is a regression introduced by the annodex ogg library updates we took in FF3.5.4, and causes a reasonably common set of valid vorbis files to not play in 3.5.4.
Attachment #406148 - Flags: approval1.9.1.6?
Comment on attachment 406148 [details] [diff] [review]
Patch v1

We can take it on the 1.9.1 branch, but it missed the release.
Attachment #406148 - Flags: approval1.9.1.6? → approval1.9.1.7?
Comment on attachment 406148 [details] [diff] [review]
Patch v1

Approved for, a=dveditz for release-drivers
Attachment #406148 - Flags: approval1.9.1.8? → approval1.9.1.8+
(In reply to comment #10)
> Applied to upstream libfishsound commit
> ac2015dbd72d59208a482de5b0e69e74774e7987

It turned out that this commit broke the test suite: it made the vorbiscomment handling fail for multiple comments of the same key, and it also chopped off the first character of every comment value.

I reverted this change and the related 46faa6b4, added a test to allow comments which are not of form KEY=VALUE, and implemented a cleaner fix that can add and retrieve such comments, in these upstream commits:

e56f4d8a5f2a008f4f51f30d60d53c0584ebb930 Mozilla #520500: Allow NULL-value comments
d0b3830b86302bf903a254d9fe49ac0a44104dfb tests: allow NULL-valued comments
0e01ba99fb5f642d76b012f8a6097cf0c8bfae69 Revert ac2015db, 46faa6b4

This passes the existing testsuite in libfishsound, and also correctly decodes all the comments of files listed in this bug URL.
You need to log in before you can comment on or make changes to this bug.