Re: [PATCH v3 05/11] esp: introduce esp_fifo_pop_buf() and use it instead of fifo8_pop_buf()

[Philippe Mathieu-Daudé]

On 4/1/21 9:49 AM, Mark Cave-Ayland wrote:
> The const pointer returned by fifo8_pop_buf() lies directly within the array 
> used
> to model the FIFO. Building with address sanitisers enabled shows that if the

Typo "sanitizers"

> caller expects a minimum number of bytes present then if the FIFO is nearly 
> full,
> the caller may unexpectedly access past the end of the array.

Why isn't it a problem with the other models? Because the pointed
buffer is consumed directly?

> Introduce esp_fifo_pop_buf() which takes a destination buffer and performs a
> memcpy() in it to guarantee that the caller cannot overwrite the FIFO array 
> and
> update all callers to use it. Similarly add underflow protection similar to
> esp_fifo_push() and esp_fifo_pop() so that instead of triggering an assert()
> the operation becomes a no-op.

This is OK for your ESP model.

Now thinking loudly about the Fifo8 API, shouldn't this be part of it?

Something prototype like:

  /**
   * fifo8_pop_buf:
   * @do_copy: If %true, also copy data to @bufptr.
   */
  size_t fifo8_pop_buf(Fifo8 *fifo,
                       void **bufptr,
                       size_t buflen,
                       bool do_copy);

> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1909247
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/scsi/esp.c | 41 +++++++++++++++++++++++++++++------------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index ce88866803..1aa2caf57d 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -107,6 +107,7 @@ static void esp_fifo_push(Fifo8 *fifo, uint8_t val)
>  
>      fifo8_push(fifo, val);
>  }
> +
>  static uint8_t esp_fifo_pop(Fifo8 *fifo)
>  {
>      if (fifo8_is_empty(fifo)) {
> @@ -116,6 +117,23 @@ static uint8_t esp_fifo_pop(Fifo8 *fifo)
>      return fifo8_pop(fifo);
>  }
>  
> +static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
> +{
> +    const uint8_t *buf;
> +    uint32_t n;
> +
> +    if (maxlen == 0) {
> +        return 0;
> +    }
> +
> +    buf = fifo8_pop_buf(fifo, maxlen, &n);
> +    if (dest) {
> +        memcpy(dest, buf, n);
> +    }
> +
> +    return n;
> +}

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>