Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition bugs when reading and writing to message buffers #264

Merged
merged 11 commits into from
Mar 20, 2021
Merged

Fix race condition bugs when reading and writing to message buffers #264

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cda43dd
Fix inaccuracies in prvWriteBytesToBuffer description
milesfrain Feb 18, 2021
ee82c9f
Perform atomic message write in prvWriteMessageToBuffer
milesfrain Feb 18, 2021
1ffa6c7
Remove unnecessary length arg from prvReadMessageFromBuffer
milesfrain Feb 18, 2021
5df0744
Perform atomic message read in prvReadBytesFromBuffer
milesfrain Feb 18, 2021
4223fd5
Merge branch 'main' into atomic-msgbuf
alfred2g Feb 19, 2021
1e4aab6
Apply SpacesAvailable() fix
milesfrain Feb 23, 2021
8190467
Apply review feedback
milesfrain Feb 23, 2021
0f3149e
Edit some prv functions for simplicity and consistency
milesfrain Feb 23, 2021
f6bd2e3
Significant simplification of prvWriteMessageToBuffer
milesfrain Feb 23, 2021
3222f6f
fixup off-by-one comment indentation
milesfrain Feb 23, 2021
3f44440
Merge branch 'main' into atomic-msgbuf
RichardBarry Mar 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Edit some prv functions for simplicity and consistency 
- prvWriteMessageToBuffer
- prvReadMessageFromBuffer
- prvWriteBytesToBuffer
- prvReadBytesFromBuffer
  • Loading branch information
