Re: [PATCH v3 23/25] hw/ipmi: Assert outlen > outpos

[Corey Minyard <minyard@acm.org>]

On Wed, Feb 20, 2019 at 02:02:30AM +0100, Philippe Mathieu-Daudé wrote:
> A througfull audit show that all time data is added to outbuf[],
> 'outlen' is incremented.  Then at creation and each time
> continue_send() returns it pass thru check_reset which resets
> 'outpos', thus we always have 'outlen >= outpos'.

Perhaps: "A thorough audit shows that outlen is always incremented
when data is always added to outbuf[].  Then at creation and each
time continus_send() returns it assures if outpos reaches outlen,
both values are reset to zero, except in the case of sending
a reset where a new command is added."

This is certainly the design intent, thank you for the thorough
audit.

> Also due to the check on entry, we know outlen != 0.
> We can then add an assertion on 'outlen > outpos', which will
> helps the next patch to safely convert 'outlen - outpos' as an

I was a little confused by "next patch", there is no following
patch in the series for this.  Maybe "next part of the patch"?

> unsigned type (size_t).
> 
> Make this assertion explicit by casting 'outlen - outpos' size_t.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Outside of the minor grammer issues, this looks good.  I have
noticed the inconsistent signed/unsigned usage in qemu and IMHO
it's likely to lead to very bad bugs at some point.  There have
been studies that show that unsigned values tend to be more
buggy in usage due to underflows, but for a length value that
will eventually be converted to an unsigned value, what is
here is better, I think.

Both outpos and outlen are unsigned, so the size_t() cast is
not really necessary, but I guess it makes it clear.

Reviewed-by: Corey Minyard <cminyard@mvista.com>

> ---
>  hw/ipmi/ipmi_bmc_extern.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
> index bf0b7ee0f5..ca61b04942 100644
> --- a/hw/ipmi/ipmi_bmc_extern.c
> +++ b/hw/ipmi/ipmi_bmc_extern.c
> @@ -107,8 +107,9 @@ static void continue_send(IPMIBmcExtern *ibe)
>          goto check_reset;
>      }
>   send:
> +    assert(ibe->outlen > ibe->outpos);
>      ret = qemu_chr_fe_write(&ibe->chr, ibe->outbuf + ibe->outpos,
> -                            ibe->outlen - ibe->outpos);
> +                            (size_t)(ibe->outlen - ibe->outpos));
>      if (ret > 0) {
>          ibe->outpos += ret;
>      }
> -- 
> 2.20.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel