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

[Markus Armbruster <armbru@xxxxxxxxxx>]

Philippe Mathieu-Daudé <philmd@xxxxxxxxxx> writes:

> 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

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@xxxxxxxxxxxx/