468281 - Audit alloc failure in oggplay xiph codec

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[timeless]

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

Comment 1

[Viktor Gal]

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.

Comment 2

[cajbir (:cajbir)]

Attachment: liboggplay svn patch

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.

Comment 3

[cajbir (:cajbir)]

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.

Comment 4

[cajbir (:cajbir)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/d132b09831a1

Comment 5

[cajbir (:cajbir)]

Backed out due to reftest failure on windows:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236296540.1236302441.31033.gz

Comment 6

[Viktor Gal]

You need to apply the r3871 as well ( http://trac.annodex.net/changeset/3871 ), otherwise you'll get errors for semaphores using MSVC.

Comment 7

[cajbir (:cajbir)]

Attachment: Additional liboggplay patch to fix windows issue

Adds liboggplay commit from svn r3871 to fix windows issue. This should fix the window reftest error.

Comment 8

[cajbir (:cajbir)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/633a58ff7cb3

Comment 9

[cajbir (:cajbir)]

Pushed to mozilla-1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e77d70d21e63

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed by checking changesets on trunk and 1.9.1.