[WIP] Avoid using free'd easy handles referenced from connection cache #2669
Conversation
Ideally such a pointer wouldn't still be present, but seems to happen in some circumstances. This papers over the issue--if a new handle (or other object!) is now using this memory Bad Things (tm) will happen.
These checks are redundant on the http2 path, but might be better to check here to be more "general".
|
connections are kept in the connection cache, not easy handles. Once you remove an easy handle from the multi handle it can be killed right away. That's a common pattern and one that we've always said works. When the multi interface is used, re-using easy handles is mostly just avoiding free() / malloc() since the connections, the DNS cache etc are then all owned by the multi handle anyway.
Yes, if that handle uses a live connection involved in a HTTP/1.1 pipeline - not when HTTP/2 multiplexing is used. I would urge you to try to reproduce the problem with a stand-alone example that mimics how your "real" application works but with as much as possible of the application logic removed and the libcurl logic left that causes the problems. given your log output, it looks like a |
|
Okay, will do thank you. I say easy handles are referenced from connection cache since the connection structs have a That's encouraging to hear! I'll see about creating a standalone example although it may take some time. Thanks for the quick response! |
|
the |
|
This example reliably produces the crash for me, although unsure why it's not happening on travis. But log of such a crash is included, let me know if you can reproduce or if more information is helpful. https://github.com/dtzWill/curl-repro EDIT: oh hooray travis did reproduce it at least one time, now linked from README |
|
I think it is a stale diff --git a/lib/url.c b/lib/url.c
index d29eddaea..0cab0a303 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -963,10 +963,11 @@ static bool extract_if_dead(struct connectdata *conn,
/* The check for a dead socket makes sense only if there are no
handles in pipeline and the connection isn't already marked in
use */
bool dead;
+ conn->data = data;
if(conn->handler->connection_check) {
/* The protocol has a special method for checking the state of the
connection. Use it to check if the connection is dead. */
unsigned int state;
@@ -977,11 +978,10 @@ static bool extract_if_dead(struct connectdata *conn,
/* Use the general method for determining the death of a connection */
dead = SocketIsDead(conn->sock[FIRSTSOCKET]);
}
if(dead) {
- conn->data = data;
infof(data, "Connection %ld seems to be dead!\n", conn->connection_id);
Curl_conncache_remove_conn(conn, FALSE);
return TRUE;
}
}Possible extra thing to make the pointer not point to a dead handle: diff --git a/lib/conncache.c b/lib/conncache.c
index 6bd06582a..066542915 100644
--- a/lib/conncache.c
+++ b/lib/conncache.c
@@ -449,10 +449,11 @@ bool Curl_conncache_return_conn(struct connectdata *conn)
(void)Curl_disconnect(conn_candidate, /* dead_connection */ FALSE);
}
}
CONN_LOCK(data);
conn->inuse = FALSE; /* Mark the connection unused */
+ conn->data = NULL; /* no owner */
CONN_UNLOCK(data);
return (conn_candidate == conn) ? FALSE : TRUE;
} |
|
After some testing, I tentatively report the first patch (didn't test with/without second) seems to fix the issue! Sorry for this being a PR and not an issue as seems more appropriate-- please close this when committing your suggested patch. |
|
That's just awesome! Thanks for confirming. I'll make a PR with my patches, just to make sure the CI doesn't find anything bad there and then merge. |
... and make sure to NULLify the ->data pointer when the connection is put into the cache to make this mistake easier to detect in the future. Reported-by: Will Dietz Closes #2669
dtzWill
deleted the
experimental/dont-use-invalid-easy-handles-referenced-from-conn-cache
branch
For a while now I've been trying to sort out a problem with Nix's usage of libcurl,
one that is especially apparent when building with address sanitizer enabled
or building against musl.
The commits in this PR appear to "resolve" the issue but I don't believe are really the right answer,
as they only mask the real problem: free'd handles end up being referred to from the connection cache.
The usage is this:
a queue of requests populated by other threads and for each
creates an easy handle and adds to multi.
(similar to the ASIO example or one of those, as I recall)
curl_multi_performcurl_multi_info_read, freeing the handles as we gocurl_multi_wait(we add an extra FD that's a wake-up pipe when new requests are added)We make many many requests that use http2/openssl and enable multiplexing (but not http1 pipelining).
If it matters we also enable PIPEWAIT and MAXCONNECTS is set to 25.
Eventually, and almost immediately with ASAN or quickly with musl, it appears the memory for a deallocated handle
is used for something else and everything goes south when curl attempts to "check if the connection is dead"
which involves -- for http2+ssl at least-- using data-structures and buffers that are no longer present.
I guess glibc is either less quick to recycle free'd memory or its use of thread-local arenas mostly
mitigates the problem since the thread creating/free'ing curl handles really does nothing else and so
is less likely to run into problems.
These commits workaround this by checking if the magic bit is unset, but that of course only works sometimes
and still requires accessing free'd memory and hoping for the best.
Here's an example report from address sanitizer: https://gist.github.com/dtzWill/4b02c8943ec3be4875ab54186e2cc176
I see that in general it's known to be problematic to remove handles possibly involved in pipelines,
but I thought I'd double-check that this particular problem is known/expected and to perhaps ask
if there's a recommendation for avoiding these problems.
As far as I can tell the easy handles added to a multi handle are expected to outlive the multi-handle
(although this is not mentioned elsewhere, so I'd appreciate confirmation) since they may be referenced
from the multi handle's connection cache indefinitely and there is no way to clear or query this cache.
I tried using a pool of ~100 connection handles --and basically never free them (curl_easy_cleanup) until we're entirely done.
This seems to mitigate the issue but I don't know that this can be relied upon as it may only appear to work
by making it less problematic when multi handle attempts to deference easy handles that have been removed
(since the pool ensures this memory will likely be unused for at least a while) but in the worst case
might break if the cache refers to a handle long enough for us to cycle through the pool, in which case
the connection check might do something silly like attempt to read into the buffer of a new handle
that may or may not really be part of the cached connection referencing it.
Anyway-- I suppose this is a bug report of sorts, but also a request for insights into what we're doing wrong
or how to restructure things to use the libcurl API in a more expected/supported manner.
Thank you for your time!
FWIW this issue seems particularly problematic in 7.60 (and still in latest git), it does not happen in 7.59 or the handful of previous versions I tested.