Riot: pkg/tinydtls: async event SOCK_ASYNC_CONN_FIN does not indicate which session has been finished

[janosbrodbeck]

Description

When using tinyDTLS and the sock/dtls API with asynchronous sockets the event for a finished session does not indicate which session has been closed. It makes it impossible to keep the session for a long time because you cannot track sessions properly without touching the internals of tinydtls.

The following code block is the part in sock_dtls.c which handles this part. The internal event gets the correct closed session it is just not passed to the callback. I'm not quite sure what sock->async_cb(sock, SOCK_ASYNC_CONN_FIN, sock->async_cb_arg); hands over to the cb function. sock->async_cb_arg? Whats that?

pkg/tinydtls/contrib/sock_dtls.c

static int _event(struct dtls_context_t *ctx, session_t *session,
                  dtls_alert_level_t level, unsigned short code)
{
    (void)level;
    (void)session;

    sock_dtls_t *sock = dtls_get_app_data(ctx);
    msg_t msg = { .type = code, .content.ptr = session };

    if (IS_ACTIVE(ENABLE_DEBUG)) {
        switch (code) {
            case DTLS_EVENT_CONNECT:
                DEBUG("sock_dtls: event connect\n");
                break;
            case DTLS_EVENT_CONNECTED:
                DEBUG("sock_dtls: event connected\n");
                break;
            case DTLS_EVENT_RENEGOTIATE:
                DEBUG("sock_dtls: event renegotiate\n");
                break;
        }
    }
    if (!level && (code != DTLS_EVENT_CONNECT)) {
        mbox_put(&sock->mbox, &msg);
    }
#ifdef SOCK_HAS_ASYNC
    if (sock->async_cb != NULL) {
        switch (code) {
            case DTLS_ALERT_CLOSE_NOTIFY:
                /* peer closed their session */
                if(session) {
                    printf("session is != null!\n");
                    printf("Port: %d\n", session->port);
                }
                sock->async_cb(sock, SOCK_ASYNC_CONN_FIN, sock->async_cb_arg);
                break;
            case DTLS_EVENT_CONNECTED:
                /* we received a session handshake initialization */
                sock->async_cb(sock, SOCK_ASYNC_CONN_RECV,
                               sock->async_cb_arg);
                break;
            default:
                break;
        }
    } else {
        printf("not existing\n");
    }
#endif
    return 0;
}

Steps to reproduce the issue

Expected results

Argument of the callback event points to the closed session.

Actual results

The argument is currently null.

Versions

[janosbrodbeck]

Maybe @pokgak @leandrolanzieri could help?

[janosbrodbeck]

Changing it to sock->async_cb(sock, SOCK_ASYNC_CONN_FIN, session); does work. Of course, it is a session_t pointer but this way it's possible to retrieve information about the closed sessions. Should be possible to convert it to a sock_dtls_session_t object and then hand over that pointer. Does anything speak against it?

[pokgak]

From the code, async_cb_arg is set by the client/server: client/server call sock_dtls_event_init, which then calls sock_dtls_set_cb that sets async_cb_arg. Based on the async tinydtls test, It seems the server does not use the async_cb_arg while the client use it to hold data to send later.

I think it makes sense to return a pointer to a sock_dtls_session_t for the SOCK_ASYNC_CONN_FIN event using the arg parameter of the async_cb. One thing I'm not sure about is if we're breaking any assumption expected by the handler by not using the async_cb_arg given from the client/server.

The way I understand it, it should be okay for sock_dtls to overwrite async_cb_arg to pass back data to the client/server. @miri64 is this correct?

[miri64]

Yeah, I would set async_cb_arg to the session when the event is fired, not when it is handled.

[miri64]

Mhhh on the other hand, async_cb_arg is supposed to be set on callback initialization:

https://github.com/RIOT-OS/RIOT/blob/c337089de58aa5c75395268f483fc5fe11ed3d64/pkg/tinydtls/contrib/sock_dtls.c#L625-L628

Just overriding it might lead to unexpected behavior, so does changing the context for a special event type... Mhhhh.... can't the session be retrieved from the sock somehow?

[janosbrodbeck]

Sorry for the longer delay.
It does indeed lead to unexpected behaviour. The port was the only correct value in the retrieved session.

Endpoint in sock_dtls.c, before cb call: 
Port: 5684
Family: 4 (IPV6)
Address FE:80:00:00:00:00:00:00:C8:0F:4F:FF:FE:13:0E:87

Endpoint in SOCK_ASYNC_CONN_FIN event: 
Port: 5684
Family: 0 (UNDEF)
Address: C0:3A:61:56:01:00:00:00:01:00:00:00:00:D5:A3:F1

Hm, I don't see a clean way to hand over the pointer in another way. Of course it is possible to find out which session has been closed by examining the peer list in the tinyDTLS context (which is attached to the socket). But that's inefficient and ugly.

The internal mailbox should also not be used for that I guess, especially because then the DTLS event handler must necessarily receive the message/session.

For my internal testing in gcoap I just added, as workaround, a function to the DTLS API to retrieve the session. I store the session internally at the moment. You could attach a session pointer to the DTLS socket that would be there for exactly this one event, but this is also very unexpected procedure.

[miri64]

Mh, do we maybe need another parameter to the callback :-/? But just for that? Seems a bit overkill... Mhhh...

[janosbrodbeck]

And above all, brings some changes to existing code with it. Thats, imho, really too much for just one event type.

[miri64]

I think having an extra pointer just for the FIN event in the struct seems like the lesser of two evils in that case.

[miri64]

There should be a sock_dtls_get_session() in any case, IMHO.

[janosbrodbeck]

true. That could be used for every event to retrieve the session apart from sock_dtls_receive.
I'll look into it!