| Message ID | 20220119134040.774-3-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] lavu/hwcontext_opencl: clear dangling pointers on map failure | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
On 1/19/2022 10:40 AM, Anton Khirnov wrote: > Clear anything that av_hwframe_map() might have done to the destination > frame, but leave caller-provided fields unchanged. > --- > libavutil/hwcontext.c | 23 +++++++++++++++++++++-- > libavutil/hwcontext.h | 4 ++++ > 2 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c > index 31c7840dba..ae33da1262 100644 > --- a/libavutil/hwcontext.c > +++ b/libavutil/hwcontext.c > @@ -18,6 +18,7 @@ > > #include "config.h" > > +#include "avassert.h" > #include "buffer.h" > #include "common.h" > #include "hwcontext.h" > @@ -788,6 +789,8 @@ fail: > > int av_hwframe_map(AVFrame *dst, const AVFrame *src, int flags) > { > + AVBufferRef *orig_dst_frames = dst->hw_frames_ctx; > + enum AVPixelFormat orig_dst_fmt = dst->format; > AVHWFramesContext *src_frames, *dst_frames; > HWMapDescriptor *hwmap; > int ret; > @@ -825,7 +828,7 @@ int av_hwframe_map(AVFrame *dst, const AVFrame *src, int flags) > ret = src_frames->internal->hw_type->map_from(src_frames, > dst, src, flags); > if (ret != AVERROR(ENOSYS)) > - return ret; > + goto fail; What if ret is 0? Is the stuff in the fail label meant to run on success? > } > } > > @@ -837,11 +840,27 @@ int av_hwframe_map(AVFrame *dst, const AVFrame *src, int flags) > ret = dst_frames->internal->hw_type->map_to(dst_frames, > dst, src, flags); > if (ret != AVERROR(ENOSYS)) > - return ret; > + goto fail; > } > } > > return AVERROR(ENOSYS); > + > +fail: > + // if the caller provided dst frames context, it should be preserved > + // by this function > + av_assert0(orig_dst_frames == NULL || > + orig_dst_frames == dst->hw_frames_ctx); > + > + // preserve user-provided dst frame fields, but clean > + // anything we might have set > + dst->hw_frames_ctx = NULL; > + av_frame_unref(dst); > + > + dst->hw_frames_ctx = orig_dst_frames; > + dst->format = orig_dst_fmt; > + > + return ret; > } > > int av_hwframe_ctx_create_derived(AVBufferRef **derived_frame_ctx, > diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h > index 04d19d89c2..c18b7e1e8b 100644 > --- a/libavutil/hwcontext.h > +++ b/libavutil/hwcontext.h > @@ -571,6 +571,10 @@ enum { > * possible with the given arguments and hwframe setup, while other return > * values indicate that it failed somehow. > * > + * On failure, the destination frame will be left blank, except for the > + * hw_frames_ctx/format fields thay may have been set by the caller - those will > + * be preserved as they were. > + * > * @param dst Destination frame, to contain the mapping. > * @param src Source frame, to be mapped. > * @param flags Some combination of AV_HWFRAME_MAP_* flags.
Quoting James Almer (2022-01-19 14:45:12) > > > On 1/19/2022 10:40 AM, Anton Khirnov wrote: > > Clear anything that av_hwframe_map() might have done to the destination > > frame, but leave caller-provided fields unchanged. > > --- > > libavutil/hwcontext.c | 23 +++++++++++++++++++++-- > > libavutil/hwcontext.h | 4 ++++ > > 2 files changed, 25 insertions(+), 2 deletions(-) > > > > diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c > > index 31c7840dba..ae33da1262 100644 > > --- a/libavutil/hwcontext.c > > +++ b/libavutil/hwcontext.c > > @@ -18,6 +18,7 @@ > > > > #include "config.h" > > > > +#include "avassert.h" > > #include "buffer.h" > > #include "common.h" > > #include "hwcontext.h" > > @@ -788,6 +789,8 @@ fail: > > > > int av_hwframe_map(AVFrame *dst, const AVFrame *src, int flags) > > { > > + AVBufferRef *orig_dst_frames = dst->hw_frames_ctx; > > + enum AVPixelFormat orig_dst_fmt = dst->format; > > AVHWFramesContext *src_frames, *dst_frames; > > HWMapDescriptor *hwmap; > > int ret; > > @@ -825,7 +828,7 @@ int av_hwframe_map(AVFrame *dst, const AVFrame *src, int flags) > > ret = src_frames->internal->hw_type->map_from(src_frames, > > dst, src, flags); > > if (ret != AVERROR(ENOSYS)) > > - return ret; > > + goto fail; > > What if ret is 0? Is the stuff in the fail label meant to run on success? Critical brain failure. Will fix.
diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c index 31c7840dba..ae33da1262 100644 --- a/libavutil/hwcontext.c +++ b/libavutil/hwcontext.c @@ -18,6 +18,7 @@ #include "config.h" +#include "avassert.h" #include "buffer.h" #include "common.h" #include "hwcontext.h" @@ -788,6 +789,8 @@ fail: int av_hwframe_map(AVFrame *dst, const AVFrame *src, int flags) { + AVBufferRef *orig_dst_frames = dst->hw_frames_ctx; + enum AVPixelFormat orig_dst_fmt = dst->format; AVHWFramesContext *src_frames, *dst_frames; HWMapDescriptor *hwmap; int ret; @@ -825,7 +828,7 @@ int av_hwframe_map(AVFrame *dst, const AVFrame *src, int flags) ret = src_frames->internal->hw_type->map_from(src_frames, dst, src, flags); if (ret != AVERROR(ENOSYS)) - return ret; + goto fail; } } @@ -837,11 +840,27 @@ int av_hwframe_map(AVFrame *dst, const AVFrame *src, int flags) ret = dst_frames->internal->hw_type->map_to(dst_frames, dst, src, flags); if (ret != AVERROR(ENOSYS)) - return ret; + goto fail; } } return AVERROR(ENOSYS); + +fail: + // if the caller provided dst frames context, it should be preserved + // by this function + av_assert0(orig_dst_frames == NULL || + orig_dst_frames == dst->hw_frames_ctx); + + // preserve user-provided dst frame fields, but clean + // anything we might have set + dst->hw_frames_ctx = NULL; + av_frame_unref(dst); + + dst->hw_frames_ctx = orig_dst_frames; + dst->format = orig_dst_fmt; + + return ret; } int av_hwframe_ctx_create_derived(AVBufferRef **derived_frame_ctx, diff --git a/libavutil/hwcontext.h b/libavutil/hwcontext.h index 04d19d89c2..c18b7e1e8b 100644 --- a/libavutil/hwcontext.h +++ b/libavutil/hwcontext.h @@ -571,6 +571,10 @@ enum { * possible with the given arguments and hwframe setup, while other return * values indicate that it failed somehow. * + * On failure, the destination frame will be left blank, except for the + * hw_frames_ctx/format fields thay may have been set by the caller - those will + * be preserved as they were. + * * @param dst Destination frame, to contain the mapping. * @param src Source frame, to be mapped. * @param flags Some combination of AV_HWFRAME_MAP_* flags.