Closed Bug 468281 Opened 11 years ago Closed 11 years ago
Audit alloc failure in oggplay xiph codec
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
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.
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.
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 ago → 11 years ago
Resolution: --- → FIXED
Whiteboard: [sg:investigate] → [sg:investigate] [baking for 1.9.1]
Attachment #365812 - Flags: approval1.9.1+
Pushed to mozilla-1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e77d70d21e63
Whiteboard: [sg:investigate] [baking for 1.9.1] → [sg:investigate]
Verified fixed by checking changesets on trunk and 1.9.1.
You need to log in before you can comment on or make changes to this bug.