call fnet_socket_shutdown() in SS_CLOSING cause FNET_ERR_NOTCONN

[tranvantruonggit]

Assuming the MCU running FNET as a server, after successful connection with the client via TCP, then at some point, the client indicates to shutdown the connection by sending some messages to prepare to tear down the connection, then call API shutdown( clientSock, SHUT_WR). From the FNET server, as the client has not closed yet, calling to fnet_socket_shutdown() shall not return error with the errno FNET_ERR_NOTCONN.

How to reproduce:

• Server side (running FNET)

  • server_sock = fnet_socket(AF_INET, SOCK_STREAM, 0);
  • fnet_socket_bind(server_sock, IP_and_port, sizeof(IP_and_port);
  • fnet_socket_listen();
  • client_sock = fnet_socket_accetp();
  • Above step is successful, then send and receive data as usual, but looking for the special token "exit" every time receive message from the client.
  • Upon receiving "exit" message, invoking the fnet_socket_shutdown(client_sock, SD_WRITE);, at this point we get the return value to be (-1) and the FNET_ERR_NUMBER is FNET_ERR_NOTCONN.

• Client side (on PC):

  • `fnet_socket()'
  • 'fnet_socket_connect()`
  • upon successful, send and receive data.
  • At some point, sending the message "exit" to inform the server that we are about to tear down the connection, then calling the shutdown(clien_socket, SHUT_WR);
  • Other sequence is not relevant here.

The problem happens when the server receives the FIN packet, it changes the socket state sk->state = SS_CLOSING. And tracing the problem from the fnet_socket_shutdown, it eventually called to _fnet_tcp_shutdown, the problem seem to be the following code snippet:

static fnet_return_t _fnet_tcp_shutdown( fnet_socket_if_t *sk, fnet_sd_flags_t how )
{
    /* If the socket is not connected, return.*/
    if(sk->state != SS_CONNECTED) /* <<< I think this is where the problem happen, because the state at this point is SS_CLOSING */
    {
        _fnet_socket_set_error(sk, FNET_ERR_NOTCONN); 
        return FNET_ERR;
    }
    else
... /* some more code */

In my opinion, the SS_CLOSING meaning that the connection is still not reach the CLOSED state yet, thus, considering it is the error is not the expected behavior. In the normal case, it shall be able to run the another else branch and sending out the FIN packet and process, before we actually call the close function. My proposal, if that is the valid finding, is changing that if condition to:

if(sk->state == SS_CLOSED)
{
    /* Raise error */
}
else
{
    /* process as usual */
}

I would like to know your opinion on this finding, as it seems the behavior is different from the document from this link https://man.freebsd.org/cgi/man.cgi?query=shutdown&sektion=2

[ghost]

it seems the behavior is different from the document from this link

It is the same: [ENOTCONN] The s argument specifies a socket which is not connected.
=> if(sk->state != SS_CONNECTED)
Probably, it is better to look inside a FreeBSD source code to clarify the meaning.

[tranvantruonggit]

I have been traced some of the implementation of the Linux and FreeBSD. My observation is that the implementation of the shutdown generally allow to be successful in case the socket is not closed. With that said, based on the reference implementation is some bsd/linux implementation of the shutdown function, ENOTCONN is not returned in case SS_CLOSING (or SS_DISCONNECTING), SS_CONNECTING, SS_CONNECTED. Reference to the following implementation:

• http://fxr.watson.org/fxr/source/bsd/kern/uipc_socket.c?v=xnu-8792#L4676 (function soshutdown).

• [socket.c - net/tipc/socket.c - Linux source code (v3.7.1) - Bootlin](https://elixir.bootlin.com/linux/v3.7.1/source/net/tipc/socket.c#L1522)

Futhermore, I think the FNET implementation at the moment is that the tcp_connection_state after receiving FIN from the remoted node will be in the state cb->tcpcb_connection_state = CLOSE_WAIT, at the same time, the sk->sk_state is SS_CLOSING.

Honestly, I am not sure my finding is valid based on the other OSes' implementation, because there is no concrete document about this case. However, one of the problem I see is that the fnet' shutdown function always return ENOTCONN if it receives the FIN firstly, it doesn't process to send another FIN to the peer node.

I am eager to know your opinion regarding this.

[ghost]

So we can change the code to:

    if(sk->state == SS_CLOSED) 
    {
        _fnet_socket_set_error(sk, FNET_ERR_NOTCONN); 
        return FNET_ERR;
    }
   else

Are you OK with it?

[tranvantruonggit]

I think that is possible. I will test your suggestion and let you know the outcome within couple of days.On 17 Jul 2023, at 16:19, Andrej Butok ***@***.***> wrote: So we can change the code to: if(sk->state == SS_CLOSED) { _fnet_socket_set_error(sk, FNET_ERR_NOTCONN); return FNET_ERR; } else Are you OK with it? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

[tranvantruonggit]

After a few testings, I can confirm the change in the condition if(sk->state == SS_CLOSED) creating the expected behavior.

Do you want me to create the Pull Request or you can update it when you have a chance?

Here is the snippet of code I wrote to do the sanity test the modification. I put here just for the sake of tracking the discussion.

...
    // Check if received data is "exit"
    if (  buffer[0] == 'e' &&
          buffer[1] == 'x'&&
          buffer[2] == 'i'&&
          buffer[3] == 't') {
        // Print shutdown message
        char shutdown_msg[] = "Received 'exit' command. Shutting down writing.\n";
        fnet_socket_send(context->sockfd, shutdown_msg, sizeof(shutdown_msg) - 1,0);
    
    
        fnet_socket_getopt(context->sockfd,SOL_SOCKET,SO_STATE,(void*)&test_socket_state,(fnet_size_t*)&len);
        // Shutdown writing
        test_fnet_return = fnet_socket_shutdown(context->sockfd, SD_WRITE);
        context->state = ETH_CLIENT_CLOSING;
    }
...

The above code check the state of the socket via the variable test_socket_state and read the return of the fnet_socket_shutdown in test_fnet_return

The behaviour before the modification:

• test_socket_state = SS_CLOSING
• test_fnet_return = FNET_ERR

The behaviour after the modification:

• test_socket_state = SS_CLOSING
• test_fnet_return = FNET_OK

[ghost]

Do you want me to create the Pull Request or you can update it when you have a chance?

Yes, if you can. It will be quicker.
I am occupied by different projects now, so it can take much time waiting for me.

[butok]

Merged.

[tranvantruonggit]

After thought, though it is irrelevant, my "fix" for the _fnet_tcp_initial_seq_number_update is not an actual fix (it doesn't change the compiled code), just an typo fix by putting parenthesis in.

I have investigated a bit and it seem Henri de Veer from your the sourceforge thread has addressed it correctly.

I will make another PR to fix that, are you fine with that?

Really sorry for my overlooking in that function.

[butok]

ok