Use-after-free of conn in nopoll_conn_get_msg()

[softins]

nopoll_conn_get_msg() calls __nopoll_conn_receive(). If the connection has been closed, either by the remote party or another local thread, __nopoll_conn_receive() logs a critical error such as

(proc 11758): (critical) nopoll_conn.c:2307 received connection close while reading from conn id 4 (errno=0 : Success) (11, 11, 4), shutting down connection..

and then calls nopoll_conn_shutdown(), which frees conn.

However, when it returns 0 to nopoll_conn_get_msg(), the latter continues to use the stale conn in calls to nopoll_log(). This could result in a crash, and at the least results in a spurious error message such as

(proc 11758): (warning)nopoll_conn.c:3136 Connection id=153857472 without data, errno=0 : Success, returning no message

Suggestion would be to remove nopoll_conn_shutdown() from __nopoll_conn_receive(), also remove these lines from nopoll_conn_get_msg():

        if (bytes == 0) {
                /* connection not ready */
                nopoll_log (conn->ctx, NOPOLL_LEVEL_WARNING, "Connection id=%d without data, errno=%d : %s, returning no message",
                            conn->id, errno, strerror (errno));
                return NULL;
        }

and allow the lines just after that to shut down the connection.

However, I'm not yet familiar enough with nopoll to confidently submit a PR.

[francisbrosnan]

Hello Tony,

Thanks for reporting. Just confirm you nopoll_conn_shutdown never releases/frees (you can check it in the code). That is, nopoll_conn_shutdown never will cause memory problems (because it just updates internal states).

From there, the rest of the description must be caused by something different (passing to nopoll_conn_get_msg a reference that is already released previous, which, of cause will generate problems).

Best Regards,

[softins]

Thank you for your quick response. You are right, of course, and I said I was still learning about nopoll! It looks like the issue was another thread, when it wanted to close down the connection, was calling nopoll_conn_close(), when maybe it should be calling nopoll_conn_shutdown() instead. Changing it to do the latter seems to have overcome the problem I was having.

[francisbrosnan]

Good :-) Best Regards,