Repeatedly we see problems where using curl_multi_wait() is difficult or
just awkward because if it has no file descriptor to wait for
internally, it returns immediately and leaves it to the caller to wait
for a small amount of time in order to avoid occasional busy-looping.
This is often missed or misunderstood, leading to underperforming
applications.
This change introduces curl_multi_poll() as a replacement drop-in
function that accepts the exact same set of arguments. This function
works identically to curl_multi_wait() - EXCEPT - for the case when
there's nothing to wait for internally, as then this function will by
itself wait for a "suitable" short time before it returns. This
effectiely avoids all risks of busy-looping and should also make it less
likely that apps "over-wait".
This also changes the curl tool to use this funtion internally when
doing parallel transfers and changes curl_easy_perform() to use it
internally.
Closes#4163
It was used (intended) to pass in the size of the 'socks' array that is
also passed to these functions, but was rarely actually checked/used and
the array is defined to a fixed size of MAX_SOCKSPEREASYHANDLE entries
that should be used instead.
Closes#4169
Curl_disconnect bails out if conn->easyq is not empty, detach_connection
needs to be called first to remove the current easy from the queue.
Fixes#4144Closes#4151
- The transfer hashes weren't using the correct keys so removing entries
failed.
- Simplified the iteration logic over transfers sharing the same socket and
they now simply are set to expire and thus get handled in the "regular"
timer loop instead.
Reported-by: Tom van der Woerdt
Fixes#4012Closes#4014
Since more than one socket can be used by each transfer at a given time,
each sockhash entry how has its own hash table with transfers using that
socket.
In addition, the sockhash entry can now be marked 'blocked = TRUE'"
which then makes the delete function just set 'removed = TRUE' instead
of removing it "for real", as a way to not rip out the carpet under the
feet of a parent function that iterates over the transfers of that same
sockhash entry.
Reported-by: Tom van der Woerdt
Fixes#3961Fixes#3986Fixes#3995Fixes#4004Closes#3997
... so that timeouts or other state machine actions get going again
after a changing pause state. For example, if the last delivery was
paused there's no pending socket activity.
Reported-by: sstruchtrup on github
Fixes#3994Closes#4001
An inner loop within the singlesocket() function wrongly re-used the
variable for the outer loop which then could cause an infinite
loop. Change to using a separate variable!
Reported-by: Eric Wu
Fixes#3970Closes#3973
They need to be removed from the socket hash linked list with more care.
When sh_delentry() is called to remove a sockethash entry, remove all
individual transfers from the list first. To enable this, each Curl_easy struct
now stores a pointer to the sockethash entry to know how to remove itself.
Reported-by: Tom van der Woerdt and Kunal Ekawde
Fixes#3952Fixes#3904Closes#3953
They serve very little purpose and mostly just add noise. Most of them
have been around for a very long time. I read them all before removing
or rephrasing them.
Ref: #3876Closes#3883
Codacy/CppCheck warns about this. Consistently use parentheses as we
already do in some places to silence the warning.
Closes https://github.com/curl/curl/pull/3866
This reverts commit b0972bc.
- No longer show verbose output for the conncache closure handle.
The offending commit was added so that the conncache closure handle
would inherit verbose mode from the user's easy handle. (Note there is
no way for the user to set options for the closure handle which is why
that was necessary.) Other debug settings such as the debug function
were not also inherited since we determined that could lead to crashes
if the user's per-handle private data was used on an unexpected handle.
The reporter here says he has a debug function to capture the verbose
output, and does not expect or want any output to stderr; however
because the conncache closure handle does not inherit the debug function
the verbose output for that handle does go to stderr.
There are other plausible scenarios as well such as the user redirects
stderr on their handle, which is also not inherited since it could lead
to crashes when used on an unexpected handle.
Short of allowing the user to set options for the conncache closure
handle I don't think there's much we can safely do except no longer
inherit the verbose setting.
Bug: https://curl.haxx.se/mail/lib-2019-05/0021.html
Reported-by: Kristoffer Gleditsch
Ref: https://github.com/curl/curl/pull/3598
Ref: https://github.com/curl/curl/pull/3618
Closes https://github.com/curl/curl/pull/3856
As soon as a TLS backend gets ALPN conformation about the specific HTTP
version it can now set the multiplex situation for the "bundle" and
trigger moving potentially queued up transfers to the CONNECT state.
With transfers being queued up, we only move one at a a time back to the
CONNECT state but now we mark moved transfers so that when a moved
transfer is confirmed "successful" (it connected) it will trigger the
move of another pending transfer. Previously, it would otherwise wait
until the transfer was done before doing this. This makes queued up
pending transfers get processed (much) faster.
Remove the code too. The functionality has been disabled in code since
7.62.0. Setting this option will from now on simply be ignored and have
no function.
Closes#3654
Fixes#3745Closes#3746
The following snippet
```
int main()
{
CURL* hCurlHandle = curl_easy_init();
curl_easy_setopt(hCurlHandle, CURLOPT_URL, "http://example.com");
curl_easy_setopt(hCurlHandle, CURLOPT_PROXY, "1");
curl_easy_perform(hCurlHandle);
curl_easy_cleanup(hCurlHandle);
return 0;
}
```
triggers the following Valgrind warning
```
==4125== Invalid read of size 8
==4125== at 0x4E7D1EE: Curl_llist_remove (llist.c:97)
==4125== by 0x4E7EF5C: detach_connnection (multi.c:798)
==4125== by 0x4E80545: multi_runsingle (multi.c:1451)
==4125== by 0x4E8197C: curl_multi_perform (multi.c:2072)
==4125== by 0x4E766A0: easy_transfer (easy.c:625)
==4125== by 0x4E76915: easy_perform (easy.c:719)
==4125== by 0x4E7697C: curl_easy_perform (easy.c:738)
==4125== by 0x4008BE: main (in /home/even/curl/test)
==4125== Address 0x9b3d1d0 is 1,120 bytes inside a block of size 1,600 free'd
==4125== at 0x4C2ECF0: free (vg_replace_malloc.c:530)
==4125== by 0x4E62C36: conn_free (url.c:756)
==4125== by 0x4E62D34: Curl_disconnect (url.c:818)
==4125== by 0x4E48DF9: Curl_once_resolved (hostip.c:1097)
==4125== by 0x4E8052D: multi_runsingle (multi.c:1446)
==4125== by 0x4E8197C: curl_multi_perform (multi.c:2072)
==4125== by 0x4E766A0: easy_transfer (easy.c:625)
==4125== by 0x4E76915: easy_perform (easy.c:719)
==4125== by 0x4E7697C: curl_easy_perform (easy.c:738)
==4125== by 0x4008BE: main (in /home/even/curl/test)
==4125== Block was alloc'd at
==4125== at 0x4C2F988: calloc (vg_replace_malloc.c:711)
==4125== by 0x4E6438E: allocate_conn (url.c:1654)
==4125== by 0x4E685B4: create_conn (url.c:3496)
==4125== by 0x4E6968F: Curl_connect (url.c:4023)
==4125== by 0x4E802E7: multi_runsingle (multi.c:1368)
==4125== by 0x4E8197C: curl_multi_perform (multi.c:2072)
==4125== by 0x4E766A0: easy_transfer (easy.c:625)
==4125== by 0x4E76915: easy_perform (easy.c:719)
==4125== by 0x4E7697C: curl_easy_perform (easy.c:738)
==4125== by 0x4008BE: main (in /home/even/curl/test)
```
This has been bisected to commit 2f44e94
Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=14109
Credit to OSS Fuzz
* Adjusted unit tests 2056, 2057
* do not generally close connections with CURLAUTH_NEGOTIATE after every request
* moved negotiatedata from UrlState to connectdata
* Added stream rewind logic for CURLAUTH_NEGOTIATE
* introduced negotiatedata::GSS_AUTHDONE and negotiatedata::GSS_AUTHSUCC
* Consider authproblem state for CURLAUTH_NEGOTIATE
* Consider reuse_forbid for CURLAUTH_NEGOTIATE
* moved and adjusted negotiate authentication state handling from
output_auth_headers into Curl_output_negotiate
* Curl_output_negotiate: ensure auth done is always set
* Curl_output_negotiate: Set auth done also if result code is
GSS_S_CONTINUE_NEEDED/SEC_I_CONTINUE_NEEDED as this result code may
also indicate the last challenge request (only works with disabled
Expect: 100-continue and CURLOPT_KEEP_SENDING_ON_ERROR -> 1)
* Consider "Persistent-Auth" header, detect if not present;
Reset/Cleanup negotiate after authentication if no persistent
authentication
* apply changes introduced with #2546 for negotiate rewind logic
Fixes#1261Closes#1975
This code was once used for the non multi-interface using code path, but
ever since easy_perform was turned into a wrapper around the multi
interface, this code path never runs.
Closes#3666
- Change closure handle to receive verbose setting from the easy handle
most recently added via curl_multi_add_handle.
The closure handle is a special easy handle used for closing cached
connections. It receives limited settings from the easy handle most
recently added to the multi handle. Prior to this change that did not
include verbose which was a problem because on connection shutdown
verbose mode was not acknowledged.
Ref: https://github.com/curl/curl/pull/3598
Co-authored-by: Daniel Stenberg
Closes https://github.com/curl/curl/pull/3618
Failing to do so would make the CURLINFO_TOTAL_TIME timeout to not get
updated correctly and could end up getting reported to the application
completely wrong (way too small).
Reported-by: accountantM on github
Fixes#3602Closes#3605
The variable wasn't properly reset within the loop and thus could remain
set for sockets that hadn't been set before and miss notifying the app.
This is a follow-up to 4c35574 (shipped in curl 7.64.0)
Reported-by: buzo-ffm on github
Detected-by: Jan Alexander Steffens
Fixes#3585Closes#3589
urlapi: turn three local-only functions into statics
conncache: make conncache_find_first_connection static
multi: make detach_connnection static
connect: make getaddressinfo static
curl_ntlm_core: make hmac_md5 static
http2: make two functions static
http: make http_setup_conn static
connect: make tcpnodelay static
tests: make UNITTEST a thing to mark functions with, so they can be static for
normal builds and non-static for unit test builds
... and mark Curl_shuffle_addr accordingly.
url: make up_free static
setopt: make vsetopt static
curl_endian: make write32_le static
rtsp: make rtsp_connisdead static
warnless: remove unused functions
memdebug: remove one unused function, made another static
We use "conn" everywhere to be a pointer to the connection.
Introduces two functions that "attaches" and "detaches" the connection
to and from the transfer.
Going forward, we should favour using "data->conn" (since a transfer
always only has a single connection or none at all) to "conn->data"
(since a connection can have none, one or many transfers associated with
it and updating conn->data to be correct is error prone and a frequent
reason for internal issues).
Closes#3442
Fixes#3436Closes#3448
Problem 1
After LOTS of scratching my head, I eventually realized that even when doing
10 uploads in parallel, sometimes the socket callback to the application that
tells it what to wait for on the socket, looked like it would reflect the
status of just the single transfer that just changed state.
Digging into the code revealed that this was indeed the truth. When multiple
transfers are using the same connection, the application did not correctly get
the *combined* flags for all transfers which then could make it switch to READ
(only) when in fact most transfers wanted to get told when the socket was
WRITEABLE.
Problem 1b
A separate but related regression had also been introduced by me when I
cleared connection/transfer association better a while ago, as now the logic
couldn't find the connection and see if that was marked as used by more
transfers and then it would also prematurely remove the socket from the socket
hash table even in times other transfers were still using it!
Fix 1
Make sure that each socket stored in the socket hash has a "combined" action
field of what to ask the application to wait for, that is potentially the ORed
action of multiple parallel transfers. And remove that socket hash entry only
if there are no transfers left using it.
Problem 2
The socket hash entry stored an association to a single transfer using that
socket - and when curl_multi_socket_action() was called to tell libcurl about
activities on that specific socket only that transfer was "handled".
This was WRONG, as a single socket/connection can be used by numerous parallel
transfers and not necessarily a single one.
Fix 2
We now store a list of handles in the socket hashtable entry and when libcurl
is told there's traffic for a particular socket, it now iterates over all
known transfers using that single socket.
Added Curl_resolver_kill() for all three resolver modes, which only
blocks when necessary, along with test 1592 to confirm
curl_multi_remove_handle() doesn't block unless it must.
Closes#3428Fixes#3371
Do not assume/store assocation between a given easy handle and the
connection if it can be avoided.
Long-term, the 'conn->data' pointer should probably be removed as it is a
little too error-prone. Still used very widely though.
Reported-by: masbug on github
Fixes#3391Closes#3400
The time_t type is unsigned on some systems and these variables are used
to hold return values from functions that return timediff_t
already. timediff_t is always a signed type.
Closes#3363
This is a companion patch to cbea2fd2c (NTLM: force the connection to
HTTP/1.1, 2018-12-06): with NTLM, we can switch to HTTP/1.1
preemptively. However, with other (Negotiate) authentication it is not
clear to this developer whether there is a way to make it work with
HTTP/2, so let's try HTTP/2 first and fall back in case we encounter the
error HTTP_1_1_REQUIRED.
Note: we will still keep the NTLM workaround, as it avoids an extra
round trip.
Daniel Stenberg helped a lot with this patch, in particular by
suggesting to introduce the Curl_h2_http_1_1_error() function.
Closes#3349
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
curl_multi_wait() was erroneously used from within
curl_easy_perform(). It could lead to it believing there was no socket
to wait for and then instead sleep for a while instead of monitoring the
socket and then miss acting on that activity as swiftly as it should
(causing an up to 1000 ms delay).
Reported-by: Antoni Villalonga
Fixes#3305Closes#3306Closes#3308
The function does not return the same value as snprintf() normally does,
so readers may be mislead into thinking the code works differently than
it actually does. A different function name makes this easier to detect.
Reported-by: Tomas Hoger
Assisted-by: Daniel Gustafsson
Fixes#3296Closes#3297
When using c-ares for asyn dns, the dns socket fd was silently closed
by c-ares without curl being aware. curl would then 'realize' the fd
has been removed at next call of Curl_resolver_getsock, and only then
notify the CURLMOPT_SOCKETFUNCTION to remove fd from its poll set with
CURL_POLL_REMOVE. At this point the fd is already closed.
By using ares socket state callback (ARES_OPT_SOCK_STATE_CB), this
patch allows curl to be notified that the fd is not longer needed
for neither for write nor read. At this point by calling
Curl_multi_closed we are able to notify multi with CURL_POLL_REMOVE
before the fd is actually closed by ares.
In asyn-ares.c Curl_resolver_duphandle we can't use ares_dup anymore
since it does not allow passing a different sock_state_cb_data
Closes#3238
Curl_follow() no longer frees the string. Make sure it happens in the
caller function, like we normally handle allocations.
This bug was introduced with the use of the URL API internally, it has
never been in a release version
Reported-by: Dario Weißer
Closes#3149
