Closed Bug 468281 Opened 11 years ago Closed 11 years ago

Audit alloc failure in oggplay xiph codec

Categories

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

defect
Not set
blocker

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: timeless, Assigned: cajbir)

References

()

Details

(Keywords: crash, verified1.9.1, Whiteboard: [sg:investigate])

Attachments

(1 file, 1 obsolete file)

http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay_data.c?rev=a2c9bc656ed5&mark=277-280,301-304,339-340,384-385
http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay_seek.c?rev=a2c9bc656ed5&mark=96-101,107

107   me->buffer = oggplay_buffer_new_buffer(me->buffer->buffer_size);
has now changed hints w/o error handling. while this isn't exactly realloc (which leaks), it could be a hint that you're exploitable if people don't null check buffer (which is unlikely given coding style). i'm only commenting once about stuff.

http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay_callback.c?rev=a2c9bc656ed5&mark=479-481,598-599,600-601
http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay_callback.c?rev=a2c9bc656ed5&mark=479-481,598-599,600-601,328,332,
http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay_callback_info.c?rev=a2c9bc656ed5&mark=59,71,130-131,
http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay.c?rev=a2c9bc656ed5&mark=51-53,95-96,133,137

note about layering violation: functions 
133   OggPlay *me = oggplay_new_with_reader(reader);
141     free(me);
you shouldn't use free on a struct, your code is sample code which means it will be copied, and if i'm *not* in your library, then my |free| isn't compatible with your |free| (windows, solaris, etc.), this ignores the case where you might actually want to do something interesting. you must have an api available for destroying your struct if your api is public, and you should use it internally (if you're really concerned about performance you can use a macro which is aware about public/private and does the right thing, but you shouldn't worry about that for this stuff).

http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay_file_reader.c?rev=a2c9bc656ed5&mark=138-140,

http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay_buffer.c?rev=a2c9bc656ed5&mark=61-63,63-65,65-66,203,320,326,

instance note this file uses malloc and then memset except when it uses calloc - i can't find the consumer of the unchecked calloc, but at least the memset ensures a fast crash.

http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay_tcp_reader.c?rev=a2c9bc656ed5&mark=162,164,180,182,208-208,211,215,667,373,383-386,660-662

see previous note about free(), you need either a generic oggplay_free which is documented as the to use for oggplay_hostname_and_path, or something similar.

general note about incorrect use of realloc
Flags: blocking1.9.1?
Whiteboard: [sg:investigate]
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
I've ran through of the dynamic memory allocations in liboggplay's code and committed the error checking/handling into it's SVN repository. Revision 3863 has all the changes, see http://trac.annodex.net/changeset/3863 for the changeset.
Attached patch liboggplay svn patch (obsolete) — Splinter Review
Patches from liboggplay svn r3683 and svn r3864 to fix issues as described by wiking in comment 1. Note that changeset r3864 is an additional issue fixed by wiking.
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
The patch in attachment 365092 [details] [diff] [review] must be applied on top of the fix in bug 477899 since that includes a liboggplay update.
Depends on: 477899
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/d132b09831a1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [sg:investigate] → [sg:investigate][baking for 1.9.1]
Backed out due to reftest failure on windows:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236296540.1236302441.31033.gz
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [sg:investigate][baking for 1.9.1] → [sg:investigate]
You need to apply the r3871 as well ( http://trac.annodex.net/changeset/3871 ), otherwise you'll get errors for semaphores using MSVC.
Adds liboggplay commit from svn r3871 to fix windows issue. This should fix the window reftest error.
Attachment #365092 - Attachment is obsolete: true
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/633a58ff7cb3
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [sg:investigate] → [sg:investigate] [baking for 1.9.1]
Pushed to mozilla-1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e77d70d21e63
Keywords: fixed1.9.1
Whiteboard: [sg:investigate] [baking for 1.9.1] → [sg:investigate]
Verified fixed by checking changesets on trunk and 1.9.1.
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.2a1
Flags: wanted1.9.0.x-
Group: core-security
You need to log in before you can comment on or make changes to this bug.