| Message ID | 20200327022514.27554-2-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,v2,1/2] avcodec/avpacket: Always treat dst in av_packet_ref as uninitialized | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Quoting Andreas Rheinhardt (2020-03-27 03:25:14) > Up until now, it was completely unspecified what the content of the > destination packet dst was on error. Depending upon where the error > happened calling av_packet_unref() on dst might be dangerous. > > This commit changes this by making sure that dst is blank on error, so > unreferencing it again is safe (and still pointless). This behaviour is > documented. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > doc/APIchanges | 4 ++++ > libavcodec/avcodec.h | 3 ++- > libavcodec/avpacket.c | 7 ++++--- > libavcodec/version.h | 2 +- > 4 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index 8eeaec2028..f2bb2d242b 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -15,6 +15,10 @@ libavutil: 2017-10-21 > > API changes, most recent first: > > +2020-03-27 - xxxxxxxxxx - lavc 58.77.100 - avcodec.h > + av_packet_ref() now guarantees to return the destination packet > + in a blank state on error. > + > 2020-03-10 - xxxxxxxxxx - lavc 58.75.100 - avcodec.h > Add AV_PKT_DATA_ICC_PROFILE. > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > index f918d20a61..8fc0ad92c9 100644 > --- a/libavcodec/avcodec.h > +++ b/libavcodec/avcodec.h > @@ -4651,7 +4651,8 @@ void av_packet_free_side_data(AVPacket *pkt); > * @param dst Destination packet. Will be completely overwritten. > * @param src Source packet > * > - * @return 0 on success, a negative AVERROR on error. > + * @return 0 on success, a negative AVERROR on error. On error, dst > + * will be blank (as if returned by av_packet_alloc()). > */ > int av_packet_ref(AVPacket *dst, const AVPacket *src); > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > index 132567bc2d..c622718a45 100644 > --- a/libavcodec/avpacket.c > +++ b/libavcodec/avpacket.c > @@ -610,12 +610,13 @@ int av_packet_ref(AVPacket *dst, const AVPacket *src) > { > int ret; > > + dst->buf = NULL; > + I really think av_init_packet() would be more robust against future changes. But I'm not going to push for that especially strongly, so feel free to push whichever version you prefer.
Anton Khirnov: > Quoting Andreas Rheinhardt (2020-03-27 03:25:14) >> Up until now, it was completely unspecified what the content of the >> destination packet dst was on error. Depending upon where the error >> happened calling av_packet_unref() on dst might be dangerous. >> >> This commit changes this by making sure that dst is blank on error, so >> unreferencing it again is safe (and still pointless). This behaviour is >> documented. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> doc/APIchanges | 4 ++++ >> libavcodec/avcodec.h | 3 ++- >> libavcodec/avpacket.c | 7 ++++--- >> libavcodec/version.h | 2 +- >> 4 files changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/doc/APIchanges b/doc/APIchanges >> index 8eeaec2028..f2bb2d242b 100644 >> --- a/doc/APIchanges >> +++ b/doc/APIchanges >> @@ -15,6 +15,10 @@ libavutil: 2017-10-21 >> >> API changes, most recent first: >> >> +2020-03-27 - xxxxxxxxxx - lavc 58.77.100 - avcodec.h >> + av_packet_ref() now guarantees to return the destination packet >> + in a blank state on error. >> + >> 2020-03-10 - xxxxxxxxxx - lavc 58.75.100 - avcodec.h >> Add AV_PKT_DATA_ICC_PROFILE. >> >> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h >> index f918d20a61..8fc0ad92c9 100644 >> --- a/libavcodec/avcodec.h >> +++ b/libavcodec/avcodec.h >> @@ -4651,7 +4651,8 @@ void av_packet_free_side_data(AVPacket *pkt); >> * @param dst Destination packet. Will be completely overwritten. >> * @param src Source packet >> * >> - * @return 0 on success, a negative AVERROR on error. >> + * @return 0 on success, a negative AVERROR on error. On error, dst >> + * will be blank (as if returned by av_packet_alloc()). >> */ >> int av_packet_ref(AVPacket *dst, const AVPacket *src); >> >> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >> index 132567bc2d..c622718a45 100644 >> --- a/libavcodec/avpacket.c >> +++ b/libavcodec/avpacket.c >> @@ -610,12 +610,13 @@ int av_packet_ref(AVPacket *dst, const AVPacket *src) >> { >> int ret; >> >> + dst->buf = NULL; >> + > > I really think av_init_packet() would be more robust against future > changes. But I'm not going to push for that especially strongly, so feel > free to push whichever version you prefer. > Applied as is. After all, avpacket.c would be checked thoroughly in case of a future addition to AVPacket with something that needs to be allocated and freed (whereas lots of other places might be forgotten). - Andreas
diff --git a/doc/APIchanges b/doc/APIchanges index 8eeaec2028..f2bb2d242b 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,10 @@ libavutil: 2017-10-21 API changes, most recent first: +2020-03-27 - xxxxxxxxxx - lavc 58.77.100 - avcodec.h + av_packet_ref() now guarantees to return the destination packet + in a blank state on error. + 2020-03-10 - xxxxxxxxxx - lavc 58.75.100 - avcodec.h Add AV_PKT_DATA_ICC_PROFILE. diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index f918d20a61..8fc0ad92c9 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -4651,7 +4651,8 @@ void av_packet_free_side_data(AVPacket *pkt); * @param dst Destination packet. Will be completely overwritten. * @param src Source packet * - * @return 0 on success, a negative AVERROR on error. + * @return 0 on success, a negative AVERROR on error. On error, dst + * will be blank (as if returned by av_packet_alloc()). */ int av_packet_ref(AVPacket *dst, const AVPacket *src); diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index 132567bc2d..c622718a45 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -610,12 +610,13 @@ int av_packet_ref(AVPacket *dst, const AVPacket *src) { int ret; + dst->buf = NULL; + ret = av_packet_copy_props(dst, src); if (ret < 0) - return ret; + goto fail; if (!src->buf) { - dst->buf = NULL; ret = packet_alloc(&dst->buf, src->size); if (ret < 0) goto fail; @@ -637,7 +638,7 @@ int av_packet_ref(AVPacket *dst, const AVPacket *src) return 0; fail: - av_packet_free_side_data(dst); + av_packet_unref(dst); return ret; } diff --git a/libavcodec/version.h b/libavcodec/version.h index 1f19b67adc..7e01d9526f 100644 --- a/libavcodec/version.h +++ b/libavcodec/version.h @@ -28,7 +28,7 @@ #include "libavutil/version.h" #define LIBAVCODEC_VERSION_MAJOR 58 -#define LIBAVCODEC_VERSION_MINOR 76 +#define LIBAVCODEC_VERSION_MINOR 77 #define LIBAVCODEC_VERSION_MICRO 100 #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
Up until now, it was completely unspecified what the content of the destination packet dst was on error. Depending upon where the error happened calling av_packet_unref() on dst might be dangerous. This commit changes this by making sure that dst is blank on error, so unreferencing it again is safe (and still pointless). This behaviour is documented. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- doc/APIchanges | 4 ++++ libavcodec/avcodec.h | 3 ++- libavcodec/avpacket.c | 7 ++++--- libavcodec/version.h | 2 +- 4 files changed, 11 insertions(+), 5 deletions(-)