[FFmpeg-devel] [PATCH 2/2] avutil/executor: Fix stack overflow due to recursive call

[FFmpeg-devel] [PATCH 2/2] avutil/executor: Fix stack overflow due to recursive call

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

av_executor_execute run the task directly when thread is disabled.
The task can schedule a new task by call av_executor_execute. This
forms an implicit recursive call. This patch removed the recursive
call.
---
 libavutil/executor.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libavutil/executor.c b/libavutil/executor.c
index 89058fab2f..c145c51be9 100644
--- a/libavutil/executor.c
+++ b/libavutil/executor.c
@@ -58,6 +58,7 @@ struct AVExecutor {
     int die;
 
     AVTask *tasks;
+    int stack_depth;
 };
 
 static AVTask* remove_task(AVTask **prev, AVTask *t)
@@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, AVTask *t)
     }
 
     if (!e->thread_count || !HAVE_THREADS) {
+        if (e->stack_depth > 0)
+            return;
+        e->stack_depth++;
         // We are running in a single-threaded environment, so we must handle all tasks ourselves
         while (run_one_task(e, e->local_contexts))
             /* nothing */;
+        e->stack_depth--;
     }
 }
-- 
2.42.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 2/2] avutil/executor: Fix stack overflow due to recursive call

[Timo Rothenpieler]

On 08.07.2024 09:43, Zhao Zhili wrote:
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> av_executor_execute run the task directly when thread is disabled.
> The task can schedule a new task by call av_executor_execute. This
> forms an implicit recursive call. This patch removed the recursive
> call.
> ---
>   libavutil/executor.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/libavutil/executor.c b/libavutil/executor.c
> index 89058fab2f..c145c51be9 100644
> --- a/libavutil/executor.c
> +++ b/libavutil/executor.c
> @@ -58,6 +58,7 @@ struct AVExecutor {
>       int die;
>   
>       AVTask *tasks;
> +    int stack_depth;
>   };
>   
>   static AVTask* remove_task(AVTask **prev, AVTask *t)
> @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, AVTask *t)
>       }
>   
>       if (!e->thread_count || !HAVE_THREADS) {
> +        if (e->stack_depth > 0)
> +            return;
> +        e->stack_depth++;
>           // We are running in a single-threaded environment, so we must handle all tasks ourselves
>           while (run_one_task(e, e->local_contexts))
>               /* nothing */;
> +        e->stack_depth--;

Won't this put the above line into the "nothing" while-loop?
Is that intended? If so, the indentation should be adjusted accordingly.
If not, the while-loop should gain empty {}.

>       }
>   }
_______________________________________________
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 2/2] avutil/executor: Fix stack overflow due to recursive call

[Zhao Zhili]

> On Jul 8, 2024, at 22:07, Timo Rothenpieler <timo@rothenpieler.org> wrote:
> 
> On 08.07.2024 09:43, Zhao Zhili wrote:
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> av_executor_execute run the task directly when thread is disabled.
>> The task can schedule a new task by call av_executor_execute. This
>> forms an implicit recursive call. This patch removed the recursive
>> call.
>> ---
>>  libavutil/executor.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>> diff --git a/libavutil/executor.c b/libavutil/executor.c
>> index 89058fab2f..c145c51be9 100644
>> --- a/libavutil/executor.c
>> +++ b/libavutil/executor.c
>> @@ -58,6 +58,7 @@ struct AVExecutor {
>>      int die;
>>        AVTask *tasks;
>> +    int stack_depth;
>>  };
>>    static AVTask* remove_task(AVTask **prev, AVTask *t)
>> @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, AVTask *t)
>>      }
>>        if (!e->thread_count || !HAVE_THREADS) {
>> +        if (e->stack_depth > 0)
>> +            return;
>> +        e->stack_depth++;
>>          // We are running in a single-threaded environment, so we must handle all tasks ourselves
>>          while (run_one_task(e, e->local_contexts))
>>              /* nothing */;
>> +        e->stack_depth--;
> 
> Won't this put the above line into the "nothing" while-loop?
> Is that intended? If so, the indentation should be adjusted accordingly.
> If not, the while-loop should gain empty {}.

The comment specify it’s a while loop with empty body. Maybe it’s not obvious
in the email client.

> 
>>      }
>>  }
> _______________________________________________
> 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".

