Re: [PATCH] hw/i386/pc: Fix enum value assertion

[Richard Henderson]

On 8/21/24 08:52, Kamil Szczęk wrote:
> Coverity reported:
>   >>> CID 1559533:  Integer handling issues (CONSTANT_EXPRESSION_RESULT)
>   >>> "pcms->vmport >= 0" is always true regardless of the values of
>   >>> its operands. This occurs as the logical first operand of "&&".
>
> Signed-off-by: Kamil Szczęk <kamil@szczek.dev>
> Reported-By: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7779c88a91..523dfe3f3f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1221,7 +1221,7 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>           isa_realize_and_unref(pcms->pcspk, isa_bus, &error_fatal);
>       }
>
> -    assert(pcms->vmport >= 0 && pcms->vmport < ON_OFF_AUTO__MAX);
> +    assert(pcms->vmport < ON_OFF_AUTO__MAX);

Given the usage here

>       if (pcms->vmport == ON_OFF_AUTO_AUTO) {

and later, here

>     pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
>                     pcms->vmport != ON_OFF_AUTO_ON, &error_fatal);

absolutely nothing goes off the rails no matter the value of vmport.

It is not used to index an array, which might be out of bounds.
It it not a security issue.
There's no need or benefit for the assert at all.


r~