ogg: check and propagate memory allocation failures as appropriate

Message ID 1389552951-11053-1-git-send-email-diego@biurrun.de
State New
Headers show

Commit Message

Diego Biurrun Jan. 12, 2014, 6:55 p.m.
From: Sean McGovern <gseanmcg@gmail.com>

Signed-off-by: Diego Biurrun <diego@biurrun.de>
---

I fixed the bits that I noted during my review.

 libavformat/oggdec.c |   71 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 59 insertions(+), 12 deletions(-)

Comments

Vittorio Giovara Feb. 9, 2014, 1:45 a.m. | #1
On Sun, Jan 12, 2014 at 7:55 PM, Diego Biurrun <diego@biurrun.de> wrote:
> From: Sean McGovern <gseanmcg@gmail.com>
>
> Signed-off-by: Diego Biurrun <diego@biurrun.de>
> ---
>
> I fixed the bits that I noted during my review.
>
>  libavformat/oggdec.c |   71 +++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 59 insertions(+), 12 deletions(-)
>

Apparently ok, any more comments?
Vittorio
Luca Barbato Feb. 9, 2014, 3:59 a.m. | #2
On 09/02/14 02:45, Vittorio Giovara wrote:
> On Sun, Jan 12, 2014 at 7:55 PM, Diego Biurrun <diego@biurrun.de> wrote:
>> From: Sean McGovern <gseanmcg@gmail.com>
>>
>> Signed-off-by: Diego Biurrun <diego@biurrun.de>
>> ---
>>
>> I fixed the bits that I noted during my review.
>>
>>  libavformat/oggdec.c |   71 +++++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 59 insertions(+), 12 deletions(-)
>>
> 
> Apparently ok, any more comments?

I'm quite sure there is at least one place that requires some other kind
of undo in the fail path, remind me to look at it again.

lu
Vittorio Giovara March 1, 2014, 10:35 p.m. | #3
On Sun, Feb 9, 2014 at 4:59 AM, Luca Barbato <lu_zero@gentoo.org> wrote:
> On 09/02/14 02:45, Vittorio Giovara wrote:
>> On Sun, Jan 12, 2014 at 7:55 PM, Diego Biurrun <diego@biurrun.de> wrote:
>>> From: Sean McGovern <gseanmcg@gmail.com>
>>>
>>> Signed-off-by: Diego Biurrun <diego@biurrun.de>
>>> ---
>>>
>>> I fixed the bits that I noted during my review.
>>>
>>>  libavformat/oggdec.c |   71 +++++++++++++++++++++++++++++++++++++++++---------
>>>  1 file changed, 59 insertions(+), 12 deletions(-)
>>>
>>
>> Apparently ok, any more comments?
>
> I'm quite sure there is at least one place that requires some other kind
> of undo in the fail path, remind me to look at it again.

Ping.
Vittorio
Vittorio Giovara April 8, 2014, 10:07 p.m. | #4
On Sat, Mar 1, 2014 at 11:35 PM, Vittorio Giovara
<vittorio.giovara@gmail.com> wrote:
> On Sun, Feb 9, 2014 at 4:59 AM, Luca Barbato <lu_zero@gentoo.org> wrote:
>> On 09/02/14 02:45, Vittorio Giovara wrote:
>>> On Sun, Jan 12, 2014 at 7:55 PM, Diego Biurrun <diego@biurrun.de> wrote:
>>>> From: Sean McGovern <gseanmcg@gmail.com>
>>>>
>>>> Signed-off-by: Diego Biurrun <diego@biurrun.de>
>>>> ---
>>>>
>>>> I fixed the bits that I noted during my review.
>>>>
>>>>  libavformat/oggdec.c |   71 +++++++++++++++++++++++++++++++++++++++++---------
>>>>  1 file changed, 59 insertions(+), 12 deletions(-)
>>>>
>>>
>>> Apparently ok, any more comments?
>>
>> I'm quite sure there is at least one place that requires some other kind
>> of undo in the fail path, remind me to look at it again.
>
> Ping.

PIng.
Luca Barbato April 8, 2014, 11:09 p.m. | #5
On 09/04/14 00:07, Vittorio Giovara wrote:
> On Sat, Mar 1, 2014 at 11:35 PM, Vittorio Giovara
> <vittorio.giovara@gmail.com> wrote:
>> On Sun, Feb 9, 2014 at 4:59 AM, Luca Barbato <lu_zero@gentoo.org> wrote:
>>> On 09/02/14 02:45, Vittorio Giovara wrote:
>>>> On Sun, Jan 12, 2014 at 7:55 PM, Diego Biurrun <diego@biurrun.de> wrote:
>>>>> From: Sean McGovern <gseanmcg@gmail.com>
>>>>>
>>>>> Signed-off-by: Diego Biurrun <diego@biurrun.de>
>>>>> ---
>>>>>
>>>>> I fixed the bits that I noted during my review.
>>>>>
>>>>>  libavformat/oggdec.c |   71 +++++++++++++++++++++++++++++++++++++++++---------
>>>>>  1 file changed, 59 insertions(+), 12 deletions(-)
>>>>>
>>>>
>>>> Apparently ok, any more comments?
>>>
>>> I'm quite sure there is at least one place that requires some other kind
>>> of undo in the fail path, remind me to look at it again.
>>
>> Ping.
> 
> PIng.
> 