Re: [FFmpeg-devel] [PATCH 2/2] avutil/executor: Fix stack overflow due to recursive call

[Timo Rothenpieler]

On 08.07.2024 17:32, Zhao Zhili wrote:
> 
> 
>> On Jul 8, 2024, at 22:07, Timo Rothenpieler <timo@rothenpieler.org> wrote:
>>
>> On 08.07.2024 09:43, Zhao Zhili wrote:
>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>> av_executor_execute run the task directly when thread is disabled.
>>> The task can schedule a new task by call av_executor_execute. This
>>> forms an implicit recursive call. This patch removed the recursive
>>> call.
>>> ---
>>>   libavutil/executor.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>> diff --git a/libavutil/executor.c b/libavutil/executor.c
>>> index 89058fab2f..c145c51be9 100644
>>> --- a/libavutil/executor.c
>>> +++ b/libavutil/executor.c
>>> @@ -58,6 +58,7 @@ struct AVExecutor {
>>>       int die;
>>>         AVTask *tasks;
>>> +    int stack_depth;
>>>   };
>>>     static AVTask* remove_task(AVTask **prev, AVTask *t)
>>> @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, AVTask *t)
>>>       }
>>>         if (!e->thread_count || !HAVE_THREADS) {
>>> +        if (e->stack_depth > 0)
>>> +            return;
>>> +        e->stack_depth++;
>>>           // We are running in a single-threaded environment, so we must handle all tasks ourselves
>>>           while (run_one_task(e, e->local_contexts))
>>>               /* nothing */;
>>> +        e->stack_depth--;
>>
>> Won't this put the above line into the "nothing" while-loop?
>> Is that intended? If so, the indentation should be adjusted accordingly.
>> If not, the while-loop should gain empty {}.
> 
> The comment specify it’s a while loop with empty body. Maybe it’s not obvious
> in the email client.

Oh, there is a ; behind the comment.
Completely missed that. Can't say I like it, but yeah, that works.
_______________________________________________
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 2/2] avutil/executor: Fix stack overflow due to recursive call

[Nuo Mi]

On Tue, Jul 9, 2024 at 6:35 AM Timo Rothenpieler <timo@rothenpieler.org>
wrote:

