Stagefright: NULL deref in call to libc from convertTimeToDate

RESOLVED FIXED in Firefox 45

Status

()

Core
Audio/Video: Playback
P1
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: tsmith, Assigned: rillian)

Tracking

(Blocks: 1 bug, {crash, csectype-nullptr})

Trunk
mozilla45
x86_64
Linux
crash, csectype-nullptr
Points:
---

Firefox Tracking Flags

(firefox42 affected, firefox45 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Created attachment 8641308 [details]
call_stack.txt
(Assignee)

Updated

2 years ago
Assignee: nobody → giles
Priority: -- → P1
(Assignee)

Comment 1

2 years ago
Created attachment 8683360 [details] [diff] [review]
Check for nullptr in convertTimeToDate

I don't see now the current code can fail, so maybe this was addressed bug 1156891. Nevertheless, there's no harm in validating the argument.

Gerald, I didn't see a way to add a gtest for this without exporting the function. Any ideas? Probably ok without a test.

Tyson, can you please attach a testcase so we can verify?
Flags: needinfo?(twsmith)
Attachment #8683360 - Flags: review?(gsquelart)
Comment on attachment 8683360 [details] [diff] [review]
Check for nullptr in convertTimeToDate

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

I've also looked at this issue a few times, but couldn't find any issue.
This pointer couldn't possibly be null, as it's the address of an object on the caller stack!
But anyway, it can't hurt to check for null pointers, so r+, and let's see what's happening...
Attachment #8683360 - Flags: review?(gsquelart) → review+
(Reporter)

Comment 3

2 years ago
(In reply to Ralph Giles (:rillian) from comment #1)
> Created attachment 8683360 [details] [diff] [review]
> Check for nullptr in convertTimeToDate
> 
> I don't see now the current code can fail, so maybe this was addressed bug
> 1156891. Nevertheless, there's no harm in validating the argument.
> 
> Gerald, I didn't see a way to add a gtest for this without exporting the
> function. Any ideas? Probably ok without a test.
> 
> Tyson, can you please attach a testcase so we can verify?

Sorry I don't have a test case for this one. However I have been fuzzing for a while and haven't seen it.
Flags: needinfo?(twsmith)
(Assignee)

Comment 5

2 years ago
Ok, at least it hasn't been re-occuring. Bug 1156891 was more than 3 months ago, so it doesn't seem like it was a direct fix. Maybe it was stack corruption?

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/91b9b18087ec
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.