Shouldn't break stuff more than it would do in the current state,
probably a run of valgrind injecting memory faults (the wiki documents
how thanks to Derek) might be nice.

lu

Patch

diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 46e4b9a..40eff2d 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -63,6 +63,10 @@  static int ogg_save(AVFormatContext *s)
     struct ogg_state *ost =
         av_malloc(sizeof(*ost) + (ogg->nstreams - 1) * sizeof(*ogg->streams));
     int i;
+
+    if (!ost)
+        return AVERROR(ENOMEM);
+
     ost->pos      = avio_tell(s->pb);
     ost->curidx   = ogg->curidx;
     ost->next     = ogg->state;
@@ -72,6 +76,17 @@  static int ogg_save(AVFormatContext *s)
     for (i = 0; i < ogg->nstreams; i++) {
         struct ogg_stream *os = ogg->streams + i;
         os->buf = av_mallocz(os->bufsize + FF_INPUT_BUFFER_PADDING_SIZE);
+        if (!os->buf) {
+            int n;
+
+            for (n = 0; n < i; n++) {
+                struct ogg_stream *os = ogg->streams + n;
+                av_freep(&os->buf);
+            }
+
+            av_freep(&ost);
+            return AVERROR(ENOMEM);
+        }
         memcpy(os->buf, ost->streams[i].buf, os->bufpos);
     }
 
@@ -173,10 +188,18 @@  static int ogg_new_stream(AVFormatContext *s, uint32_t serial, int new_avstream)
     os->header        = -1;
     os->start_granule = OGG_NOGRANULE_VALUE;
 
+    if (!os->buf) {
+        av_freep(&os);
+        return AVERROR(ENOMEM);
+    }
+
     if (new_avstream) {
         st = avformat_new_stream(s, NULL);
-        if (!st)
+        if (!st) {
+            av_freep(&os->buf);
+            av_freep(&os);
             return AVERROR(ENOMEM);
+        }
 
         st->id = idx;
         avpriv_set_pts_info(st, 64, 1, 1000000);
@@ -191,6 +214,9 @@  static int ogg_new_buf(struct ogg *ogg, int idx)
     uint8_t *nb = av_malloc(os->bufsize + FF_INPUT_BUFFER_PADDING_SIZE);
     int size = os->bufpos - os->pstart;
 
+    if (!nb)
+        return AVERROR(ENOMEM);
+
     if (os->buf) {
         memcpy(nb, os->buf + os->pstart, size);
         av_free(os->buf);
@@ -276,8 +302,11 @@  static int ogg_read_page(AVFormatContext *s, int *str)
     os = ogg->streams + idx;
     os->page_pos = avio_tell(bc) - 27;
 
-    if (os->psize > 0)
-        ogg_new_buf(ogg, idx);
+    if (os->psize > 0) {
+        ret = ogg_new_buf(ogg, idx);
+        if (ret < 0)
+            return ret;
+    }
 
     ret = avio_read(bc, os->segments, nsegs);
     if (ret < nsegs)
@@ -497,7 +526,7 @@  static int ogg_get_headers(AVFormatContext *s)
 static int ogg_get_length(AVFormatContext *s)
 {
     struct ogg *ogg = s->priv_data;
-    int i;
+    int i, ret = 0;
     int64_t size, end;
 
     if (!s->pb->seekable)
@@ -512,7 +541,10 @@  static int ogg_get_length(AVFormatContext *s)
         return 0;
     end = size > MAX_PAGE_SIZE ? size - MAX_PAGE_SIZE : 0;
 
-    ogg_save(s);
+    ret = ogg_save(s);
+    if (ret < 0)
+        goto fail;
+
     avio_seek(s->pb, end, SEEK_SET);
 
     while (!ogg_read_page(s, &i)) {
@@ -525,9 +557,20 @@  static int ogg_get_length(AVFormatContext *s)
         }
     }
 
-    ogg_restore(s, 0);
+    ret = ogg_restore(s, 0);
+    if (ret < 0)
+        goto fail;
 
-    return 0;
+    return ret;
+
+fail:
+    for (i = 0; i < ogg->nstreams; i++) {
+        struct ogg_stream *os = ogg->streams + i;
+        av_freep(&os->buf);
+    }
+
+    av_freep(&ogg->state);
+    return ret;
 }
 
 static int ogg_read_close(AVFormatContext *s)
@@ -554,20 +597,24 @@  static int ogg_read_header(AVFormatContext *s)
     ogg->curidx = -1;
     //linear headers seek from start
     ret = ogg_get_headers(s);
-    if (ret < 0) {
-        ogg_read_close(s);
-        return ret;
-    }
+    if (ret < 0)
+        goto fail;
 
     for (i = 0; i < ogg->nstreams; i++)
         if (ogg->streams[i].header < 0)
             ogg->streams[i].codec = NULL;
 
     //linear granulepos seek from end
-    ogg_get_length(s);
+    ret = ogg_get_length(s);
+    if (ret < 0)
+        goto fail;
 
     //fill the extradata in the per codec callbacks
     return 0;
+
+fail:
+    ogg_read_close(s);
+    return ret;
 }
 
 static int64_t ogg_calc_pts(AVFormatContext *s, int idx, int64_t *dts)