> On 08.07.2024 17:32, Zhao Zhili wrote:
> >
> >
> >> On Jul 8, 2024, at 22:07, Timo Rothenpieler <timo@rothenpieler.org>
> wrote:
> >>
> >> On 08.07.2024 09:43, Zhao Zhili wrote:
> >>> From: Zhao Zhili <zhilizhao@tencent.com>
> >>> av_executor_execute run the task directly when thread is disabled.
> >>> The task can schedule a new task by call av_executor_execute. This
> >>> forms an implicit recursive call. This patch removed the recursive
> >>> call.
> >>> ---
> >>>   libavutil/executor.c | 5 +++++
> >>>   1 file changed, 5 insertions(+)
> >>> diff --git a/libavutil/executor.c b/libavutil/executor.c
> >>> index 89058fab2f..c145c51be9 100644
> >>> --- a/libavutil/executor.c
> >>> +++ b/libavutil/executor.c
> >>> @@ -58,6 +58,7 @@ struct AVExecutor {
> >>>       int die;
> >>>         AVTask *tasks;
> >>> +    int stack_depth;
> >>>   };
> >>>     static AVTask* remove_task(AVTask **prev, AVTask *t)
> >>> @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, AVTask *t)
> >>>       }
> >>>         if (!e->thread_count || !HAVE_THREADS) {
> >>> +        if (e->stack_depth > 0)
> >>> +            return;
> >>> +        e->stack_depth++;
> >>>           // We are running in a single-threaded environment, so we
> must handle all tasks ourselves
> >>>           while (run_one_task(e, e->local_contexts))
> >>>               /* nothing */;
> >>> +        e->stack_depth--;
> >>
> >> Won't this put the above line into the "nothing" while-loop?
> >> Is that intended? If so, the indentation should be adjusted accordingly.
> >> If not, the while-loop should gain empty {}.
> >
> > The comment specify it’s a while loop with empty body. Maybe it’s not
> obvious
> > in the email client.
>
> Oh, there is a ; behind the comment.
> Completely missed that. Can't say I like it, but yeah, that works.
>
Google suggests using "while (cond) continue;":
https://google.github.io/styleguide/cppguide.html#Formatting_Looping_Branching
.
However, it's not better than using /* nothing */.

> _______________________________________________
> 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".

Re: [FFmpeg-devel] [PATCH 2/2] avutil/executor: Fix stack overflow due to recursive call

[Anton Khirnov]

Quoting Nuo Mi (2024-07-09 13:36:09)
> On Tue, Jul 9, 2024 at 6:35 AM Timo Rothenpieler <timo@rothenpieler.org>
> wrote:
> 
> > On 08.07.2024 17:32, Zhao Zhili wrote:
> > >
> > >
> > >> On Jul 8, 2024, at 22:07, Timo Rothenpieler <timo@rothenpieler.org>
> > wrote:
> > >>
> > >> On 08.07.2024 09:43, Zhao Zhili wrote:
> > >>> From: Zhao Zhili <zhilizhao@tencent.com>
> > >>> av_executor_execute run the task directly when thread is disabled.
> > >>> The task can schedule a new task by call av_executor_execute. This
> > >>> forms an implicit recursive call. This patch removed the recursive
> > >>> call.
> > >>> ---
> > >>>   libavutil/executor.c | 5 +++++
> > >>>   1 file changed, 5 insertions(+)
> > >>> diff --git a/libavutil/executor.c b/libavutil/executor.c
> > >>> index 89058fab2f..c145c51be9 100644
> > >>> --- a/libavutil/executor.c
> > >>> +++ b/libavutil/executor.c
> > >>> @@ -58,6 +58,7 @@ struct AVExecutor {
> > >>>       int die;
> > >>>         AVTask *tasks;
> > >>> +    int stack_depth;
> > >>>   };
> > >>>     static AVTask* remove_task(AVTask **prev, AVTask *t)
> > >>> @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, AVTask *t)
> > >>>       }
> > >>>         if (!e->thread_count || !HAVE_THREADS) {
> > >>> +        if (e->stack_depth > 0)
> > >>> +            return;
> > >>> +        e->stack_depth++;
> > >>>           // We are running in a single-threaded environment, so we
> > must handle all tasks ourselves
> > >>>           while (run_one_task(e, e->local_contexts))
> > >>>               /* nothing */;
> > >>> +        e->stack_depth--;
> > >>
> > >> Won't this put the above line into the "nothing" while-loop?
> > >> Is that intended? If so, the indentation should be adjusted accordingly.
> > >> If not, the while-loop should gain empty {}.
> > >
> > > The comment specify it’s a while loop with empty body. Maybe it’s not
> > obvious
> > > in the email client.
> >
> > Oh, there is a ; behind the comment.
> > Completely missed that. Can't say I like it, but yeah, that works.
> >
> Google suggests using "while (cond) continue;":
> https://google.github.io/styleguide/cppguide.html#Formatting_Looping_Branching
> .
> However, it's not better than using /* nothing */.

continue gets hilit by your editor.

Also, I'd put it on a separate line for extra visibility.

And bikesheds should be black with red stripes.

-- 
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".

Re: [FFmpeg-devel] [PATCH 2/2] avutil/executor: Fix stack overflow due to recursive call

[Nuo Mi]

On Wed, Jul 10, 2024 at 5:14 PM Anton Khirnov <anton@khirnov.net> wrote:

> Quoting Nuo Mi (2024-07-09 13:36:09)
> > On Tue, Jul 9, 2024 at 6:35 AM Timo Rothenpieler <timo@rothenpieler.org>
> > wrote:
> >
> > > On 08.07.2024 17:32, Zhao Zhili wrote:
> > > >
> > > >
> > > >> On Jul 8, 2024, at 22:07, Timo Rothenpieler <timo@rothenpieler.org>
> > > wrote:
> > > >>
> > > >> On 08.07.2024 09:43, Zhao Zhili wrote:
> > > >>> From: Zhao Zhili <zhilizhao@tencent.com>
> > > >>> av_executor_execute run the task directly when thread is disabled.
> > > >>> The task can schedule a new task by call av_executor_execute. This
> > > >>> forms an implicit recursive call. This patch removed the recursive
> > > >>> call.
> > > >>> ---
> > > >>>   libavutil/executor.c | 5 +++++
> > > >>>   1 file changed, 5 insertions(+)
> > > >>> diff --git a/libavutil/executor.c b/libavutil/executor.c
> > > >>> index 89058fab2f..c145c51be9 100644
> > > >>> --- a/libavutil/executor.c
> > > >>> +++ b/libavutil/executor.c
> > > >>> @@ -58,6 +58,7 @@ struct AVExecutor {
> > > >>>       int die;
> > > >>>         AVTask *tasks;
> > > >>> +    int stack_depth;
> > > >>>   };
> > > >>>     static AVTask* remove_task(AVTask **prev, AVTask *t)
> > > >>> @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e,
> AVTask *t)
> > > >>>       }
> > > >>>         if (!e->thread_count || !HAVE_THREADS) {
> > > >>> +        if (e->stack_depth > 0)
> > > >>> +            return;
> > > >>> +        e->stack_depth++;
> > > >>>           // We are running in a single-threaded environment, so we
> > > must handle all tasks ourselves
> > > >>>           while (run_one_task(e, e->local_contexts))
> > > >>>               /* nothing */;
> > > >>> +        e->stack_depth--;
> > > >>
> > > >> Won't this put the above line into the "nothing" while-loop?
> > > >> Is that intended? If so, the indentation should be adjusted
> accordingly.
> > > >> If not, the while-loop should gain empty {}.
> > > >
> > > > The comment specify it’s a while loop with empty body. Maybe it’s not
> > > obvious
> > > > in the email client.
> > >
> > > Oh, there is a ; behind the comment.
> > > Completely missed that. Can't say I like it, but yeah, that works.
> > >
> > Google suggests using "while (cond) continue;":
> >
> https://google.github.io/styleguide/cppguide.html#Formatting_Looping_Branching
> > .
> > However, it's not better than using /* nothing */.
>
> continue gets hilit by your editor.
>
> Also, I'd put it on a separate line for extra visibility.
>
> And bikesheds should be black with red stripes.
>
Editors will highlight the comment with green too, which will help protect
our eyes.

>
> --
> 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 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 2/2] avutil/executor: Fix stack overflow due to recursive call

