On 17/07/2025 12:58, Peter Maydell wrote:
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 both!
I can certainly look to update fifo8_push_all() if you think it would be a better
solution?
ATB,
Mark.
