On 4/1/21 12:51 PM, Mark Cave-Ayland wrote: > On 01/04/2021 10:34, Philippe Mathieu-Daudé wrote: >> 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" > > Ha. It's definitely "sanitiser" here in the UK (UK English) as opposed > to "sanitizer" (US English).
Oh OK, TIL :) > I don't really mind either way, but I can > fix this if it needs a v4 following Paolo's comments. Not needed since it is correct. >>> 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? > > Yes that's correct, which is why Fifo8 currently doesn't support > wraparound. I haven't analysed how other devices have used it but I > would imagine there would be an ASan hit if it were being misused this way. > >>> 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); > > That could work, and may even allow support for wraparound in future. I > suspect things would become clearer after looking at the other Fifo8 > users to see if this is worth an API change/alternative API. Thanks for the feedback! Phil.