[Nuo Mi]

On Thu, Jul 11, 2024 at 8:21 PM Nuo Mi <nuomi2021@gmail.com> wrote:

>
>
> On Wed, Jul 10, 2024 at 5:14 PM Anton Khirnov <anton@khirnov.net> wrote:
>
>> Quoting Nuo Mi (2024-07-09 13:36:09)
>> > On Tue, Jul 9, 2024 at 6:35 AM Timo Rothenpieler <timo@rothenpieler.org
>> >
>> > wrote:
>> >
>> > > On 08.07.2024 17:32, Zhao Zhili wrote:
>> > > >
>> > > >
>> > > >> On Jul 8, 2024, at 22:07, Timo Rothenpieler <timo@rothenpieler.org
>> >
>> > > wrote:
>> > > >>
>> > > >> On 08.07.2024 09:43, Zhao Zhili wrote:
>> > > >>> From: Zhao Zhili <zhilizhao@tencent.com>
>> > > >>> av_executor_execute run the task directly when thread is disabled.
>> > > >>> The task can schedule a new task by call av_executor_execute. This
>> > > >>> forms an implicit recursive call. This patch removed the recursive
>> > > >>> call.
>> > > >>> ---
>> > > >>>   libavutil/executor.c | 5 +++++
>> > > >>>   1 file changed, 5 insertions(+)
>> > > >>> diff --git a/libavutil/executor.c b/libavutil/executor.c
>> > > >>> index 89058fab2f..c145c51be9 100644
>> > > >>> --- a/libavutil/executor.c
>> > > >>> +++ b/libavutil/executor.c
>> > > >>> @@ -58,6 +58,7 @@ struct AVExecutor {
>> > > >>>       int die;
>> > > >>>         AVTask *tasks;
>> > > >>> +    int stack_depth;
>> > > >>>   };
>> > > >>>     static AVTask* remove_task(AVTask **prev, AVTask *t)
>> > > >>> @@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e,
>> AVTask *t)
>> > > >>>       }
>> > > >>>         if (!e->thread_count || !HAVE_THREADS) {
>> > > >>> +        if (e->stack_depth > 0)
>> > > >>> +            return;
>> > > >>> +        e->stack_depth++;
>> > > >>>           // We are running in a single-threaded environment, so
>> we
>> > > must handle all tasks ourselves
>> > > >>>           while (run_one_task(e, e->local_contexts))
>> > > >>>               /* nothing */;
>> > > >>> +        e->stack_depth--;
>> > > >>
>> > > >> Won't this put the above line into the "nothing" while-loop?
>> > > >> Is that intended? If so, the indentation should be adjusted
>> accordingly.
>> > > >> If not, the while-loop should gain empty {}.
>> > > >
>> > > > The comment specify it’s a while loop with empty body. Maybe it’s
>> not
>> > > obvious
>> > > > in the email client.
>> > >
>> > > Oh, there is a ; behind the comment.
>> > > Completely missed that. Can't say I like it, but yeah, that works.
>> > >
>> > Google suggests using "while (cond) continue;":
>> >
>> https://google.github.io/styleguide/cppguide.html#Formatting_Looping_Branching
>> > .
>> > However, it's not better than using /* nothing */.
>>
>> continue gets hilit by your editor.
>>
>> Also, I'd put it on a separate line for extra visibility.
>>
>> And bikesheds should be black with red stripes.
>>
> Editors will highlight the comment with green too, which will help protect
> our eyes.
>
Applied.
Thank you, for the patch and review.


>> --
>> 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 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/2] avutil/executor: Fix stack overflow due to recursive call

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

av_executor_execute run the task directly when thread is disabled.
The task can schedule a new task by call av_executor_execute. This
forms an implicit recursive call. This patch removed the recursive
call.
---
 libavutil/executor.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libavutil/executor.c b/libavutil/executor.c
index 89058fab2f..c145c51be9 100644
--- a/libavutil/executor.c
+++ b/libavutil/executor.c
@@ -58,6 +58,7 @@ struct AVExecutor {
     int die;
 
     AVTask *tasks;
+    int stack_depth;
 };
 
 static AVTask* remove_task(AVTask **prev, AVTask *t)
@@ -207,8 +208,12 @@ void av_executor_execute(AVExecutor *e, AVTask *t)
     }
 
     if (!e->thread_count || !HAVE_THREADS) {
+        if (e->stack_depth > 0)
+            return;
+        e->stack_depth++;
         // We are running in a single-threaded environment, so we must handle all tasks ourselves
         while (run_one_task(e, e->local_contexts))
             /* nothing */;
+        e->stack_depth--;
     }
 }
-- 
2.42.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".