From 61fc74f0c5c50a7d8ac7fa7f4a2f6deb3a215036 Mon Sep 17 00:00:00 2001
From: Simon Beaudoin <simon.beaudoin.0935@gmail.com>
Date: Mon, 10 Aug 2020 12:55:04 -0400
Subject: [PATCH] Update stream_buffer.c (#94)

Add necessary checks when sending data to the stream/message buffer in order to avoid a task deadlock when attempting to write a longer stream/message than the underlying buffer can write.
---
 stream_buffer.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 50 insertions(+), 3 deletions(-)

diff --git a/stream_buffer.c b/stream_buffer.c
index bef8214e2..449c4febb 100644
--- a/stream_buffer.c
+++ b/stream_buffer.c
@@ -518,7 +518,11 @@ size_t xStreamBufferSend( StreamBufferHandle_t xStreamBuffer,
     size_t xReturn, xSpace = 0;
     size_t xRequiredSpace = xDataLengthBytes;
     TimeOut_t xTimeOut;
-
+    
+    /* Having a 'isFeasible' variable allows to respect the convention that there is only a return statement at the end. Othewise, return
+     * could be done as soon as we realise the send cannot happen. We will let the call to 'prvWriteMessageToBuffer' dealing with this scenario. */
+    BaseType_t xIsFeasible;
+    
     configASSERT( pvTxData );
     configASSERT( pxStreamBuffer );
 
@@ -532,13 +536,56 @@ size_t xStreamBufferSend( StreamBufferHandle_t xStreamBuffer,
 
         /* Overflow? */
         configASSERT( xRequiredSpace > xDataLengthBytes );
+        
+        /* In the case of the message buffer, one has to be able to write the complete message as opposed to
+		 * a stream buffer for semantic reasons. Check if it is physically possible to write the message given
+         * the length of the buffer. */
+		if(xRequiredSpace > pxStreamBuffer->xLength)
+		{
+			/* The message could never be written because it is greater than the buffer length.
+			 * By setting xIsFeasable to FALSE, we skip over the following do..while loop, thus avoiding
+			 * a deadlock. The call to 'prvWriteMessageToBuffer' toward the end of this function with
+			 * xRequiredSpace greater than xSpace will suffice in not writing anything to the internal buffer.
+			 * Now, the function will return 0 because the message could not be written. Should an error code be
+			 * returned instead ??? In my opinion, probably.. But the return type doesn't allow for negative
+			 * values to be returned. A confusion could exist to the caller. Returning 0 because a timeout occurred
+			 * and a subsequent send attempts could eventually succeed, and returning 0 because a write could never
+			 * happen because of the size are two scenarios to me :/ */
+			xIsFeasible = FALSE;
+		}
+		else
+		{
+			/* It is possible to write the message completely in the buffer. This is the intended route.
+			 * Let's continue with the regular timeout logic. */
+			xIsFeasible = TRUE;
+		}
     }
     else
     {
-        mtCOVERAGE_TEST_MARKER();
+        /* In the case of the stream buffer, not being able to completely write the message in the buffer
+		 * is an acceptable scenario, but it has to be dealt with properly */
+		if(xRequiredSpace > pxStreamBuffer->xLength)
+		{
+			/* Not enough buffer space. We will attempt to write as much as we can in this run
+			 * so that the caller can send the remaining in subsequent calls. We avoid a deadlock by
+			 * offering the possibility to take the 'else' branch in the  'if( xSpace < xRequiredSpace )'
+			 * condition inside the following do..while loop */
+			xRequiredSpace = pxStreamBuffer->xLength;
+			
+			/* TODO FIXME: Is there a check we should do with the xTriggerLevelBytes value ? */
+
+			/* With the adjustment to 'xRequiredSpace', the deadlock is avoided, thus it's now feasible. */
+			xIsFeasible = TRUE;
+		}
+		else
+		{
+			/* It is possible to write the message completely in the buffer. */
+			xIsFeasible = TRUE;
+		}
     }
 
-    if( xTicksToWait != ( TickType_t ) 0 )
+    /* Added check against xIsFeasible. If it's not feasible, don't even wait for notification, let the call to 'prvWriteMessageToBuffer' do nothing and return 0 */
+    if( xTicksToWait != ( TickType_t ) 0 && xIsFeasible == TRUE )
     {
         vTaskSetTimeOutState( &xTimeOut );