On Thu, 17 Jul 2025 at 12:51, Philippe Mathieu-Daudé <phi...@linaro.org> wrote:
>
> Hi Mark,
>
> On 15/7/25 08:19, Philippe Mathieu-Daudé wrote:
> > From: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
> >
> > In the cases where mixed DMA/non-DMA transfers are used or no data is
> > available, it is possible for the calculated transfer length to be zero.
> > Only call the dma_memory_read function where the transfer length is
> > non-zero to avoid invoking the DMA engine for a zero length transfer
> > which can have side-effects (along with generating additional tracing
> > noise).
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
> > Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
> > Tested-by: Philippe Mathieu-Daudé <phi...@linaro.org>
> > Message-ID: <20250711204636.542964-5-mark.cave-ayl...@ilande.co.uk>
> > Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
> > ---
> >   hw/scsi/esp.c | 20 +++++++++++++-------
> >   1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> > index 62ba4061492..ec9fcbeddf4 100644
> > --- a/hw/scsi/esp.c
> > +++ b/hw/scsi/esp.c
> > @@ -487,8 +487,10 @@ static void esp_do_dma(ESPState *s)
> >       case STAT_MO:
> >           if (s->dma_memory_read) {
> >               len = MIN(len, fifo8_num_free(&s->cmdfifo));
> > -            s->dma_memory_read(s->dma_opaque, buf, len);
> > -            esp_set_tc(s, esp_get_tc(s) - len);
> > +            if (len) {
> > +                s->dma_memory_read(s->dma_opaque, buf, len);
> > +                esp_set_tc(s, esp_get_tc(s) - len);
> > +            }
> >           } else {
> >               len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
> >               len = MIN(fifo8_num_free(&s->cmdfifo), len);
>              }
>              fifo8_push_all(&s->cmdfifo, buf, len);
>
> Coverity reported access to uninitialized buf[]:
>
>  >>>     CID 1612373:         Uninitialized variables  (UNINIT)
>  >>>     Using uninitialized value "*buf" when calling "fifo8_push_all".
>
> Do you mind having a look?

I think this is a false positive (and marked it that way in
the Coverity Scan UI). Coverity is complaining that
we might access buf[] uninitialized, but in the code path
it is complaining about we know that len is zero. The
fifo8_push_all() does a "memcpy(&fifo->data[start], data, num)"
and if num is 0 that is valid and won't access anything in buf[].

We could I suppose make fifo8_push_all() return early for the
num == 0 case, just to make it clearer that it's supposed to work.

thanks
-- PMM

Reply via email to