On Tue, 8 Oct 2013, Alexandra Khirnova wrote:
also make functions use ff_dynarray directly instead of dynarray_add
macro, macro is removed
---
libavformat/cutils.c | 15 +++++++--
libavformat/hls.c | 7 ++--
libavformat/hlsproto.c | 9 +++--
libavformat/internal.h | 17 +---------
libavformat/matroskadec.c | 8 +++--
libavformat/mpegtsenc.c | 3 +-
libavformat/rtpproto.c | 17 +++++++---
libavformat/rtsp.c | 82 +++++++++++++++++++++++++++++----------------
libavformat/rtspenc.c | 5 ++-
libavformat/utils.c | 6 ++--
10 files changed, 107 insertions(+), 62 deletions(-)
diff --git a/libavformat/cutils.c b/libavformat/cutils.c
index f58e152..5cf85b5 100644
--- a/libavformat/cutils.c
+++ b/libavformat/cutils.c
@@ -22,10 +22,10 @@
#include "internal.h"
/* add one element to a dynamic array */
-void ff_dynarray_add(intptr_t **tab_ptr, int *nb_ptr, intptr_t elem)
+int ff_dynarray_add(intptr_t **tab_ptr, int *nb_ptr, intptr_t elem)
{
/* see similar avconv.c:grow_array() */
- int nb, nb_alloc;
+ int nb, nb_alloc, i;
intptr_t *tab;
nb = *nb_ptr;
@@ -35,11 +35,20 @@ void ff_dynarray_add(intptr_t **tab_ptr, int *nb_ptr,
intptr_t elem)
nb_alloc = 1;
else
nb_alloc = nb * 2;
- tab = av_realloc(tab, nb_alloc * sizeof(intptr_t));
+ tab = av_realloc(tab, nb_alloc * sizeof(*tab));
Changing intptr_t to *tab here is pretty much unrelated to the rest, so
please don't do that, the patch is hard enough to review already.
+ if (!tab) {
+ for (i = 0; i < nb; i++)
+ av_freep(tab[i]);
Did you test this? Because I'm pretty sure that this isn't right.
If unsure, please try to trigger the error paths and make sure they work
right. If not, they're just adding exploitable crashes.
Also in general, the code doesn't seem to expect all things that are added
to be pointers, so can we really assume that we can free the elements
using av_free? I haven't read through every single caller yet.
If this now is a requirement, it needs to be documented very clearly so
that nobody accidentally tries to use this function for anything else than
pointers allocated with av_malloc/av_realloc.
It should probably free the new element that it tries to add as well.
+ av_freep(tab);
+ *nb_ptr = 0;
You really need to set *tab_ptr to NULL here as well
+ return AVERROR(ENOMEM);
+ }
*tab_ptr = tab;
}
tab[nb++] = elem;
*nb_ptr = nb;
+
+ return 0;
}
#define ISLEAP(y) (((y) % 4 == 0) && (((y) % 100) != 0 || ((y) % 400) == 0))
diff --git a/libavformat/hls.c b/libavformat/hls.c
index ea16f8a..2d56d80 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -161,7 +161,8 @@ static struct variant *new_variant(HLSContext *c, int
bandwidth,
reset_packet(&var->pkt);
var->bandwidth = bandwidth;
ff_make_absolute_url(var->url, sizeof(var->url), base, url);
- dynarray_add(&c->variants, &c->n_variants, var);
+ if (ff_dynarray_add(&c->variants, &c->n_variants, var) < 0)
+ return NULL;
return var;
}
@@ -287,6 +288,7 @@ static int parse_playlist(HLSContext *c, const char *url,
}
if (is_segment) {
struct segment *seg;
+ int err;
if (!var) {
var = new_variant(c, 0, url, NULL);
if (!var) {
@@ -310,7 +312,8 @@ static int parse_playlist(HLSContext *c, const char *url,
}
ff_make_absolute_url(seg->key, sizeof(seg->key), url, key);
ff_make_absolute_url(seg->url, sizeof(seg->url), url, line);
- dynarray_add(&var->segments, &var->n_segments, seg);
+ if ((err = ff_dynarray_add(&var->segments, &var->n_segments,
seg)) < 0)
+ return err;
is_segment = 0;
}
}
diff --git a/libavformat/hlsproto.c b/libavformat/hlsproto.c
index ec357de..7bb6ca0 100644
--- a/libavformat/hlsproto.c
+++ b/libavformat/hlsproto.c
@@ -146,6 +146,7 @@ static int parse_playlist(URLContext *h, const char *url)
} else if (av_strstart(line, "#", NULL)) {
continue;
} else if (line[0]) {
+ int err;
if (is_segment) {
struct segment *seg = av_malloc(sizeof(struct segment));
if (!seg) {
@@ -154,7 +155,10 @@ static int parse_playlist(URLContext *h, const char *url)
}
seg->duration = duration;
ff_make_absolute_url(seg->url, sizeof(seg->url), url, line);
- dynarray_add(&s->segments, &s->n_segments, seg);
+ if ((s->segments = av_realloc_array(s->segments, s->n_segments,
+ sizeof(*s->segments))) < 0)
+
+ return AVERROR(ENOMEM);
is_segment = 0;
This seems broken - it only reallocs an array but doesn't actually add the
element.
} else if (is_variant) {
struct variant *var = av_malloc(sizeof(struct variant));
@@ -164,7 +168,8 @@ static int parse_playlist(URLContext *h, const char *url)
}
var->bandwidth = bandwidth;
ff_make_absolute_url(var->url, sizeof(var->url), url, line);
- dynarray_add(&s->variants, &s->n_variants, var);
+ if ((err = ff_dynarray_add(&s->variants, &s->n_variants, var))
< 0)
+ return err;
is_variant = 0;
}
}
diff --git a/libavformat/internal.h b/libavformat/internal.h
index 1bc3e51..2ce9374 100644
--- a/libavformat/internal.h
+++ b/libavformat/internal.h
@@ -42,22 +42,7 @@ typedef struct CodecMime{
enum AVCodecID id;
} CodecMime;
-void ff_dynarray_add(intptr_t **tab_ptr, int *nb_ptr, intptr_t elem);
-
-#ifdef __GNUC__
-#define dynarray_add(tab, nb_ptr, elem)\
-do {\
- __typeof__(tab) _tab = (tab);\
- __typeof__(elem) _elem = (elem);\
- (void)sizeof(**_tab == _elem); /* check that types are compatible */\
- ff_dynarray_add((intptr_t **)_tab, nb_ptr, (intptr_t)_elem);\
-} while(0)
-#else
-#define dynarray_add(tab, nb_ptr, elem)\
-do {\
- ff_dynarray_add((intptr_t **)(tab), nb_ptr, (intptr_t)(elem));\
-} while(0)
-#endif
+int ff_dynarray_add(intptr_t **tab_ptr, int *nb_ptr, intptr_t elem);
struct tm *ff_brktimegm(time_t secs, struct tm *tm);
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index e6c9adf..85efc8e 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1966,6 +1966,7 @@ static int matroska_parse_rm_audio(MatroskaDemuxContext
*matroska,
}
while (track->audio.pkt_cnt) {
+ int err;
AVPacket *pkt = av_mallocz(sizeof(AVPacket));
av_new_packet(pkt, a);
memcpy(pkt->data, track->audio.buf
@@ -1974,7 +1975,8 @@ static int matroska_parse_rm_audio(MatroskaDemuxContext
*matroska,
track->audio.buf_timecode = AV_NOPTS_VALUE;
pkt->pos = pos;
pkt->stream_index = st->index;
- dynarray_add(&matroska->packets,&matroska->num_packets,pkt);
+ if ((err = ff_dynarray_add(&matroska->packets, &matroska->num_packets,
pkt)) < 0)
+ return err;
}
And this is the first example that shows that the current design is
flawed.
In this case, each element that is added is an AVPacket*, but each
AVPacket* also contains allocated data in the pkt->data pointer. If the
dynarray_add failed and you try to free all the elements, you will leak
all the pkt->data allocations.
I did not read further since the way the error handling design in
this patch is flawed.
// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
