| Message ID | 20170921220438.8216-1-jamrial@gmail.com |
|---|---|
| State | Withdrawn |
| Headers | show |
On 9/21/2017 7:04 PM, James Almer wrote: > The scenario makes no sense and produces all kinds of memory leaks. > Return 0 instead of an error as the process is pretty much a nop. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/avpacket.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > index 5ce3228166..a68e2501ad 100644 > --- a/libavcodec/avpacket.c > +++ b/libavcodec/avpacket.c > @@ -225,14 +225,14 @@ failed_alloc: > > int av_copy_packet_side_data(AVPacket *pkt, const AVPacket *src) > { > + if (src == pkt) > + return 0; > + > if (src->side_data_elems) { > int i; > - DUP_DATA(pkt->side_data, src->side_data, > - src->side_data_elems * sizeof(*src->side_data), 0, ALLOC_MALLOC); > - if (src != pkt) { > - memset(pkt->side_data, 0, > - src->side_data_elems * sizeof(*src->side_data)); > - } > + pkt->side_data = av_mallocz_array(src->side_data_elems, sizeof(*src->side_data)); > + if (!pkt->side_data) > + goto failed_alloc; > for (i = 0; i < src->side_data_elems; i++) { > DUP_DATA(pkt->side_data[i].data, src->side_data[i].data, > src->side_data[i].size, 1, ALLOC_MALLOC); > Right, so there's one scenario for src == dst, but it's a hacky one. Basically, something like AVPacket tmp = *pkt; av_copy_packet_side_data(pkt, pkt); Which means there will be no leaks as long as tmp is properly freed afterguards. Changing that behavior will potentially break existing code using the function (commit fdd1aaf87aa hints that there are some uses of this kind), so seeing it's already inside relevant guards I'm going to effectively deprecate it in favor of the saner av_packet_copy_props().
diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index 5ce3228166..a68e2501ad 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -225,14 +225,14 @@ failed_alloc: int av_copy_packet_side_data(AVPacket *pkt, const AVPacket *src) { + if (src == pkt) + return 0; + if (src->side_data_elems) { int i; - DUP_DATA(pkt->side_data, src->side_data, - src->side_data_elems * sizeof(*src->side_data), 0, ALLOC_MALLOC); - if (src != pkt) { - memset(pkt->side_data, 0, - src->side_data_elems * sizeof(*src->side_data)); - } + pkt->side_data = av_mallocz_array(src->side_data_elems, sizeof(*src->side_data)); + if (!pkt->side_data) + goto failed_alloc; for (i = 0; i < src->side_data_elems; i++) { DUP_DATA(pkt->side_data[i].data, src->side_data[i].data, src->side_data[i].size, 1, ALLOC_MALLOC);
The scenario makes no sense and produces all kinds of memory leaks. Return 0 instead of an error as the process is pretty much a nop. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/avpacket.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)