1446932 - NULL crash [@ FlacFrameParser::GetTags()]

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

While testing the media layer I ended up with a crash [@ FlacFrameParser::GetTags()] which was called with mParser == nullptr.

I fixed this by adding a nullptr check in the function and returning tags without anything being added to it, e.g.:

>    MetadataTags* tags;
>  
>    tags = new MetadataTags;
> +
> +  if (!mParser)
> +    return tags;
> +
>    for (uint32_t i = 0; i < mParser->mTags.Length(); i++) {

Is this the right fix here? I can make a proper patch in that case.

If this is not sufficient, I can provide you with a testcase and/or crash stack.

Comment 1

[Jean-Yves Avenard [:jya]]

yes, that fix will be sufficient. it will return an empty tags, which is what we want.

Or better, at the top:
if (!mParser) {
  return nullptr;
}

as the code checks for the return value anyway, and prevent the unnecessary memory allocation.


The alternative path would be to consider the FlacFrameParser to not be valid until FlacFrameParser::mParser isn't null, but that's more work.

Comment 2

[Christian Holler (:decoder)]

Attachment: Bug 1446932 - Handle nullptr in FlacFrameParser::GetTags.

Review commit: https://reviewboard.mozilla.org/r/231536/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231536/

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8962674 [details]
Bug 1446932 - Handle nullptr in FlacFrameParser::GetTags.

https://reviewboard.mozilla.org/r/231536/#review237020

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5bd43e4e83d
Handle nullptr in FlacFrameParser::GetTags. r=jya

Comment 5

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/f5bd43e4e83d

Comment 6

[Ryan VanderMeulen [:RyanVM]]

No crashes in the wild from what I can see, so let's let this ride the trains.