@milesfrain
milesfrain committed Feb 23, 2021
commit 0f3149e2a765d4240a84c061fb6e7925fc497d28
101 changes: 43 additions & 58 deletions stream_buffer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,7 @@ static size_t prvWriteMessageToBuffer( StreamBuffer_t * const pxStreamBuffer,
{
BaseType_t xShouldWrite;
size_t xReturn;
size_t xNextHead = pxStreamBuffer->xHead;
size_t xNextHead = pxStreamBuffer->xHead;

if( xSpace == ( size_t ) 0 )
{
Expand Down Expand Up @@ -759,9 +759,8 @@ static size_t prvWriteMessageToBuffer( StreamBuffer_t * const pxStreamBuffer,

if( xShouldWrite != pdFALSE )
{
/* Writes the data itself. */
xNextHead = prvWriteBytesToBuffer( pxStreamBuffer, ( const uint8_t * ) pvTxData, xDataLengthBytes, xNextHead ); /*lint !e9079 Storage buffer is implemented as uint8_t for ease of sizing, alignment and access. */
pxStreamBuffer->xHead = xNextHead;
/* Write the actual data and update the head to mark the data as officially written. */
pxStreamBuffer->xHead = prvWriteBytesToBuffer( pxStreamBuffer, ( const uint8_t * ) pvTxData, xDataLengthBytes, xNextHead ); /*lint !e9079 Storage buffer is implemented as uint8_t for ease of sizing, alignment and access. */
xReturn = xDataLengthBytes;
}
else
Expand Down Expand Up @@ -979,16 +978,15 @@ static size_t prvReadMessageFromBuffer( StreamBuffer_t * pxStreamBuffer,
size_t xBufferLengthBytes,
size_t xBytesAvailable )
{
size_t xTail, xCount, xNextMessageLength;
size_t xCount, xNextMessageLength;
configMESSAGE_BUFFER_LENGTH_TYPE xTempNextMessageLength;

xTail = pxStreamBuffer->xTail;
size_t xNextTail = pxStreamBuffer->xTail;

if( ( pxStreamBuffer->ucFlags & sbFLAGS_IS_MESSAGE_BUFFER ) != ( uint8_t ) 0 )
{
/* A discrete message is being received. First receive the length
* of the message. */
xTail = prvReadBytesFromBuffer( pxStreamBuffer, ( uint8_t * ) &xTempNextMessageLength, sbBYTES_TO_STORE_MESSAGE_LENGTH, xTail );
xNextTail = prvReadBytesFromBuffer( pxStreamBuffer, ( uint8_t * ) &xTempNextMessageLength, sbBYTES_TO_STORE_MESSAGE_LENGTH, xNextTail );
xNextMessageLength = ( size_t ) xTempNextMessageLength;

/* Reduce the number of bytes available by the number of bytes just
Expand Down Expand Up @@ -1019,11 +1017,8 @@ static size_t prvReadMessageFromBuffer( StreamBuffer_t * pxStreamBuffer,

if( xCount != ( size_t ) 0 )
{
/* Read the actual data. */
xTail = prvReadBytesFromBuffer( pxStreamBuffer, ( uint8_t * ) pvRxData, xCount, xTail); /*lint !e9079 Data storage area is implemented as uint8_t array for ease of sizing, indexing and alignment. */

/* Update the tail to mark the data as officially consumed. */
pxStreamBuffer->xTail = xTail;
/* Read the actual data and update the tail to mark the data as officially consumed. */
pxStreamBuffer->xTail = prvReadBytesFromBuffer( pxStreamBuffer, ( uint8_t * ) pvRxData, xCount, xNextTail); /*lint !e9079 Data storage area is implemented as uint8_t array for ease of sizing, indexing and alignment. */
}

return xCount;
Expand Down Expand Up @@ -1156,20 +1151,18 @@ static size_t prvWriteBytesToBuffer( StreamBuffer_t * const pxStreamBuffer,
size_t xCount,
size_t xHead )
{
size_t xNextHead, xFirstLength;
size_t xFirstLength;

configASSERT( xCount > ( size_t ) 0 );

xNextHead = xHead;

/* Calculate the number of bytes that can be added in the first write -
* which may be less than the total number of bytes that need to be added if
* the buffer will wrap back to the beginning. */
xFirstLength = configMIN( pxStreamBuffer->xLength - xNextHead, xCount );
xFirstLength = configMIN( pxStreamBuffer->xLength - xHead, xCount );

/* Write as many bytes as can be written in the first write. */
configASSERT( ( xNextHead + xFirstLength ) <= pxStreamBuffer->xLength );
( void ) memcpy( ( void * ) ( &( pxStreamBuffer->pucBuffer[ xNextHead ] ) ), ( const void * ) pucData, xFirstLength ); /*lint !e9087 memcpy() requires void *. */
configASSERT( ( xHead + xFirstLength ) <= pxStreamBuffer->xLength );
( void ) memcpy( ( void * ) ( &( pxStreamBuffer->pucBuffer[ xHead ] ) ), ( const void * ) pucData, xFirstLength ); /*lint !e9087 memcpy() requires void *. */

/* If the number of bytes written was less than the number that could be
* written in the first write... */
Expand All @@ -1184,18 +1177,18 @@ static size_t prvWriteBytesToBuffer( StreamBuffer_t * const pxStreamBuffer,
mtCOVERAGE_TEST_MARKER();
}

xNextHead += xCount;
xHead += xCount;

if( xNextHead >= pxStreamBuffer->xLength )
if( xHead >= pxStreamBuffer->xLength )
{
xNextHead -= pxStreamBuffer->xLength;
xHead -= pxStreamBuffer->xLength;
}
else
{
mtCOVERAGE_TEST_MARKER();
}

return xNextHead;
return xHead;
}
/*-----------------------------------------------------------*/

Expand All @@ -1204,51 +1197,43 @@ static size_t prvReadBytesFromBuffer( StreamBuffer_t * pxStreamBuffer,
size_t xCount,
size_t xTail )
{
size_t xFirstLength, xNextTail;
size_t xFirstLength;

xNextTail = xTail;
if( xCount > ( size_t ) 0 )
{
configASSERT( xCount != ( size_t ) 0 );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The non-zero xCount check is performed by all callers, so configASSERT seems more appropriate here, and lets us drop a level of indentation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this one. It is conceivable a user would pass xBufferLengthBytes as zero into prvReadMessageFromBuffer() at run time - in which case it should recover gracefully. Also in that case it would err if configASSERT() was not defined. While it would seem strange to pass xBufferLengthBytes as zero it might happen if, for example, a user passed xBufferLengthBytes as the free space in a buffer and that buffer were to become full.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is conceivable a user would pass xBufferLengthBytes as zero into prvReadMessageFromBuffer() at run time

Are we talking about prvReadMessageFromBuffer() or prvReadBytesFromBuffer()? In either case, the user can't call these private helper functions directly, and any attempt to pass a zero length into these functions is guarded against by existing checks. No guarantees that future code edits preserve all these checks though.

Copy link
Contributor Author

@milesfrain milesfrain Feb 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why my other review comment on this line is being marked as "outdated". Also, github's web viewer gets confused with this section unless you enable the "hide whitespace changes" diff setting.


/* Calculate the number of bytes that can be read - which may be
* less than the number wanted if the data wraps around to the start of
* the buffer. */
xFirstLength = configMIN( pxStreamBuffer->xLength - xNextTail, xCount );
/* Calculate the number of bytes that can be read - which may be
* less than the number wanted if the data wraps around to the start of
* the buffer. */
milesfrain marked this conversation as resolved.
Show resolved Hide resolved
xFirstLength = configMIN( pxStreamBuffer->xLength - xTail, xCount );

/* Obtain the number of bytes it is possible to obtain in the first
* read. Asserts check bounds of read and write. */
configASSERT( xFirstLength <= xCount );
configASSERT( ( xNextTail + xFirstLength ) <= pxStreamBuffer->xLength );
( void ) memcpy( ( void * ) pucData, ( const void * ) &( pxStreamBuffer->pucBuffer[ xNextTail ] ), xFirstLength ); /*lint !e9087 memcpy() requires void *. */
/* Obtain the number of bytes it is possible to obtain in the first
* read. Asserts check bounds of read and write. */
configASSERT( xFirstLength <= xCount );
configASSERT( ( xTail + xFirstLength ) <= pxStreamBuffer->xLength );
( void ) memcpy( ( void * ) pucData, ( const void * ) &( pxStreamBuffer->pucBuffer[ xTail ] ), xFirstLength ); /*lint !e9087 memcpy() requires void *. */

/* If the total number of wanted bytes is greater than the number
* that could be read in the first read... */
if( xCount > xFirstLength )
{
/*...then read the remaining bytes from the start of the buffer. */
configASSERT( xCount <= xCount );
( void ) memcpy( ( void * ) &( pucData[ xFirstLength ] ), ( void * ) ( pxStreamBuffer->pucBuffer ), xCount - xFirstLength ); /*lint !e9087 memcpy() requires void *. */
}
else
{
mtCOVERAGE_TEST_MARKER();
}

/* Move the tail pointer to effectively remove the data read from
* the buffer. */
xNextTail += xCount;

if( xNextTail >= pxStreamBuffer->xLength )
{
xNextTail -= pxStreamBuffer->xLength;
}
/* If the total number of wanted bytes is greater than the number
* that could be read in the first read... */
if( xCount > xFirstLength )
{
/*...then read the remaining bytes from the start of the buffer. */
configASSERT( xCount <= xCount );
( void ) memcpy( ( void * ) &( pucData[ xFirstLength ] ), ( void * ) ( pxStreamBuffer->pucBuffer ), xCount - xFirstLength ); /*lint !e9087 memcpy() requires void *. */
}
else
{
mtCOVERAGE_TEST_MARKER();
}

return xNextTail;
/* Move the tail pointer to effectively remove the data read from the buffer. */
xTail += xCount;

if( xTail >= pxStreamBuffer->xLength )
{
xTail -= pxStreamBuffer->xLength;
}

return xTail;
}
/*-----------------------------------------------------------*/

Expand Down