Hi Marko,
Thank you for that analysis. I used this fifo design a lot, e.g in
FreeDV GUI and the SM1000. No current issues that I am aware of but
they could be subtle.
What specific changes to fifo.c would you suggest?
Is there a unit test that can demonstrate the problem?
Thanks,
David
On 29/06/18 13:56, Marko Milutinovic wrote:
> Hello,
>
>
>
> I was just going through the code and noticed a possible issue with the
> fifo code. I think there will be instances where it is not two process
> safe. I have similar code which I got burned on recently. It applies to
> both read and write.
>
>
>
> int fifo_write(struct FIFO *fifo, short data[], int n) {
>
> int i;
>
> short *pdata;
>
> short *pin = fifo->pin;
>
>
>
> assert(fifo != NULL);
>
> assert(data != NULL);
>
>
>
> if (n > fifo_free(fifo)) {
>
> return -1;
>
> }
>
> else {
>
>
>
> /* This could be made more efficient with block copies
>
> using memcpy */
>
>
>
> pdata = data;
>
> for(i=0; i<n; i++) {
>
> 1. -> * *pin++ = *pdata++; *
> 2. -> * if (pin == (fifo->buf + fifo->nshort))*
>
> * pin = fifo->buf;*
>
> }
>
> fifo->pin = pin;
>
> }
>
>
>
> return 0;
>
> }
>
>
>
> 1. Depending on compiler/optimizations it is possible that the
> following happens:
> 1. *pin* pointer value will be loaded to a register from memory
> 2. The actual value in memory will be incremented. The pointer
> increment is essentially done before storing data
> 3. The value stored in the register is used to store *pdata*.
>
>
>
> The intent of the C code is not violated by this sequence, but the
> compiler has no idea this is being used as a 2 process safe code. If
> this happens and the reading process runs at just the right time it
> could read out invalid data. To get around this make *pin* volatile and
> do the increment after the data transfer (the volatile will ensure the
> compiler maintains the order you want).
>
>
>
> 2. At this point it’s possible for *pin* to point to invalid memory. If
> the reading process gets context at this instant there could be a
> lot of invalid data read out. This one is harder to fix. Typically I
> use free running offsets instead of pointers for this type of code.
> Also my code requires modulus, but to make it more efficient I make
> the Fifo size be a power of 2 so that I can use the and operator
> (ulMyBufferMask is fifo size -1)
>
>
>
> ULONG
>
> Length()
>
> {
>
> return (((ULONG)(ulMyHead - ulMyTail)) & ulMyBufferMask);
>
> }
>
>
>
> ULONG
>
> FreeSpace()
>
> {
>
> return (ulMyBufferMask - Length());
>
> }
>
>
>
> BOOL
>
> Put(
>
> UCHAR* pucBuf_, //The buffer to store
>
> USHORT usLen_) //Number of elements in the passed in buffer that
> will be stored
>
> {
>
> //Check there is enough room for the entire buffer
>
> if (usLen_ > FreeSpace())
>
> return FALSE;
>
>
>
> //Store buffer
>
> for (USHORT i = 0; i < usLen_; i++)
>
> {
>
> pucMyBuffer[ulMyHead & ulMyBufferMask] = pucBuf_[i];
>
> ulMyHead++;
>
> }
>
>
>
> return TRUE;
>
> }
>
>
>
> If I misunderstood the code and there are no issues please ignore my
> long blurb. Just bringing this up as the comments indicate the Fifo is
> meant to be 2 process safe. Data corruption created by these types of
> bugs will be very timing dependent and infuriating to debug so hopefully
> this helps.
>
>
>
> Cheers,
>
> Marko
>
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>
>
>
> _______________________________________________
> Freetel-codec2 mailing list
> Freetel-codec2@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/freetel-codec2
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Freetel-codec2 mailing list
Freetel-codec2@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/freetel-codec2
