Closed Bug 1446932 Opened 3 years ago Closed 3 years ago

NULL crash [@ FlacFrameParser::GetTags()]

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: decoder, Assigned: decoder)

Details

(Keywords: crash)

Attachments

(1 file)

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: 3 years ago
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.