Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connection upkeep function doesn't attach Curl_easy to each connection #7386

Closed
Josie-H opened this issue Jul 13, 2021 · 0 comments
Closed

Connection upkeep function doesn't attach Curl_easy to each connection #7386

Josie-H opened this issue Jul 13, 2021 · 0 comments
Labels

Comments

@Josie-H
Copy link
Contributor

@Josie-H Josie-H commented Jul 13, 2021

I did this

Had code running curl_easy_upkeep() for connection upkeep, using libcurl 7.75.0. At a couple of customers, we started to hear reports of segfaults killing the application that was using Curl, and core dumps showed this happening during connection upkeep.

I debugged the bug

After turning on debug symbols, I realised that functions called by the connection check expect "Curl_easy data" and "connectdata conn" to be connected to each other. In 7.75.0 that means that data->conn=conn, and conn->data=data. (I see that in more recent versions the second requirement has been removed.) In these crashes, data->conn was NULL.

In case you're interested in why this pointer is required to be set, the stack for one of the crashes looks like:
ossl_send (vtls/openssl.c:4174) (attempts to dereference data->conn)
send_callback (http2.c:362)
nghttp2_session_send (nghttp2_session.c:3258)
http2_conncheck (http2.c:250)
http2_conncheck (http2.c:214)
conn_upkeep (easy.c:1230)
Curl_conncache_foreach (conncache.c:352)
upkeep (easy.c:1258)

I fixed the bug

I found similar function extract_if_dead() (url.c) where we're also looping through connections in the connection cache and doing something to each of them. That one just does Curl_attach_connnection() before checking the connection and Curl_detach_connnection() afterwards. I figure that's probably the solution in conn_upkeep() too.

The one thing I'm not quite sure about (and would definitely appreciate feedback on) is whether we'd expect that data->conn might be set to anything else important at the point of calling the upkeep function. So might there be anything in that field that we should store off and reset back later? In my current patch for our customers I'm doing that just to be safe, but for the moment I'm only submitting a pull request here with the simpler code, since that's what extract_if_dead does. But I'm prepared to believe that actually both of them should be storing/restoring a previous value if present.

Josie-H added a commit to Josie-H/curl that referenced this issue Jul 13, 2021
During the protocol-specific parts of connection upkeep, some code 
assumes that the data->conn pointer already is set correctly.  However, 
there's currently no guarantee of that in the code.

This fix temporarily attaches each connection to the Curl_easy object 
before performing the protocol-specific connection check on it, in a 
similar manner to the connection checking in extract_if_dead().

Fixes curl#7386
Reported-by: Josie-H
@jay jay added the crash label Jul 14, 2021
@bagder bagder closed this in c12ad2d Jul 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants