Remove unnecessary BUG_ON #1984
Conversation
This BUG_ON can be removed, because `cli_conn->refcnt` can be below then two at this place. For instance in case when socket was stolen and after it client connection was aborted. Therefore server connection will be upgraded for invalid client connection. It's not a good case for us but not critical. We can add check that client connection is still alive in `tfw_ws_srv_new_steal_sk()`, but it will not fix the problem completely.
There was a problem hiding this comment.
This is very subtle place and we might observe use-after-free crashes - probably something is completely wrong, something that wasn't considered in #1797. I think to fix the issue we need to consider #1968 crash along with #1647 and review the #1797 fixes - maybe some of the decisions were wrong. But just removing the assertion looks scary.
| @@ -197,7 +197,6 @@ tfw_http_websocket_upgrade(TfwSrvConn *srv_conn, TfwCliConn *cli_conn) | |||
| */ | |||
| cli_conn->pair = (TfwConn *)ws_conn; | |||
| tfw_connection_get(cli_conn->pair); | |||
| BUG_ON(atomic_read(&cli_conn->refcnt) < 2); | |||
There was a problem hiding this comment.
I'm afraid that with this change we might observe referencing the dead client connection as in #1647 we tried to send a response on a dead client connection. This assertion was introduced in #1797 and we can not drop a reference count on the client connection because we have not forwarded response on the connection (the comment above references tfw_http_conn_drop() for this logic).
Which function calls sequence leads to cli_conn->refcnt < 2 and what's its value (I hope not 0)?
This BUG_ON can be removed, because
cli_conn->refcntcan be below then two at this place. For instance in case when socket was stolen and after it client connection was aborted. Therefore server connection will be upgraded for invalid client connection. It's not a good case for us but not critical. We can add check that client connection is still alive intfw_ws_srv_new_steal_sk(), but it will not fix the problem completely.