789617 - Improve vorbis comment validation

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

Currently we don't do careful validation of vorbis comments when returning metadata.

This bug is about adding checks to be more strict.

Comment 1

[Ralph Giles (:rillian)]

Attachment: proposed fix

Comment 2

[Ralph Giles (:rillian)]

Attachment: proposed fix

Updated patch. Validates tag names using the correct subset of ASCII.

Comment 3

[Ralph Giles (:rillian)]

Attachment: corresponding tests

Corresponding tests to verify the new validation code. Also cleans up the test_metadata.html status messages a bit.

Comment 4

[Ralph Giles (:rillian)]

pushed to try https://tbpl.mozilla.org/?tree=Try&rev=a3d392bfe049

Comment 5

[Ralph Giles (:rillian)]

Comment on attachment 659846 [details] [diff] [review]
proposed fix

Ready for review.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 659884 [details] [diff] [review]
corresponding tests

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

::: content/media/test/manifest.js
@@ +347,5 @@
>      }
>    },
> +  { name:"sound.ogg", tags: { } },
> +  { name:"badtags.ogg", tags: {
> +      // We list only the valid tags here, and verify

It would be good to include the list of invalid tags that are filtered out here too, for code future reference.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 659846 [details] [diff] [review]
proposed fix

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

::: content/media/ogg/nsOggReader.cpp
@@ +162,5 @@
>  
> +static bool IsValidVorbisTagName(nsCString& name)
> +{
> +  // Vorbis comment tag names must be ASCII.
> +  if (!IsASCII(name)) {

I don't think you need to call IsASCII(); your loop below does a more restrictive version of the same check that IsASCII() does, so you'll be doing more work than necessary.

Comment 8

[Ralph Giles (:rillian)]

Attachment: proposed fix

You're right. I've removed the IsASCII check, which also let me simplify the logging by several lines.

Comment 9

[Ralph Giles (:rillian)]

Attachment: corresponding tests

Updated tests, adding comments describing the invalid tags we verify the implementation is skipping.

Carrying forward :cpearce's earlier r+ after irc discussion.

Comment 10

[Ralph Giles (:rillian)]

New try push https://tbpl.mozilla.org/?tree=Try&rev=af08711aceff

Comment 11

[Ralph Giles (:rillian)]

Push is green. Three known orange outside the media suite.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ralph Giles (:rillian) from comment #10)
> New try push https://tbpl.mozilla.org/?tree=Try&rev=af08711aceff

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b86416037e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/72e3b9a6615f

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7b86416037e1
https://hg.mozilla.org/mozilla-central/rev/72e3b9a6615f