468296 - Audit alloc failure in ogg xiph container

Status: RESOLVED FIXED

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

Description

[timeless]

note that we have ogg_bitwise.c and bitwise.c, and ogg_framing.c and framing.c diff says they're identical, i'd say this is a bug.

http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/libogg/src/ogg_bitwise.c?rev=a2c9bc656ed5&mark=41-42,71-73,105-107,169-173

same file, two names...

general note about realloc, and some are much worse than others.

http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/media/libogg/src/ogg_framing.c?rev=79c023857355&mark=192,231,281,195,238,300,196,239,301,1510,1526,520-522,999-1000,1003-1004,1206,1239,1510,1526,

useless warning about realloc v. mallco, you can actually safely pass 0 to realloc as the first arg, i.e. this is in some ways overly complicated:
519     if(oy->data)
520       oy->data=_ogg_realloc(oy->data,newsize);
521     else
522       oy->data=_ogg_malloc(newsize);

you could do this:
void *p = ogg_realloc(oy->data, newsize);
if (!p) {
 if (oy->data)
  ogg_free(oy->data);
 local_cleanup();
 return NULL;
}
oy->data = p;

void oggpack_writeinit allocs but because it's void it can't signal failure. this is a design botch (the same applies to a number of other things including readinit ...)

Comment 1

[cajbir (:cajbir)]

(In reply to comment #0)
> note that we have ogg_bitwise.c and bitwise.c, and ogg_framing.c and framing.c
> diff says they're identical, i'd say this is a bug.

It's a bug in the script we use to copy the original ogg source into the mozilla tree. we rename the files to have ogg_ prepended otherwise we get link errors due to similar named files in other libraries,

Comment 2

[cajbir (:cajbir)]

Attachment: Update to libogg svn

Removes unused framing.c and bitwise.c. Updates to libogg svn r16074 to pull in fixes. The issues with malloc and realloc are fixed. The other issues raised don't seem to have been addressed yet.

Comment 3

[Matthew Gregan [:kinetik]]

We should update to the libogg 1.1.4 release to resolve this.

Comment 4

[Matthew Gregan [:kinetik]]

Actually, we should update to SVN to pick up at least r16649 as well.  This'll make bug 548639 easier since we won't need a local patch for the DSP types.

Comment 5

[Matthew Gregan [:kinetik]]

Attachment: update libogg to SVN r16911

The duplicate files will be deleted in bug 550360 (and the update script doesn't have the bug that retains both copies now).  From a quick skim it looks like realloc is called more carefully now.

The problem with the oggpack_writeinit API is "fixed" by adding an oggpack_writecheck API (which is going to require libogg API users to be modified to call it).  It looks like the only in-tree caller is libvorbis, which has not been updated upstream to depend on this new API.

Comment 6

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/mozilla-central/rev/bf58973256df

I'm marking this as fixed as the major issues appear to be resolved in the current libogg source.  We'll naturally pick up the writeinit/writecheck fix for libvorbis in a later update when it begins using the new API.