Closed Bug 468296 Opened 11 years ago Closed 10 years ago

Audit alloc failure in ogg xiph container

Categories

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

defect
Not set
blocker

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Assigned: kinetik)

References

()

Details

(Keywords: crash, Whiteboard: [sg:audit])

Attachments

(1 file, 1 obsolete file)

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 ...)
Flags: blocking1.9.1?
(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,
Whiteboard: [sg:investigate]
Flags: wanted1.9.1-
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Flags: wanted1.9.1-
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Flags: blocking1.9.1+
Attached patch Update to libogg svn (obsolete) — Splinter Review
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.
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
Whiteboard: [sg:investigate] → [sg:audit]
We should update to the libogg 1.1.4 release to resolve this.
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.
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.
Attachment #383209 - Attachment is obsolete: true
Attachment #433237 - Flags: review?(chris.double)
Assignee: chris.double → kinetik
Attachment #433237 - Flags: review?(chris.double) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.