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

[Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>]

On 14/10/25 14:59, Peter Maydell wrote:
> 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 :-))
See this thread where it was discussed (until I gave up...):
https://lore.kernel.org/qemu-devel/873564spze.fsf@xxxxxxxxxxxx/