927462 - String matching logic in OmxPlugin.cpp is brittle

Status: RESOLVED WORKSFORME

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

Description

[Gian-Carlo Pascutto [:gcp]]

From bug 898767:

One thing Anthony Jones pointed out is that The calls to Match (which calls the strncmp) in OmxDecoder.cpp seem to be incorrect:

 if (!Match(aMimeChars, aMimeLen, "video/mp4") &&
      !Match(aMimeChars, aMimeLen, "audio/mp4") &&
      !Match(aMimeChars, aMimeLen, "audio/mpeg") &&
      !Match(aMimeChars, aMimeLen, "application/octet-stream")) { // file urls
    return false;
  }

If 'aMimeChars' is an empty string, then aMimeLen will be zero and the strncmp will return true. It will also match for aMimeChars being any shorter substring of the needle. Match should use strcmp. I can't see this causing the crash but it should be fixed.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #0)
> Match should use strcmp.

Drive-by comment: are you sure we should switch to strcmp here?

In particular: do we know that aMimeChars is guaranteed to be null-terminated, with the null-terminator at exactly aMimeChars[aMimeLen]? (And if so, why do we even bother with the aMimeLen arg here in the first place, and should we add an assertion that aMimeLen is what we expect it to be?)

Anyway -- it seems like the simpler solution would be to add an "if (aMimeLen == 0)" early-return case before we even bother calling Match. OR, perhaps extending Match() to fail if strlen(aNeedle) != aMimeLen.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

>Drive-by comment: are you sure we should switch to strcmp here?
>
>In particular: do we know that aMimeChars is guaranteed to be null-terminated, with the null-terminator 
>at exactly aMimeChars[aMimeLen]? (And if so, why do we even bother with the aMimeLen arg here in the 
>first place, and should we add an assertion that aMimeLen is what we expect it to be?)

needinfo? doublec

>if (aMimeLen == 0)" early-return case before we even bother calling Match.

I don't think that's enough, see the earlier statement "It will also match for aMimeChars being any shorter substring of the needle."

Comment 3

[Daniel Holbert [:dholbert]]

Right, sorry; I wasn't sure if we were concerned about that situation (overlooked that part of comment 0).

The compiler can calculate the length of the static strings for us, so we might as well pass that in to "Match()" and use it there (i.e. failing early if the lengths don't match; and then still using the (known-equal) length for the call to strnlen so we don't walk off the end of aMimeChars)

The Match invocations might then look something like:
 if (!Match(aMimeChars, aMimeLen, VIDEO_MP4, NS_ARRAY_LENGTH(VIDEO_MP4) - 1)
or maybe
 if (!Match(aMimeChars, aMimeLen, LITERAL_AND_LENGTH("video/mp4"))
where LITERAL_AND_LENGTH would be a macro that evaluates to "arg, (NS_ARRAY_LENGTH(arg) - 1)"

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Patches accepted.

Comment 5

[cajbir (:cajbir)]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> In particular: do we know that aMimeChars is guaranteed to be
> null-terminated, with the null-terminator at exactly aMimeChars[aMimeLen]?
> (And if so, why do we even bother with the aMimeLen arg here in the first
> place, and should we add an assertion that aMimeLen is what we expect it to
> be?)

I'm not sure this is guaranteed as this is a plugin, built into a shared library, and the caller can provide an aMimeLen as needed. We could change the interface but I'm not sure there's a need to.

> if (!Match(aMimeChars, aMimeLen, VIDEO_MP4, NS_ARRAY_LENGTH(VIDEO_MP4) - 1)
> or maybe
>  if (!Match(aMimeChars, aMimeLen, LITERAL_AND_LENGTH("video/mp4"))
> where LITERAL_AND_LENGTH would be a macro that evaluates to "arg, (NS_ARRAY_LENGTH(arg) - 1)"

Just be aware that we can't call Mozilla library functions from within the OmxPlugin code. I'm not sure if NS_ARRAY_LENGTH counts.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #4)
> Patches accepted.

I'll pass for the moment, as I don't have cycles to actually figure out how to test a patch that I'd write. :)

Unassigning since I don't plan on working on this in the near term, but I may end up taking it eventually.

Comment 7

[Nils Ohlmeier [:drno]]

Mass closing do to inactivity.
Feel free to re-open if still needed.

Comment 8

[Daniel Holbert [:dholbert]]

Yeah, looks like this code was removed entirely in bug https://hg.mozilla.org/mozilla-central/rev/1f7435a8c0d5#l1.1067 so there's nothing to be fixed anymore.

--> WORKSFORME due to bug 1379190