On Wed, Apr 13, 2011 at 11:19:26AM -0400, Ronald S. Bultje wrote: > Hi, > > On Wed, Apr 13, 2011 at 11:14 AM, Kostya <[email protected]> wrote: > > On Wed, Apr 13, 2011 at 11:03:26AM -0400, Ronald S. Bultje wrote: > >> On Wed, Apr 13, 2011 at 10:33 AM, Kostya <[email protected]> wrote: > >> > Here are improved patches. > >> [..] > >> > + * Additional packet data that can be provided by the container. > >> > + * Packet can contain several types of side information. > >> > + */ > >> > + struct { > >> > + uint8_t *data; > >> > + int size; > >> > + int type; > >> > >> enum AVSideDataType. Also the palette-type is missing here (you > >> probably put that in the second patch. I'd put it here just so it's > >> clear from the patch what this is to be used for). > > > > moved > > > >> [..] > >> > +#define DUP_DATA(dst, size, tmp, padding) \ > >> [..] > >> > + tmp = av_malloc(size + FF_INPUT_BUFFER_PADDING_SIZE); \ > >> > + } else { \ > >> > + tmp = av_malloc(size); \ > >> > + } \ > >> > + if (!data) \ > >> > + return AVERROR(ENOMEM); \ > >> > >> s/data/tmp/? > >> > >> Also the declaration of tmp outside the macro is kinda ugly, why not > >> just declare it inside the macro as void *tmp? > > > > done > [..] > > +#define DUP_DATA(dst, size, padding) \ > > + do { \ > > + void *data; \ > > + if (padding) { \ > > + if ((unsigned)(size) > (unsigned)(size) + > > FF_INPUT_BUFFER_PADDING_SIZE) \ > > + return AVERROR(ENOMEM); \ > > + data = av_malloc(size + FF_INPUT_BUFFER_PADDING_SIZE); \ > > + } else { \ > > + data = av_malloc(size); \ > > + } \ > > + if (!data) \ > > + return AVERROR(ENOMEM); \ > > + memcpy(data, dst, size); \ > > + if (padding) \ > > + memset((uint8_t*)data + size, 0, > > FF_INPUT_BUFFER_PADDING_SIZE); \ > > + dst = data; \ > > + } while(0) > > + > > int av_dup_packet(AVPacket *pkt) > > { > > if (((pkt->destruct == av_destruct_packet_nofree) || (pkt->destruct == > > NULL)) && pkt->data) { > > - uint8_t *data; > > - /* We duplicate the packet and don't forget to add the padding > > again. */ > > - if((unsigned)pkt->size > (unsigned)pkt->size + > > FF_INPUT_BUFFER_PADDING_SIZE) > > - return AVERROR(ENOMEM); > > - data = av_malloc(pkt->size + FF_INPUT_BUFFER_PADDING_SIZE); > > - if (!data) { > > - return AVERROR(ENOMEM); > > - } > > - memcpy(data, pkt->data, pkt->size); > > - memset(data + pkt->size, 0, FF_INPUT_BUFFER_PADDING_SIZE); > > - pkt->data = data; > > + DUP_DATA(pkt->data, pkt->size, 1); > > pkt->destruct = av_destruct_packet; > > + > > + if (pkt->side_data_elems) { > > + int i; > > + > > + DUP_DATA(pkt->side_data, pkt->side_data_elems * > > sizeof(*pkt->side_data), 0); > > + for (i = 0; i < pkt->side_data_elems; i++) { > > + DUP_DATA(pkt->side_data[i].data, pkt->side_data[i].size, > > 1); > > + } > > + } > > } > > So if any of these malloc()s fails, who is responsible for cleaning up > the mess? I assume that av_packet_free() is supposed to do the right > thing, right?
don't know > Currently, if the first malloc() fails, then the side-data is still > pointing to the original packet's side-data and will free it, thus > doing horrible stuff. Same, if the side-data or any of the chunks > inside it malloc() fails, the side-data still points to the old > side-data or chunks and will free() it, thus leading to horrible > outcomes. I thought it's no worse than current scheme - what happens to duplicate data pointer if it's not reallocated. Will it be double-freed or just ignored? Anyway, here's the cautious av_dup_packet(), feel free to squash into first patch. > Ronald > > Ronald
>From 27c435ae3cacdcec194c48d111cd7fcf1dd0f2fc Mon Sep 17 00:00:00 2001 From: Kostya Shishkov <[email protected]> Date: Wed, 13 Apr 2011 17:36:02 +0200 Subject: [PATCH 3/3] make av_dup_packet() more cautious on allocation failures --- libavcodec/avpacket.c | 26 +++++++++++++++++++------- 1 files changed, 19 insertions(+), 7 deletions(-) diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index 4b1002f..e0e4df4 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -100,19 +100,19 @@ int av_grow_packet(AVPacket *pkt, int grow_by) return 0; } -#define DUP_DATA(dst, size, padding) \ +#define DUP_DATA(dst, src, size, padding) \ do { \ void *data; \ if (padding) { \ if ((unsigned)(size) > (unsigned)(size) + FF_INPUT_BUFFER_PADDING_SIZE) \ - return AVERROR(ENOMEM); \ + goto failed_alloc; \ data = av_malloc(size + FF_INPUT_BUFFER_PADDING_SIZE); \ } else { \ data = av_malloc(size); \ } \ if (!data) \ - return AVERROR(ENOMEM); \ - memcpy(data, dst, size); \ + goto failed_alloc; \ + memcpy(data, src, size); \ if (padding) \ memset((uint8_t*)data + size, 0, FF_INPUT_BUFFER_PADDING_SIZE); \ dst = data; \ @@ -120,20 +120,32 @@ int av_grow_packet(AVPacket *pkt, int grow_by) int av_dup_packet(AVPacket *pkt) { + AVPacket tmp_pkt; + if (((pkt->destruct == av_destruct_packet_nofree) || (pkt->destruct == NULL)) && pkt->data) { - DUP_DATA(pkt->data, pkt->size, 1); + tmp_pkt = *pkt; + + pkt->data = NULL; + pkt->side_data = NULL; + DUP_DATA(pkt->data, tmp_pkt.data, pkt->size, 1); pkt->destruct = av_destruct_packet; if (pkt->side_data_elems) { int i; - DUP_DATA(pkt->side_data, pkt->side_data_elems * sizeof(*pkt->side_data), 0); + DUP_DATA(pkt->side_data, tmp_pkt.side_data, + pkt->side_data_elems * sizeof(*pkt->side_data), 0); + memset(pkt->side_data, 0, pkt->side_data_elems * sizeof(*pkt->side_data)); for (i = 0; i < pkt->side_data_elems; i++) { - DUP_DATA(pkt->side_data[i].data, pkt->side_data[i].size, 1); + DUP_DATA(pkt->side_data[i].data, tmp_pkt.side_data[i].data, + pkt->side_data[i].size, 1); } } } return 0; +failed_alloc: + av_destruct_packet(pkt); + return AVERROR(ENOMEM); } void av_free_packet(AVPacket *pkt) -- 1.7.0.4
_______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
