[BUG] Buffer/Window properties for incoming connections

[evpopov]

Describe the bug
FreeRTOS+TCP allows us to set/override the TCP stream buffer and window sizes of a socket to values other than the values coming from FreeRTOSIPConfig.h This is accomplished by doing something like this:

#if ( ipconfigUSE_TCP_WIN == 1 )
{
    WinProperties_t xWinProperties;
    BaseType_t xSockOptResult;

    memset( &xWinProperties, '\0', sizeof xWinProperties );

    xWinProperties.lTxBufSize = ( 2 * ipconfigTCP_MSS ); /* Units of bytes. */
    xWinProperties.lTxWinSize = 2; /* Size in units of MSS */
    xWinProperties.lRxBufSize = ( 2 * ipconfigTCP_MSS ); /* Units of bytes. */
    xWinProperties.lRxWinSize = 2; /* Size in units of MSS */

    xSockOptResult = FreeRTOS_setsockopt( NewSocket, 0, FREERTOS_SO_WIN_PROPERTIES, ( void * ) &xWinProperties, sizeof( xWinProperties ) );
    if ( xSockOptResult != 0 )
    {
        FreeRTOS_debug_printf( ( "%s: ERROR %i could not set TCP Buffer/Window properties", pcTaskGetName( NULL ), xSockOptResult ) );
    }
}
#endif /* ( ipconfigUSE_TCP_WIN == 1 ) */

The code above works great for outgoing connection. It does not work reliably for incoming connections.
As far as I can tell, prvSetOptionTCPWindows fails with -pdFREERTOS_ERRNO_EINVAL because the call to prvSockopt_so_buffer
detects that the receive buffer is already allocated. The issue happens intermittently.

It appears that, when accept returns a new connected socket, it depends on the remote host whether the code above succeeds or fails. If the remote host manages to send data before the code above executes, then the socket's stream buffers will get allocated and the call to FreeRTOS_setsockopt fails. If the code above executes before the first data arrives, then FreeRTOS_setsockopt succeeds.

I do not have a fix for this and it looks like this may not be a fundamental and unfixable problem.

Workaround
The new socket returned by FreeRTOS_accept inherits its buffer/window properties from the listening socket, so one workaround might be to execute the code above on the listening socket.

Target
ALL

To Reproduce
Sorry, but I don't have a reliable way of reproducing this. I ran into the issue by running my own http server at very low priority and noticing that every once in a while, the code above would fail. As far as I can tell, it just comes down to timing of when that first piece of data comes from the client that connects.

[htibosch]

it looks like this may not be a fundamental and unfixable problem.

Don't despair, you already gave the solution: set the properties in the parent socket, and every child socket will inherit those properties.
I have written about this in various earlier posts.

On a LAN it happens often that data has been received on a new socket, before you have seen the new socket returned by accept().

When I wrote an FTP server, I saw sockets were opened and closed at a high speed. All child sockets inherit the properties of the parent. Other properties like timing can still be set right after accept().

For sockets that call connect(), this doesn't play a role.

Cheers,

[evpopov]

Thanks for your comment @htibosch
Do you think it would make sense if we always return an error if the user tries to set the buffer/window properties of an incoming connection socket?
The reason I'm asking is because I think it will provide a little more consistency in the behavior.
We can have a comment in the code and a sentence in the documentation explaining the problem and instructing people to apply those parameters to the listening socket. Personally, it took me a few weeks before I noticed something was wrong. If I had gotten an error while writing my accept() code in the first place, I would have read the source and/or documentation and found out how to set these parameters in a predictable manner.

thoughts?

[htibosch]

As it works now, when you try to change a TCP streaming buffer size after the buffer has already been created, the code will show this logging at debug level:

    if( ( lOptionName == FREERTOS_SO_RCVBUF ) && ( rxStream != NULL ) )
    {
        FreeRTOS_debug_printf( ( "Set SO_RCVBUF: buffer already created" ) );
        return -pdFREERTOS_ERRNO_EINVAL;
    }
    if( ( lOptionName == FREERTOS_SO_SNDBUF ) && ( rxStream != NULL ) )
    {
        FreeRTOS_debug_printf( ( "Set SO_SNDBUF: buffer already created" ) );
        return -pdFREERTOS_ERRNO_EINVAL;
    }

( most developers only use FreeRTOS_printf(), and not FreeRTOS_debug_printf() )

Now I must admit that I do not check the result of FreeRTOS_setsockopt() very often, because I don't expect an error.

Now I am defending backward compatibility: there will be some projects that succeed to set SO_RCVBUF/DO_SNDBUF after accept(). After an upgrade, these projects might show unexpected problems because the buffer size not as usual.

