Re: [PATCH] hw/display/xenfb: Replace unreachable code by abort()

[Peter Maydell <peter.maydell@xxxxxxxxxx>]

On Tue, 14 Oct 2025 at 09:36, Peter Maydell <peter.maydell@xxxxxxxxxx> wrote:
>
> On Tue, 29 Jul 2025 at 12:14, Markus Armbruster <armbru@xxxxxxxxxx> 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