NULL crash [@ FlacFrameParser::GetTags()]

RESOLVED FIXED in Firefox 61

Status

()

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: decoder, Assigned: decoder)

Tracking

(Blocks 1 bug, {crash})

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

Details

Attachments

(1 attachment)

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.
Component: Audio/Video → Audio/Video: Playback
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 on attachment 8962674 [details]
Bug 1446932 - Handle nullptr in FlacFrameParser::GetTags.

https://reviewboard.mozilla.org/r/231536/#review237020
Attachment #8962674 - Flags: review?(jyavenard) → review+
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f5bd43e4e83d
Handle nullptr in FlacFrameParser::GetTags. r=jya
Assignee: nobody → choller
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/f5bd43e4e83d
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
No crashes in the wild from what I can see, so let's let this ride the trains.
You need to log in before you can comment on or make changes to this bug.