Therefor, I would rather propose the following, symbolically:

     if( ( lOptionName == FREERTOS_SO_RCVBUF ) && ( rxStream != NULL ) )
     {
-        FreeRTOS_debug_printf( ( "Set SO_RCVBUF: buffer already created" ) );
+        FreeRTOS_printf( ( "Set SO_RCVBUF: buffer already created" ) );
         return -pdFREERTOS_ERRNO_EINVAL;
     }
     if( ( lOptionName == FREERTOS_SO_SNDBUF ) && ( rxStream != NULL ) )
     {
-        FreeRTOS_debug_printf( ( "Set SO_SNDBUF: buffer already created" ) );
+        FreeRTOS_printf( ( "Set SO_SNDBUF: buffer already created" ) );
         return -pdFREERTOS_ERRNO_EINVAL;
     }
+    if( ( lOptionName == FREERTOS_SO_RCV/SNDBUF ) && ( FreeRTOS_issocketconnected( xSocket ) == pdTRUE ) )
+    {
+        FreeRTOS_printf( ( "Set SO_RCVBUF/SNDBUF: connection already established" ) );
+        /* Print a warning but don't make it a fatal error. */
+        xReturn = 0;
+    }

With the proposed change, you would have seen warnings in the logging about the socket being connected, and it does not break existing projects.

What do you think? Colleague reviewers?

[amazonKamath]

Thanks @evpopov for reporting this. I agree with @htibosch approach considering backwards compatibility. May be add some configASSERTS additionally?

[evpopov]

@htibosch ,you convinced me. I like the idea of using FreeRTOS_printf instead of FreeRTOS_debug_printf. It wouldn't have saved me as I have both disable, but it's a good idea in general.
Let me try and refine your suggestion a bit with the actual existing code:

static BaseType_t prvSockopt_so_buffer( FreeRTOS_Socket_t * pxSocket,
                                        int32_t lOptionName,
                                        const void * pvOptionValue )
{
    uint32_t ulNewValue;
    BaseType_t xReturn;

+   if( ( FreeRTOS_issocketconnected( xSocket ) == pdTRUE ) )
+   {
+       /* If this socket is the child of a listening socket, the remote client may or may not have already sent
+        * us data. If data was already sent, then pxSocket->u.xTCP.rxStream != NULL and this call will fail.
+        * Warn the user about this inconsistent behavior. */
+       FreeRTOS_printf( ( "Warning: Changing buffer/window properties on a connected socket may fail." ) );
+   }
+
    if( pxSocket->ucProtocol != ( uint8_t ) FREERTOS_IPPROTO_TCP )
    {
        FreeRTOS_debug_printf( ( "Set SO_%sBUF: wrong socket type\n",
                                    ( lOptionName == FREERTOS_SO_SNDBUF ) ? "SND" : "RCV" ) );
        xReturn = -pdFREERTOS_ERRNO_EINVAL;
    }
    else if( ( ( lOptionName == FREERTOS_SO_SNDBUF ) && ( pxSocket->u.xTCP.txStream != NULL ) ) ||
                ( ( lOptionName == FREERTOS_SO_RCVBUF ) && ( pxSocket->u.xTCP.rxStream != NULL ) ) )
    {
-       FreeRTOS_debug_printf( ( "Set SO_%sBUF: buffer already created\n",
+       FreeRTOS_printf( ( "Set SO_%sBUF: buffer already created\n",
                                    ( lOptionName == FREERTOS_SO_SNDBUF ) ? "SND" : "RCV" ) );
        xReturn = -pdFREERTOS_ERRNO_EINVAL;
    }
    else
    {
        ulNewValue = *( ( const uint32_t * ) pvOptionValue );

        if( lOptionName == FREERTOS_SO_SNDBUF )
        {
            /* Round up to nearest MSS size */
            ulNewValue = FreeRTOS_round_up( ulNewValue, ( uint32_t ) pxSocket->u.xTCP.usMSS );
            pxSocket->u.xTCP.uxTxStreamSize = ulNewValue;
        }
        else
        {
            pxSocket->u.xTCP.uxRxStreamSize = ulNewValue;
        }

        xReturn = 0;
    }

    return xReturn;
}

Some comments on the changes above:

• In the logs, it would look batter to get the warning and then potentially get the error message instead of the other way around.
• When printing the warning, I am not checking lOptionName because I think it's just a waste of time. This if-clause is just a warning, and prvSockopt_so_buffer() is never called with an invalid parameter anyways.
• Added more info in the comment to keep the FreeRTOS_printf concise, yet have some additional information for inquisitive users that dig deeper and read comments ;-)

what do you think?

[htibosch]

Emil, I agree with all remarks and proposals that you make.
Well except "adding more configASSERT's".
Would you have time to turn this into a PR?
Thanks

[evpopov]

Yes Hein, I don't see any good place or use for assert here as well.
And yes, I will open a PR.