Correctly delist request on response parsing errors

[vankoven]

When a server sends invalid response, Tempesta sends an error
message to the client and destroys the request. But if the request
was the only request in server connection fwd_queue, msg_sent
is not updated and contains invalid pointer. Later during server
connection repair this invalid pointer is used and a crash may happen.

Steps to reproduce the issue:

1. Configure exactly one server connection: server 127.0.0.1:8080
2. Send a request
3. Force server to send invalid request
4. Send another request, most likely that crash will happen

[keshonok]

I think the idea was that both req and srv_conn were known already so there's no need to get/derive them again. The called function is inlined and so the values are just used in place. Perhaps, GCC optimizes this just as needed, and there's no "calculation" of these values again.

[keshonok]

Also, is it possible that this situation can happen in other places as well? This particular function is only called once. The unlocked version is called in all other cases.

[vankoven]

@keshonok Thank you for review!

In "hot path" functions we normally try to reduce number of calculations, but this sometimes makes function readability worse. Take a look at this mostly obsolete (and resolved) example. Function takes two parameters: int f(TfwHttpReq *req, TfwHttpResp *resp). But in the same time we know, that we can get req as resp->req or resp as req->resp. So why the two different pointers are passed here? Is it the same req as saved in resp->req or not?

It isn't a hot path function, it's very and very unlikely situation instead. We think about of protected servers as reliable and good servers, so we actually expect server to send a valid message, not a garbage that can't be parsed or processed. So extra calculations (if happen) doesn't hurt much.

In the same time, the function is self-sufficient, it takes care of server connection forwarding queue, and keeps it to be consistent (see #1133 (comment) ).

That is why I thought that extra calculations doesn't hurt and being more explicit helps to make understanding of the function role (readability) better. But taking into account review comments, it seems to be partly done, I should rename the function and add a good comment description to mirror function role.

[keshonok]

Incidentally, this is exactly what's done in tfw_http_popreq(). If this change is here to stay, then this sequence in that function can be replaced with a call to tfw_http_req_delist().

I think the question I have is whether this check should be here or in _tfw_http_req_delist(). If it's the latter, then are there cases where this check is unnecessary?

[vankoven]

@keshonok Thanks for your review!

Functions tfw_http_req_delist() and __http_req_delist() are really looks like a locked/unlocked pair. But the meaning of the lock is very complicated here, it's not for just safe list operations. Absence of the lock (__http_req_delist()) means that the function simply delists one singe request from the forward queue, while the caller takes care of the full state of the forward queue.

"Locked" function (tfw_http_req_delist()) - has other meaning. It takes takes care of the full state of the forwarding queue. The queue is consistent before and after the call, no additional actions are required.

That is why there is no need to move msg_sent condition into __http_req_delist(): the function is called as a part of connection repair and request forwarding operations. Both has very specific msg_sent management. I think the best solution here is to rename tfw_http_req_delist() and add add a good description for the function, to show why it's so different from __http_req_delist().

[vankoven]

Added function description, to make it more explicit and easy to understand.

[keshonok]

Frankly, the new name looks rather long and unattractive while not really telling much. Perhaps it's best to leave it as it is. Requests are always removed in the order they come, from top. The updated comment to the function pretty much tells all there's to know about specifics of it.

Also, tfw_http_req_ is a common prefix for functions operating on requests, which is kind of broken with this new name.

I am still wondering what's the difference with what's done in tfw_http_popreq() at a slightly later stage. Special care is taken there in regards to NIP requests - the case where a non-idempotent request may become idempotent.

[aleksostapenko]

Good catch! Looks good to me, with minor correction.

[aleksostapenko]

@srv_conn->msg sent -> @srv_conn->msg_sent

[krizhanovsky]

fwd_unsent is used to execute the small code above only and exit the function. Moreover, the code above is http_req_delist() with zeroing and tfw_http_conn_nip_adjust() call. I propose to leave http_req_delist() as

http_req_delist()
{
    if ((TfwMsg *)req == srv_conn->msg_sent)
        srv_conn->msg_sent = NULL;
    __http_req_delist(srv_conn, req);
    tfw_http_conn_nip_adjust(srv_conn);

call it here and define

http_req_delist_locked()
{
    spin_lock(&srv_conn->fwd_qlock);
    http_req_delist();
    spin_unlock(&srv_conn->fwd_qlock);
}

and call it instead of tfw_http_popreq( .., false)

[krizhanovsky]

Good to merge after a small cleanup to make the code more reader-friendly. If you find fwd_unsent more attractive and want to leave with it, I don't mind. If so, then just rename __http_req_delist() to http_req_delist() since we don't have the second version of the function.