| Message ID | 20210310160815.3411019-1-samuel.thibault@ens-lyon.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | baum: Fix crash when Braille output is not available | expand |
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
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
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>
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
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
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
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);
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(-)