Re: [PATCH] hw/display/xenfb: Replace unreachable code by abort()
On Tue, 29 Jul 2025 at 12:14, Markus Armbruster wrote:
>
> xenfb_mouse_event() has a switch statement whose controlling
> expression move->axis is an enum InputAxis. The enum values are
> INPUT_AXIS_X and INPUT_AXIS_Y, encoded as 0 and 1. The switch has a
> case for both axes. In addition, it has an unreachable default label.
> This convinces Coverity that move->axis can be greater than 1. It
> duly reports a buffer overrun when it is used to subscript an array
> with two elements.
I think also that Coverity gets confused by QAPI's convention
in generated code of defining enumerations like this:
typedef enum InputAxis {
INPUT_AXIS_X,
INPUT_AXIS_Y,
INPUT_AXIS__MAX,
} InputAxis;
Coverity thinks that INPUT_AXIS__MAX might be a valid
value it can see in move->axis, because we defined the
enum that way.
In theory I suppose that since the __MAX value is only
there to be an array or enumeration bound that we could
emit code that #defines it rather than making it part
of the enum.
thanks
-- PMM
Re: [PATCH] hw/display/xenfb: Replace unreachable code by abort()
On Tue, 14 Oct 2025 at 09:36, Peter Maydell wrote:
>
> On Tue, 29 Jul 2025 at 12:14, Markus Armbruster wrote:
> >
> > xenfb_mouse_event() has a switch statement whose controlling
> > expression move->axis is an enum InputAxis. The enum values are
> > INPUT_AXIS_X and INPUT_AXIS_Y, encoded as 0 and 1. The switch has a
> > case for both axes. In addition, it has an unreachable default label.
> > This convinces Coverity that move->axis can be greater than 1. It
> > duly reports a buffer overrun when it is used to subscript an array
> > with two elements.
>
> I think also that Coverity gets confused by QAPI's convention
> in generated code of defining enumerations like this:
>
> typedef enum InputAxis {
> INPUT_AXIS_X,
> INPUT_AXIS_Y,
> INPUT_AXIS__MAX,
> } InputAxis;
>
> Coverity thinks that INPUT_AXIS__MAX might be a valid
> value it can see in move->axis, because we defined the
> enum that way.
>
> In theory I suppose that since the __MAX value is only
> there to be an array or enumeration bound that we could
> emit code that #defines it rather than making it part
> of the enum.
Also, there's an argument that this function should
ignore unknown input-axis enum values. If we ever in future
extend this to support a Z-axis, it would be better to
ignore the events we can't send, the same way we already
do for unknown INPUT_EVENT_KIND_BTN button types, rather
than aborting. But it's not very important, so the
g_assert_not_reached() will do.
(In some other languages, we'd get a compile failure for
adding a new value to the enum that we didn't handle.
But not in C :-))
thanks
-- PMM
Re: [PATCH] hw/display/xenfb: Replace unreachable code by abort()
Ping? Markus Armbruster writes: > xenfb_mouse_event() has a switch statement whose controlling > expression move->axis is an enum InputAxis. The enum values are > INPUT_AXIS_X and INPUT_AXIS_Y, encoded as 0 and 1. The switch has a > case for both axes. In addition, it has an unreachable default label. > This convinces Coverity that move->axis can be greater than 1. It > duly reports a buffer overrun when it is used to subscript an array > with two elements. > > Replace the unreachable code by abort(). > > Resolves: Coverity CID 1613906 > Signed-off-by: Markus Armbruster > --- > hw/display/xenfb.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c > index 22822fecea..5e6c691779 100644 > --- a/hw/display/xenfb.c > +++ b/hw/display/xenfb.c > @@ -283,8 +283,7 @@ static void xenfb_mouse_event(DeviceState *dev, > QemuConsole *src, > scale = surface_height(surface) - 1; > break; > default: > -scale = 0x8000; > -break; > +abort(); > } > xenfb->axis[move->axis] = move->value * scale / 0x7fff; > }
Re: [PATCH] hw/display/xenfb: Replace unreachable code by abort()
On 14/10/25 14:59, Peter Maydell wrote:
On Tue, 14 Oct 2025 at 09:36, Peter Maydell wrote:
On Tue, 29 Jul 2025 at 12:14, Markus Armbruster wrote:
xenfb_mouse_event() has a switch statement whose controlling
expression move->axis is an enum InputAxis. The enum values are
INPUT_AXIS_X and INPUT_AXIS_Y, encoded as 0 and 1. The switch has a
case for both axes. In addition, it has an unreachable default label.
This convinces Coverity that move->axis can be greater than 1. It
duly reports a buffer overrun when it is used to subscript an array
with two elements.
I think also that Coverity gets confused by QAPI's convention
in generated code of defining enumerations like this:
typedef enum InputAxis {
INPUT_AXIS_X,
INPUT_AXIS_Y,
INPUT_AXIS__MAX,
} InputAxis;
Coverity thinks that INPUT_AXIS__MAX might be a valid
value it can see in move->axis, because we defined the
enum that way.
In theory I suppose that since the __MAX value is only
there to be an array or enumeration bound that we could
emit code that #defines it rather than making it part
of the enum.
Also, there's an argument that this function should
ignore unknown input-axis enum values. If we ever in future
extend this to support a Z-axis, it would be better to
ignore the events we can't send, the same way we already
do for unknown INPUT_EVENT_KIND_BTN button types, rather
than aborting. But it's not very important, so the
g_assert_not_reached() will do.
(In some other languages, we'd get a compile failure for
adding a new value to the enum that we didn't handle.
But not in C :-))
See this thread where it was discussed (until I gave up...):
https://lore.kernel.org/qemu-devel/[email protected]/
Re: [PATCH] hw/display/xenfb: Replace unreachable code by abort()
Philippe Mathieu-Daudé writes:
> On 14/10/25 14:59, Peter Maydell wrote:
>> On Tue, 14 Oct 2025 at 09:36, Peter Maydell wrote:
>>>
>>> On Tue, 29 Jul 2025 at 12:14, Markus Armbruster wrote:
xenfb_mouse_event() has a switch statement whose controlling
expression move->axis is an enum InputAxis. The enum values are
INPUT_AXIS_X and INPUT_AXIS_Y, encoded as 0 and 1. The switch has a
case for both axes. In addition, it has an unreachable default label.
This convinces Coverity that move->axis can be greater than 1. It
duly reports a buffer overrun when it is used to subscript an array
with two elements.
>>>
>>> I think also that Coverity gets confused by QAPI's convention
>>> in generated code of defining enumerations like this:
>>>
>>> typedef enum InputAxis {
>>> INPUT_AXIS_X,
>>> INPUT_AXIS_Y,
>>> INPUT_AXIS__MAX,
>>> } InputAxis;
>>>
>>> Coverity thinks that INPUT_AXIS__MAX might be a valid
>>> value it can see in move->axis, because we defined the
>>> enum that way.
>>>
>>> In theory I suppose that since the __MAX value is only
>>> there to be an array or enumeration bound
Correct.
>>> that we could
>>> emit code that #defines it rather than making it part
>>> of the enum.
I'd love that, but it's harder than it has any right to be; see the
message Philippe referred to below.
>> Also, there's an argument that this function should
>> ignore unknown input-axis enum values. If we ever in future
>> extend this to support a Z-axis, it would be better to
>> ignore the events we can't send, the same way we already
>> do for unknown INPUT_EVENT_KIND_BTN button types, rather
>> than aborting.
No objection.
>>But it's not very important, so the
>> g_assert_not_reached() will do.
>>
>> (In some other languages, we'd get a compile failure for
>> adding a new value to the enum that we didn't handle.
>> But not in C :-))
>
> See this thread where it was discussed (until I gave up...):
> https://lore.kernel.org/qemu-devel/[email protected]/
Re: [PATCH] hw/display/xenfb: Replace unreachable code by abort()
Bernhard Beschow writes: > Am 13. Oktober 2025 11:10:45 UTC schrieb Markus Armbruster > : >>Ping? >> >>Markus Armbruster writes: >> >>> xenfb_mouse_event() has a switch statement whose controlling >>> expression move->axis is an enum InputAxis. The enum values are >>> INPUT_AXIS_X and INPUT_AXIS_Y, encoded as 0 and 1. The switch has a >>> case for both axes. In addition, it has an unreachable default label. >>> This convinces Coverity that move->axis can be greater than 1. It >>> duly reports a buffer overrun when it is used to subscript an array >>> with two elements. >>> >>> Replace the unreachable code by abort(). >>> >>> Resolves: Coverity CID 1613906 >>> Signed-off-by: Markus Armbruster >>> --- >>> hw/display/xenfb.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c >>> index 22822fecea..5e6c691779 100644 >>> --- a/hw/display/xenfb.c >>> +++ b/hw/display/xenfb.c >>> @@ -283,8 +283,7 @@ static void xenfb_mouse_event(DeviceState *dev, >>> QemuConsole *src, >>> scale = surface_height(surface) - 1; >>> break; >>> default: >>> -scale = 0x8000; >>> -break; >>> +abort(); > > Don't we prefer g_assert_not_reached() these days, for more expressiveness? See https://lore.kernel.org/qemu-devel/[email protected]/ [...]
Re: [PATCH] hw/display/xenfb: Replace unreachable code by abort()
Am 13. Oktober 2025 11:10:45 UTC schrieb Markus Armbruster : >Ping? > >Markus Armbruster writes: > >> xenfb_mouse_event() has a switch statement whose controlling >> expression move->axis is an enum InputAxis. The enum values are >> INPUT_AXIS_X and INPUT_AXIS_Y, encoded as 0 and 1. The switch has a >> case for both axes. In addition, it has an unreachable default label. >> This convinces Coverity that move->axis can be greater than 1. It >> duly reports a buffer overrun when it is used to subscript an array >> with two elements. >> >> Replace the unreachable code by abort(). >> >> Resolves: Coverity CID 1613906 >> Signed-off-by: Markus Armbruster >> --- >> hw/display/xenfb.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c >> index 22822fecea..5e6c691779 100644 >> --- a/hw/display/xenfb.c >> +++ b/hw/display/xenfb.c >> @@ -283,8 +283,7 @@ static void xenfb_mouse_event(DeviceState *dev, >> QemuConsole *src, >> scale = surface_height(surface) - 1; >> break; >> default: >> -scale = 0x8000; >> -break; >> +abort(); Don't we prefer g_assert_not_reached() these days, for more expressiveness? Best regards, Bernhard >> } >> xenfb->axis[move->axis] = move->value * scale / 0x7fff; >> } > >
Re: [PATCH] hw/display/xenfb: Replace unreachable code by abort()
On 13/10/25 13:10, Markus Armbruster wrote: Ping? Markus Armbruster writes: xenfb_mouse_event() has a switch statement whose controlling expression move->axis is an enum InputAxis. The enum values are INPUT_AXIS_X and INPUT_AXIS_Y, encoded as 0 and 1. The switch has a case for both axes. In addition, it has an unreachable default label. This convinces Coverity that move->axis can be greater than 1. It duly reports a buffer overrun when it is used to subscript an array with two elements. Replace the unreachable code by abort(). Resolves: Coverity CID 1613906 Signed-off-by: Markus Armbruster --- hw/display/xenfb.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé and queued with g_assert_not_reached(), thanks!
Re: [PATCH] hw/display/xenfb: Replace unreachable code by abort()
On Tue, Jul 29, 2025 at 02:59:46PM +0200, Philippe Mathieu-Daudé wrote: > On 29/7/25 14:16, Markus Armbruster wrote: > > Philippe Mathieu-Daudé writes: > > > > > On 29/7/25 13:12, Markus Armbruster wrote: > > > > xenfb_mouse_event() has a switch statement whose controlling > > > > expression move->axis is an enum InputAxis. The enum values are > > > > INPUT_AXIS_X and INPUT_AXIS_Y, encoded as 0 and 1. The switch has a > > > > case for both axes. In addition, it has an unreachable default label. > > > > This convinces Coverity that move->axis can be greater than 1. It > > > > duly reports a buffer overrun when it is used to subscript an array > > > > with two elements. > > > > Replace the unreachable code by abort(). > > > > Resolves: Coverity CID 1613906 > > > > Signed-off-by: Markus Armbruster > > > > --- > > > >hw/display/xenfb.c | 3 +-- > > > >1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c > > > > index 22822fecea..5e6c691779 100644 > > > > --- a/hw/display/xenfb.c > > > > +++ b/hw/display/xenfb.c > > > > @@ -283,8 +283,7 @@ static void xenfb_mouse_event(DeviceState *dev, > > > > QemuConsole *src, > > > >scale = surface_height(surface) - 1; > > > >break; > > > >default: > > > > -scale = 0x8000; > > > > -break; > > > > +abort(); > > > > > > We prefer GLib g_assert_not_reached() over abort() because it displays > > > the file, line number & function before aborting. > > > > The purpose of this line is to tell the compiler we can't get there, > > with the least amount of ceremony. > > > > We have ~600 calls of abort(). > > And ~1600 of g_assert_not_reached() =) > > $ git grep -w 'abort();' | wc -l > 556 > $ git grep -w 'g_assert_not_reached();' | wc -l > 1551 Sounds like we could create a gitlab issue for "replace abort with g_assert_not_reached" that could be a easy on-ramp for someone to start contributing. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] hw/display/xenfb: Replace unreachable code by abort()
On 29/7/25 14:16, Markus Armbruster wrote: Philippe Mathieu-Daudé writes: On 29/7/25 13:12, Markus Armbruster wrote: xenfb_mouse_event() has a switch statement whose controlling expression move->axis is an enum InputAxis. The enum values are INPUT_AXIS_X and INPUT_AXIS_Y, encoded as 0 and 1. The switch has a case for both axes. In addition, it has an unreachable default label. This convinces Coverity that move->axis can be greater than 1. It duly reports a buffer overrun when it is used to subscript an array with two elements. Replace the unreachable code by abort(). Resolves: Coverity CID 1613906 Signed-off-by: Markus Armbruster --- hw/display/xenfb.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index 22822fecea..5e6c691779 100644 --- a/hw/display/xenfb.c +++ b/hw/display/xenfb.c @@ -283,8 +283,7 @@ static void xenfb_mouse_event(DeviceState *dev, QemuConsole *src, scale = surface_height(surface) - 1; break; default: -scale = 0x8000; -break; +abort(); We prefer GLib g_assert_not_reached() over abort() because it displays the file, line number & function before aborting. The purpose of this line is to tell the compiler we can't get there, with the least amount of ceremony. We have ~600 calls of abort(). And ~1600 of g_assert_not_reached() =) $ git grep -w 'abort();' | wc -l 556 $ git grep -w 'g_assert_not_reached();' | wc -l 1551 Whoever merges this: feel free to replace by g_assert_not_reached(). } xenfb->axis[move->axis] = move->value * scale / 0x7fff; }
Re: [PATCH] hw/display/xenfb: Replace unreachable code by abort()
Philippe Mathieu-Daudé writes: > On 29/7/25 13:12, Markus Armbruster wrote: >> xenfb_mouse_event() has a switch statement whose controlling >> expression move->axis is an enum InputAxis. The enum values are >> INPUT_AXIS_X and INPUT_AXIS_Y, encoded as 0 and 1. The switch has a >> case for both axes. In addition, it has an unreachable default label. >> This convinces Coverity that move->axis can be greater than 1. It >> duly reports a buffer overrun when it is used to subscript an array >> with two elements. >> Replace the unreachable code by abort(). >> Resolves: Coverity CID 1613906 >> Signed-off-by: Markus Armbruster >> --- >> hw/display/xenfb.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c >> index 22822fecea..5e6c691779 100644 >> --- a/hw/display/xenfb.c >> +++ b/hw/display/xenfb.c >> @@ -283,8 +283,7 @@ static void xenfb_mouse_event(DeviceState *dev, >> QemuConsole *src, >> scale = surface_height(surface) - 1; >> break; >> default: >> -scale = 0x8000; >> -break; >> +abort(); > > We prefer GLib g_assert_not_reached() over abort() because it displays > the file, line number & function before aborting. The purpose of this line is to tell the compiler we can't get there, with the least amount of ceremony. We have ~600 calls of abort(). Whoever merges this: feel free to replace by g_assert_not_reached(). >> } >> xenfb->axis[move->axis] = move->value * scale / 0x7fff; >> }
Re: [PATCH] hw/display/xenfb: Replace unreachable code by abort()
On 29/7/25 13:12, Markus Armbruster wrote: xenfb_mouse_event() has a switch statement whose controlling expression move->axis is an enum InputAxis. The enum values are INPUT_AXIS_X and INPUT_AXIS_Y, encoded as 0 and 1. The switch has a case for both axes. In addition, it has an unreachable default label. This convinces Coverity that move->axis can be greater than 1. It duly reports a buffer overrun when it is used to subscript an array with two elements. Replace the unreachable code by abort(). Resolves: Coverity CID 1613906 Signed-off-by: Markus Armbruster --- hw/display/xenfb.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index 22822fecea..5e6c691779 100644 --- a/hw/display/xenfb.c +++ b/hw/display/xenfb.c @@ -283,8 +283,7 @@ static void xenfb_mouse_event(DeviceState *dev, QemuConsole *src, scale = surface_height(surface) - 1; break; default: -scale = 0x8000; -break; +abort(); We prefer GLib g_assert_not_reached() over abort() because it displays the file, line number & function before aborting. } xenfb->axis[move->axis] = move->value * scale / 0x7fff; }
