[FFmpeg-devel] [PATCH 2/5] avutil/hwcontext: Don't assume frames_uninit is reentrant

[FFmpeg-devel] [PATCH 2/5] avutil/hwcontext: Don't assume frames_uninit is reentrant

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

Fix heap use after free when vulkan_frames_init failed.
---
 libavutil/hwcontext.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/libavutil/hwcontext.c b/libavutil/hwcontext.c
index e8c6256a73..dec8b84783 100644
--- a/libavutil/hwcontext.c
+++ b/libavutil/hwcontext.c
@@ -360,7 +360,7 @@ int av_hwframe_ctx_init(AVBufferRef *ref)
     if (ctx->internal->hw_type->frames_init) {
         ret = ctx->internal->hw_type->frames_init(ctx);
         if (ret < 0)
-            goto fail;
+            return ret;
     }
 
     if (ctx->internal->pool_internal && !ctx->pool)
@@ -370,14 +370,10 @@ int av_hwframe_ctx_init(AVBufferRef *ref)
     if (ctx->initial_pool_size > 0) {
         ret = hwframe_pool_prealloc(ref);
         if (ret < 0)
-            goto fail;
+            return ret;
     }
 
     return 0;
-fail:
-    if (ctx->internal->hw_type->frames_uninit)
-        ctx->internal->hw_type->frames_uninit(ctx);
-    return ret;
 }
 
 int av_hwframe_transfer_get_formats(AVBufferRef *hwframe_ref,
-- 
2.34.1

_______________________________________________
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/5] avutil/hwcontext_vulkan: Fix leaks when semaphore creation fails

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

---
 libavutil/hwcontext_vulkan.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
index a84713e621..c64094f31c 100644
--- a/libavutil/hwcontext_vulkan.c
+++ b/libavutil/hwcontext_vulkan.c
@@ -1807,23 +1807,30 @@ static void vulkan_frame_free(AVHWFramesContext *hwfc, AVVkFrame *f)
     VulkanDevicePriv *p = hwfc->device_ctx->internal->priv;
     FFVulkanFunctions *vk = &p->vkctx.vkfn;
     int nb_images = ff_vk_count_images(f);
+    int nb_sems = 0;
+
+    while (nb_sems < FF_ARRAY_ELEMS(f->sem) && f->sem[nb_sems])
+        nb_sems++;
+
+    if (nb_sems) {
+        VkSemaphoreWaitInfo sem_wait = {
+            .sType          = VK_STRUCTURE_TYPE_SEMAPHORE_WAIT_INFO,
+            .flags          = 0x0,
+            .pSemaphores    = f->sem,
+            .pValues        = f->sem_value,
+            .semaphoreCount = nb_sems,
+        };
 
-    VkSemaphoreWaitInfo sem_wait = {
-        .sType          = VK_STRUCTURE_TYPE_SEMAPHORE_WAIT_INFO,
-        .flags          = 0x0,
-        .pSemaphores    = f->sem,
-        .pValues        = f->sem_value,
-        .semaphoreCount = nb_images,
-    };
-
-    vk->WaitSemaphores(hwctx->act_dev, &sem_wait, UINT64_MAX);
+        vk->WaitSemaphores(hwctx->act_dev, &sem_wait, UINT64_MAX);
+    }
 
     vulkan_free_internal(f);
 
     for (int i = 0; i < nb_images; i++) {
         vk->DestroyImage(hwctx->act_dev,     f->img[i], hwctx->alloc);
         vk->FreeMemory(hwctx->act_dev,       f->mem[i], hwctx->alloc);
-        vk->DestroySemaphore(hwctx->act_dev, f->sem[i], hwctx->alloc);
+        if (f->sem[i])
+            vk->DestroySemaphore(hwctx->act_dev, f->sem[i], hwctx->alloc);
     }
 
     av_free(f);
@@ -2098,7 +2105,8 @@ static int create_frame(AVHWFramesContext *hwfc, AVVkFrame **frame,
         if (ret != VK_SUCCESS) {
             av_log(hwctx, AV_LOG_ERROR, "Failed to create semaphore: %s\n",
                    ff_vk_ret2str(ret));
-            return AVERROR_EXTERNAL;
+            err = AVERROR_EXTERNAL;
+            goto fail;
         }
 
         f->queue_family[i] = p->nb_img_qfs > 1 ? VK_QUEUE_FAMILY_IGNORED : p->img_qfs[0];
-- 
2.34.1

_______________________________________________
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 4/5] avutil/hwcontext_vulkan: Fix leaks in map_from_drm

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

Also simplify error handing.
---
This patch is untested since I don't have a proper device.

 libavutil/hwcontext_vulkan.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
