[FFmpeg-devel] [PATCH 1/3] lavu/hwcontext_opencl: clear dangling pointers on map failure

[FFmpeg-devel] [PATCH 1/3] lavu/hwcontext_opencl: clear dangling pointers on map failure

[Anton Khirnov]

---
untested
---
 libavutil/hwcontext_opencl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libavutil/hwcontext_opencl.c b/libavutil/hwcontext_opencl.c
index 26a3a24593..40ee611943 100644
--- a/libavutil/hwcontext_opencl.c
+++ b/libavutil/hwcontext_opencl.c
@@ -2146,6 +2146,7 @@ fail:
             clReleaseMemObject(mapping->frame.planes[p]);
     }
     av_free(mapping);
+    memset(dst->data, 0, sizeof(dst->data));
     return err;
 }
 
@@ -2317,6 +2318,7 @@ fail:
         if (desc->planes[p])
             clReleaseMemObject(desc->planes[p]);
     av_freep(&desc);
+    memset(dst->data, 0, sizeof(dst->data));
     return err;
 }
 
@@ -2407,6 +2409,7 @@ fail:
         0, NULL, &event);
     if (cle == CL_SUCCESS)
         opencl_wait_events(dst_fc, &event, 1);
+    memset(dst->data, 0, sizeof(dst->data));
     return err;
 }
 
@@ -2562,6 +2565,7 @@ fail:
         0, NULL, &event);
     if (cle == CL_SUCCESS)
         opencl_wait_events(dst_fc, &event, 1);
+    memset(dst->data, 0, sizeof(dst->data));
     return err;
 }
 
@@ -2793,6 +2797,7 @@ fail:
             clReleaseMemObject(mapping->object_buffers[i]);
     }
     av_free(mapping);
+    memset(dst->data, 0, sizeof(dst->data));
     return err;
 }
 
-- 
2.33.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/3] lavu/hwcontext_vulkan: clear dangling pointers on map failure

[Anton Khirnov]

---
untested
---
 libavutil/hwcontext_vulkan.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
index 60a6cf6a91..ae19fc2ab6 100644
--- a/libavutil/hwcontext_vulkan.c
+++ b/libavutil/hwcontext_vulkan.c
@@ -2927,6 +2927,7 @@ static int vulkan_map_from_drm(AVHWFramesContext *hwfc, AVFrame *dst,
 
 fail:
     vulkan_frame_free(hwfc->device_ctx->hwctx, (uint8_t *)f);
+    dst->data[0] = NULL;
     return err;
 }
 
-- 
2.33.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/3] lavu/hwcontext: clarify behavior on av_hwframe_map() failure

[Anton Khirnov]

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;
         }
     }
 
@@ -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.
-- 
2.33.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/3] lavu/hwcontext: clarify behavior on av_hwframe_map() failure

[James Almer]

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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/3] lavu/hwcontext: clarify behavior on av_hwframe_map() failure

[Anton Khirnov]

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.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH] lavu/hwcontext: clarify behavior on av_hwframe_map() failure

[Anton Khirnov]

Clear anything that av_hwframe_map() might have done to the destination
frame, but leave caller-provided fields unchanged.
---
Interdiff:
  diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
  index ae33da1262..ab9ad3703e 100644
  --- a/libavutil/hwcontext.c
  +++ b/libavutil/hwcontext.c
  @@ -827,7 +827,9 @@ int av_hwframe_map(AVFrame *dst, const AVFrame *src, int flags)
               src_frames->internal->hw_type->map_from) {
               ret = src_frames->internal->hw_type->map_from(src_frames,
                                                             dst, src, flags);
  -            if (ret != AVERROR(ENOSYS))
  +            if (ret >= 0)
  +                return ret;
  +            else if (ret != AVERROR(ENOSYS))
                   goto fail;
           }
       }
  @@ -839,7 +841,9 @@ int av_hwframe_map(AVFrame *dst, const AVFrame *src, int flags)
               dst_frames->internal->hw_type->map_to) {
               ret = dst_frames->internal->hw_type->map_to(dst_frames,
                                                           dst, src, flags);
  -            if (ret != AVERROR(ENOSYS))
  +            if (ret >= 0)
  +                return ret;
  +            else if (ret != AVERROR(ENOSYS))
                   goto fail;
           }
       }

 libavutil/hwcontext.c | 27 +++++++++++++++++++++++++--
 libavutil/hwcontext.h |  4 ++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
index 31c7840dba..ab9ad3703e 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;
@@ -824,8 +827,10 @@ int av_hwframe_map(AVFrame *dst, const AVFrame *src, int flags)
             src_frames->internal->hw_type->map_from) {
             ret = src_frames->internal->hw_type->map_from(src_frames,
                                                           dst, src, flags);
-            if (ret != AVERROR(ENOSYS))
+            if (ret >= 0)
                 return ret;
+            else if (ret != AVERROR(ENOSYS))
+                goto fail;
         }
     }
 
@@ -836,12 +841,30 @@ int av_hwframe_map(AVFrame *dst, const AVFrame *src, int flags)
             dst_frames->internal->hw_type->map_to) {
             ret = dst_frames->internal->hw_type->map_to(dst_frames,
                                                         dst, src, flags);
-            if (ret != AVERROR(ENOSYS))
+            if (ret >= 0)
                 return ret;
+            else if (ret != AVERROR(ENOSYS))
+                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.
-- 
2.33.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".