hw/display/xenfb.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
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 <armbru@redhat.com>
---
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;
}
--
2.49.0On Tue, 29 Jul 2025 at 12:14, Markus Armbruster <armbru@redhat.com> 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
On Tue, 14 Oct 2025 at 09:36, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 29 Jul 2025 at 12:14, Markus Armbruster <armbru@redhat.com> 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
On 14/10/25 14:59, Peter Maydell wrote:
> On Tue, 14 Oct 2025 at 09:36, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Tue, 29 Jul 2025 at 12:14, Markus Armbruster <armbru@redhat.com> 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/873564spze.fsf@pond.sub.org/
Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> On 14/10/25 14:59, Peter Maydell wrote:
>> On Tue, 14 Oct 2025 at 09:36, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>
>>> On Tue, 29 Jul 2025 at 12:14, Markus Armbruster <armbru@redhat.com> 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/873564spze.fsf@pond.sub.org/
Ping? Markus Armbruster <armbru@redhat.com> 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 <armbru@redhat.com> > --- > 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; > }
Am 13. Oktober 2025 11:10:45 UTC schrieb Markus Armbruster <armbru@redhat.com>: >Ping? > >Markus Armbruster <armbru@redhat.com> 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 <armbru@redhat.com> >> --- >> 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; >> } > >
Bernhard Beschow <shentey@gmail.com> writes: > Am 13. Oktober 2025 11:10:45 UTC schrieb Markus Armbruster <armbru@redhat.com>: >>Ping? >> >>Markus Armbruster <armbru@redhat.com> 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 <armbru@redhat.com> >>> --- >>> 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/87v7nbdwfx.fsf@pond.sub.org/ [...]
On 13/10/25 13:10, Markus Armbruster wrote: > Ping? > > Markus Armbruster <armbru@redhat.com> 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 <armbru@redhat.com> >> --- >> hw/display/xenfb.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> and queued with g_assert_not_reached(), thanks!
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 <armbru@redhat.com> > --- > 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; > }
Philippe Mathieu-Daudé <philmd@linaro.org> 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 <armbru@redhat.com> >> --- >> 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; >> }
On 29/7/25 14:16, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> 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 <armbru@redhat.com>
>>> ---
>>> 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;
>>> }
>
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é <philmd@linaro.org> 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 <armbru@redhat.com> > > > > --- > > > > 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 :|
© 2016 - 2025 Red Hat, Inc.