index c64094f31c..35a7d75cce 100644
--- a/libavutil/hwcontext_vulkan.c
+++ b/libavutil/hwcontext_vulkan.c
@@ -2633,7 +2633,8 @@ static int vulkan_map_from_drm_frame_desc(AVHWFramesContext *hwfc, AVVkFrame **f
         if (ret != VK_SUCCESS) {
             av_log(hwctx, AV_LOG_ERROR, "Failed to create semaphore: %s\n",
                    ff_vk_ret2str(ret));
-            return AVERROR_EXTERNAL;
+            err = AVERROR_EXTERNAL;
+            goto fail;
         }
 
         /* We'd import a semaphore onto the one we created using
@@ -2752,14 +2753,7 @@ static int vulkan_map_from_drm_frame_desc(AVHWFramesContext *hwfc, AVVkFrame **f
     return 0;
 
 fail:
-    for (int i = 0; i < desc->nb_layers; i++) {
-        vk->DestroyImage(hwctx->act_dev, f->img[i], hwctx->alloc);
-        vk->DestroySemaphore(hwctx->act_dev, f->sem[i], hwctx->alloc);
-    }
-    for (int i = 0; i < desc->nb_objects; i++)
-        vk->FreeMemory(hwctx->act_dev, f->mem[i], hwctx->alloc);
-
-    av_free(f);
+    vulkan_frame_free(hwfc, f);
 
     return err;
 }
-- 
2.34.1

_______________________________________________
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 5/5] avutil/vulkan: avoid overreads in ff_vk_count_images

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

---
 libavutil/vulkan.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavutil/vulkan.h b/libavutil/vulkan.h
index b666841836..a5e78760d7 100644
--- a/libavutil/vulkan.h
+++ b/libavutil/vulkan.h
@@ -271,7 +271,7 @@ typedef struct FFVulkanContext {
 static inline int ff_vk_count_images(AVVkFrame *f)
 {
     int cnt = 0;
-    while (f->img[cnt])
+    while (cnt < FF_ARRAY_ELEMS(f->img) && f->img[cnt])
         cnt++;
 
     return cnt;
-- 
2.34.1

_______________________________________________
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/5] avutil/hwcontext_vulkan: Fix leaks when semaphore creation fails

[Zhao Zhili]

I found serious memory leaks (dozens of frames depends on video filter) when test vulkan, which may or may not directly related to vulkan. See trac 10873.

> 在 2024年2月20日，下午8:08，Zhao Zhili <quinkblack@foxmail.com> 写道：
> 
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> ---
> libavutil/hwcontext_vulkan.c | 30 +++++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
> index a84713e621..c64094f31c 100644
> --- a/libavutil/hwcontext_vulkan.c
> +++ b/libavutil/hwcontext_vulkan.c
> @@ -1807,23 +1807,30 @@ static void vulkan_frame_free(AVHWFramesContext *hwfc, AVVkFrame *f)
>     VulkanDevicePriv *p = hwfc->device_ctx->internal->priv;
>     FFVulkanFunctions *vk = &p->vkctx.vkfn;
>     int nb_images = ff_vk_count_images(f);
> +    int nb_sems = 0;
> +
> +    while (nb_sems < FF_ARRAY_ELEMS(f->sem) && f->sem[nb_sems])
> +        nb_sems++;
> +
> +    if (nb_sems) {
> +        VkSemaphoreWaitInfo sem_wait = {
> +            .sType          = VK_STRUCTURE_TYPE_SEMAPHORE_WAIT_INFO,
> +            .flags          = 0x0,
> +            .pSemaphores    = f->sem,
> +            .pValues        = f->sem_value,
> +            .semaphoreCount = nb_sems,
> +        };
> 
> -    VkSemaphoreWaitInfo sem_wait = {
> -        .sType          = VK_STRUCTURE_TYPE_SEMAPHORE_WAIT_INFO,
> -        .flags          = 0x0,
> -        .pSemaphores    = f->sem,
> -        .pValues        = f->sem_value,
> -        .semaphoreCount = nb_images,
> -    };
> -
> -    vk->WaitSemaphores(hwctx->act_dev, &sem_wait, UINT64_MAX);
> +        vk->WaitSemaphores(hwctx->act_dev, &sem_wait, UINT64_MAX);
> +    }
> 
>     vulkan_free_internal(f);
> 
>     for (int i = 0; i < nb_images; i++) {
>         vk->DestroyImage(hwctx->act_dev,     f->img[i], hwctx->alloc);
>         vk->FreeMemory(hwctx->act_dev,       f->mem[i], hwctx->alloc);
> -        vk->DestroySemaphore(hwctx->act_dev, f->sem[i], hwctx->alloc);
> +        if (f->sem[i])
> +            vk->DestroySemaphore(hwctx->act_dev, f->sem[i], hwctx->alloc);
>     }
> 
>     av_free(f);
> @@ -2098,7 +2105,8 @@ static int create_frame(AVHWFramesContext *hwfc, AVVkFrame **frame,
>         if (ret != VK_SUCCESS) {
>             av_log(hwctx, AV_LOG_ERROR, "Failed to create semaphore: %s\n",
>                    ff_vk_ret2str(ret));
> -            return AVERROR_EXTERNAL;
> +            err = AVERROR_EXTERNAL;
> +            goto fail;
>         }
> 
>         f->queue_family[i] = p->nb_img_qfs > 1 ? VK_QUEUE_FAMILY_IGNORED : p->img_qfs[0];
> -- 
> 2.34.1
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email

_______________________________________________
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/5] avutil/hwcontext_vulkan: Fix leaks when semaphore creation fails

[Lynne]

Feb 20, 2024, 13:10 by quinkblack@foxmail.com:

> From: Zhao Zhili <zhilizhao@tencent.com>
>
> ---
>  libavutil/hwcontext_vulkan.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
> index a84713e621..c64094f31c 100644
> --- a/libavutil/hwcontext_vulkan.c
> +++ b/libavutil/hwcontext_vulkan.c
> @@ -1807,23 +1807,30 @@ static void vulkan_frame_free(AVHWFramesContext *hwfc, AVVkFrame *f)
>  VulkanDevicePriv *p = hwfc->device_ctx->internal->priv;
>  FFVulkanFunctions *vk = &p->vkctx.vkfn;
>  int nb_images = ff_vk_count_images(f);
> +    int nb_sems = 0;
> +
> +    while (nb_sems < FF_ARRAY_ELEMS(f->sem) && f->sem[nb_sems])
> +        nb_sems++;
> +
> +    if (nb_sems) {
> +        VkSemaphoreWaitInfo sem_wait = {
> +            .sType          = VK_STRUCTURE_TYPE_SEMAPHORE_WAIT_INFO,
> +            .flags          = 0x0,
> +            .pSemaphores    = f->sem,
> +            .pValues        = f->sem_value,
> +            .semaphoreCount = nb_sems,
> +        };
>  
> -    VkSemaphoreWaitInfo sem_wait = {
> -        .sType          = VK_STRUCTURE_TYPE_SEMAPHORE_WAIT_INFO,
> -        .flags          = 0x0,
> -        .pSemaphores    = f->sem,
> -        .pValues        = f->sem_value,
> -        .semaphoreCount = nb_images,
> -    };
> -
> -    vk->WaitSemaphores(hwctx->act_dev, &sem_wait, UINT64_MAX);
> +        vk->WaitSemaphores(hwctx->act_dev, &sem_wait, UINT64_MAX);
> +    }
>  
>  vulkan_free_internal(f);
>  
>  for (int i = 0; i < nb_images; i++) {
>  vk->DestroyImage(hwctx->act_dev,     f->img[i], hwctx->alloc);
>  vk->FreeMemory(hwctx->act_dev,       f->mem[i], hwctx->alloc);
> -        vk->DestroySemaphore(hwctx->act_dev, f->sem[i], hwctx->alloc);
> +        if (f->sem[i])
> +            vk->DestroySemaphore(hwctx->act_dev, f->sem[i], hwctx->alloc);
>

vkDestroySemaphore should already check for NULL, though?
_______________________________________________
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/5] avutil/hwcontext_vulkan: Fix leaks when semaphore creation fails

[Zhao Zhili]

On 2024/2/21 01:10, Lynne wrote:
> Feb 20, 2024, 13:10 by quinkblack@foxmail.com:
>
>> From: Zhao Zhili <zhilizhao@tencent.com>
>>
>> ---
>>   libavutil/hwcontext_vulkan.c | 30 +++++++++++++++++++-----------
>>   1 file changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
>> index a84713e621..c64094f31c 100644
>> --- a/libavutil/hwcontext_vulkan.c
>> +++ b/libavutil/hwcontext_vulkan.c
>> @@ -1807,23 +1807,30 @@ static void vulkan_frame_free(AVHWFramesContext *hwfc, AVVkFrame *f)
>>   VulkanDevicePriv *p = hwfc->device_ctx->internal->priv;
>>   FFVulkanFunctions *vk = &p->vkctx.vkfn;
>>   int nb_images = ff_vk_count_images(f);
>> +    int nb_sems = 0;
>> +
>> +    while (nb_sems < FF_ARRAY_ELEMS(f->sem) && f->sem[nb_sems])
>> +        nb_sems++;
>> +
>> +    if (nb_sems) {
>> +        VkSemaphoreWaitInfo sem_wait = {
>> +            .sType          = VK_STRUCTURE_TYPE_SEMAPHORE_WAIT_INFO,
>> +            .flags          = 0x0,
>> +            .pSemaphores    = f->sem,
>> +            .pValues        = f->sem_value,
>> +            .semaphoreCount = nb_sems,
>> +        };
>>   
>> -    VkSemaphoreWaitInfo sem_wait = {
>> -        .sType          = VK_STRUCTURE_TYPE_SEMAPHORE_WAIT_INFO,
>> -        .flags          = 0x0,
>> -        .pSemaphores    = f->sem,
>> -        .pValues        = f->sem_value,
>> -        .semaphoreCount = nb_images,
>> -    };
>> -
>> -    vk->WaitSemaphores(hwctx->act_dev, &sem_wait, UINT64_MAX);
>> +        vk->WaitSemaphores(hwctx->act_dev, &sem_wait, UINT64_MAX);
>> +    }
>>   
>>   vulkan_free_internal(f);
>>   
>>   for (int i = 0; i < nb_images; i++) {
>>   vk->DestroyImage(hwctx->act_dev,     f->img[i], hwctx->alloc);
>>   vk->FreeMemory(hwctx->act_dev,       f->mem[i], hwctx->alloc);
>> -        vk->DestroySemaphore(hwctx->act_dev, f->sem[i], hwctx->alloc);
>> +        if (f->sem[i])
>> +            vk->DestroySemaphore(hwctx->act_dev, f->sem[i], hwctx->alloc);
>>
> vkDestroySemaphore should already check for NULL, though?

Yes, the check before vkDestroySemaphore can be removed. Will update the 
patch.

> _______________________________________________
> 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 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 v2 3/5] avutil/hwcontext_vulkan: Fix leaks when semaphore creation fails

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

---
 libavutil/hwcontext_vulkan.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
index d660f63afe..ee13788ead 100644
--- a/libavutil/hwcontext_vulkan.c
+++ b/libavutil/hwcontext_vulkan.c
@@ -1807,16 +1807,22 @@ static void vulkan_frame_free(AVHWFramesContext *hwfc, AVVkFrame *f)
     VulkanDevicePriv *p = hwfc->device_ctx->internal->priv;
     FFVulkanFunctions *vk = &p->vkctx.vkfn;
     int nb_images = ff_vk_count_images(f);
+    int nb_sems = 0;
+
+    while (nb_sems < FF_ARRAY_ELEMS(f->sem) && f->sem[nb_sems])
+        nb_sems++;
+
+    if (nb_sems) {
+        VkSemaphoreWaitInfo sem_wait = {
+            .sType          = VK_STRUCTURE_TYPE_SEMAPHORE_WAIT_INFO,
+            .flags          = 0x0,
+            .pSemaphores    = f->sem,
+            .pValues        = f->sem_value,
+            .semaphoreCount = nb_sems,
+        };
 
-    VkSemaphoreWaitInfo sem_wait = {
-        .sType          = VK_STRUCTURE_TYPE_SEMAPHORE_WAIT_INFO,
-        .flags          = 0x0,
-        .pSemaphores    = f->sem,
-        .pValues        = f->sem_value,
-        .semaphoreCount = nb_images,
-    };
-
-    vk->WaitSemaphores(hwctx->act_dev, &sem_wait, UINT64_MAX);
+        vk->WaitSemaphores(hwctx->act_dev, &sem_wait, UINT64_MAX);
+    }
 
     vulkan_free_internal(f);
 
@@ -2098,7 +2104,8 @@ static int create_frame(AVHWFramesContext *hwfc, AVVkFrame **frame,
         if (ret != VK_SUCCESS) {
             av_log(hwctx, AV_LOG_ERROR, "Failed to create semaphore: %s\n",
                    ff_vk_ret2str(ret));
-            return AVERROR_EXTERNAL;
+            err = AVERROR_EXTERNAL;
+            goto fail;
         }
 
         f->queue_family[i] = p->nb_img_qfs > 1 ? VK_QUEUE_FAMILY_IGNORED : p->img_qfs[0];
-- 
2.34.1

_______________________________________________
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 v2 3/5] avutil/hwcontext_vulkan: Fix leaks when semaphore creation fails

[Lynne]

Feb 22, 2024, 15:43 by quinkblack@foxmail.com:

> From: Zhao Zhili <zhilizhao@tencent.com>
>
> ---
>  libavutil/hwcontext_vulkan.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>

LGTM on all Vulkan patches you posted.
_______________________________________________
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".