[PATCH] baum: Fix crash when Braille output is not available

[PATCH] baum: Fix crash when Braille output is not available

[Samuel Thibault]

When Braille output is not available, the backend properly reports being
unable to be created, but 5f8e93c3e262 ("util/qemu-timer: Make timer_free()
imply timer_del()") made the timer_free() call now refuse any NULL
parameter. char_braille_finalize thus now has to be more careful with
calling it on baum->cellCount_timer.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 chardev/baum.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/chardev/baum.c b/chardev/baum.c
index 5deca778bc..aca5bf12fb 100644
--- a/chardev/baum.c
+++ b/chardev/baum.c
@@ -631,7 +631,9 @@ static void char_braille_finalize(Object *obj)
 {
     BaumChardev *baum = BAUM_CHARDEV(obj);
 
-    timer_free(baum->cellCount_timer);
+    if (baum->cellCount_timer) {
+        timer_free(baum->cellCount_timer);
+    }
     if (baum->brlapi) {
         brlapi__closeConnection(baum->brlapi);
         g_free(baum->brlapi);
-- 
2.30.1

Re: [PATCH] baum: Fix crash when Braille output is not available

[Peter Maydell]

On Wed, 10 Mar 2021 at 16:08, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
>
> When Braille output is not available, the backend properly reports being
> unable to be created, but 5f8e93c3e262 ("util/qemu-timer: Make timer_free()
> imply timer_del()") made the timer_free() call now refuse any NULL
> parameter. char_braille_finalize thus now has to be more careful with
> calling it on baum->cellCount_timer.

It wasn't the intention of that commit to make freeing a NULL
timer invalid; I think Paolo's patch restoring the ability to
timer_free(NULL) is probably the right thing here:
https://patchew.org/QEMU/20210310154526.463850-1-pbonzini@redhat.com/

thanks
-- PMM

Re: [PATCH] baum: Fix crash when Braille output is not available

[Samuel Thibault]

Peter Maydell, le mer. 10 mars 2021 17:18:11 +0000, a ecrit:
> On Wed, 10 Mar 2021 at 16:08, Samuel Thibault
> <samuel.thibault@ens-lyon.org> wrote:
> >
> > When Braille output is not available, the backend properly reports being
> > unable to be created, but 5f8e93c3e262 ("util/qemu-timer: Make timer_free()
> > imply timer_del()") made the timer_free() call now refuse any NULL
> > parameter. char_braille_finalize thus now has to be more careful with
> > calling it on baum->cellCount_timer.
> 
> It wasn't the intention of that commit to make freeing a NULL
> timer invalid;

Ok!

Samuel

Re: [PATCH] baum: Fix crash when Braille output is not available

[Thomas Huth]

On 10/03/2021 17.08, Samuel Thibault wrote:
> When Braille output is not available, the backend properly reports being
> unable to be created, but 5f8e93c3e262 ("util/qemu-timer: Make timer_free()
> imply timer_del()") made the timer_free() call now refuse any NULL
> parameter. char_braille_finalize thus now has to be more careful with
> calling it on baum->cellCount_timer.
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>   chardev/baum.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/chardev/baum.c b/chardev/baum.c
> index 5deca778bc..aca5bf12fb 100644
> --- a/chardev/baum.c
> +++ b/chardev/baum.c
> @@ -631,7 +631,9 @@ static void char_braille_finalize(Object *obj)
>   {
>       BaumChardev *baum = BAUM_CHARDEV(obj);
>   
> -    timer_free(baum->cellCount_timer);
> +    if (baum->cellCount_timer) {
> +        timer_free(baum->cellCount_timer);
> +    }
>       if (baum->brlapi) {
>           brlapi__closeConnection(baum->brlapi);
>           g_free(baum->brlapi);

I just tried to debug this problem, too, and came to the same conclusion.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH] baum: Fix crash when Braille output is not available

[Philippe Mathieu-Daudé]

Hi Samuel,

On 3/10/21 5:08 PM, Samuel Thibault wrote:
> When Braille output is not available, the backend properly reports being
> unable to be created, but 5f8e93c3e262 ("util/qemu-timer: Make timer_free()
> imply timer_del()") made the timer_free() call now refuse any NULL
> parameter. char_braille_finalize thus now has to be more careful with
> calling it on baum->cellCount_timer.
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>  chardev/baum.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/chardev/baum.c b/chardev/baum.c
> index 5deca778bc..aca5bf12fb 100644
> --- a/chardev/baum.c
> +++ b/chardev/baum.c
> @@ -631,7 +631,9 @@ static void char_braille_finalize(Object *obj)
>  {
>      BaumChardev *baum = BAUM_CHARDEV(obj);
>  
> -    timer_free(baum->cellCount_timer);
> +    if (baum->cellCount_timer) {
> +        timer_free(baum->cellCount_timer);
> +    }

Paolo's approach is more generic:
https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg03457.html

Re: [PATCH] baum: Fix crash when Braille output is not available

[Laurent Vivier]

Le 10/03/2021 à 18:23, Thomas Huth a écrit :
> On 10/03/2021 17.08, Samuel Thibault wrote:
>> When Braille output is not available, the backend properly reports being
>> unable to be created, but 5f8e93c3e262 ("util/qemu-timer: Make timer_free()
>> imply timer_del()") made the timer_free() call now refuse any NULL
>> parameter. char_braille_finalize thus now has to be more careful with
>> calling it on baum->cellCount_timer.
>>
>> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
>> ---
>>   chardev/baum.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/chardev/baum.c b/chardev/baum.c
>> index 5deca778bc..aca5bf12fb 100644
>> --- a/chardev/baum.c
>> +++ b/chardev/baum.c
>> @@ -631,7 +631,9 @@ static void char_braille_finalize(Object *obj)
>>   {
>>       BaumChardev *baum = BAUM_CHARDEV(obj);
>>   -    timer_free(baum->cellCount_timer);
>> +    if (baum->cellCount_timer) {
>> +        timer_free(baum->cellCount_timer);
>> +    }
>>       if (baum->brlapi) {
>>           brlapi__closeConnection(baum->brlapi);
>>           g_free(baum->brlapi);
> 
> I just tried to debug this problem, too, and came to the same conclusion.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [PATCH] baum: Fix crash when Braille output is not available

[Laurent Vivier]

Le 30/04/2021 à 19:01, Laurent Vivier a écrit :
> Le 10/03/2021 à 18:23, Thomas Huth a écrit :
>> On 10/03/2021 17.08, Samuel Thibault wrote:
>>> When Braille output is not available, the backend properly reports being
>>> unable to be created, but 5f8e93c3e262 ("util/qemu-timer: Make timer_free()
>>> imply timer_del()") made the timer_free() call now refuse any NULL
>>> parameter. char_braille_finalize thus now has to be more careful with
>>> calling it on baum->cellCount_timer.
>>>
>>> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
>>> ---
>>>   chardev/baum.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/chardev/baum.c b/chardev/baum.c
>>> index 5deca778bc..aca5bf12fb 100644
>>> --- a/chardev/baum.c
>>> +++ b/chardev/baum.c
>>> @@ -631,7 +631,9 @@ static void char_braille_finalize(Object *obj)
>>>   {
>>>       BaumChardev *baum = BAUM_CHARDEV(obj);
>>>   -    timer_free(baum->cellCount_timer);
>>> +    if (baum->cellCount_timer) {
>>> +        timer_free(baum->cellCount_timer);
>>> +    }
>>>       if (baum->brlapi) {
>>>           brlapi__closeConnection(baum->brlapi);
>>>           g_free(baum->brlapi);
>>
>> I just tried to debug this problem, too, and came to the same conclusion.
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
>>
> 
> Applied to my trivial-patches branch.
> 

Unapplied from my branch because of:

https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg03457.html

[missed it because all the thread was not cc'ed to trivial]

Thanks,
Laurent