Well-behaving HTTP2 servers send two GOAWAY messages. The first
message is a warning that indicates that the server is going to
stop accepting streams. The second one actually closes the stream.
nghttp2 reports this state (and the other state of no more stream
identifiers) via the call nghttp2_session_check_request_allowed().
In this state the client should not create more streams on the
session (tcp connection), and in curl this means that the server
has requested that the connection is closed.
It would be also be possible to put the connclose() call into the
on_http2_frame_recv() function that triggers on the GOAWAY message.
This fixes a bug seen when the client sees the following sequence of
frames:
// advisory GOAWAY
HTTP2 GOAWAY [stream-id = 0, promised-stream-id = -1]
... some additional frames
// final GOAWAY
HTTP2 GOAWAY [stream-id = 0, promised-stream-id = N ]
Before this change, curl will attempt to reuse the connection even
after the last stream, will encounter this error:
* Found bundle for host localhost: 0x5595f0a694e0 [can multiplex]
* Re-using existing connection! (#0) with host localhost
* Connected to localhost (::1) port 10443 (#0)
* Using Stream ID: 9 (easy handle 0x5595f0a72e30)
> GET /index.html?5 HTTP/2
> Host: localhost:10443
> user-agent: curl/7.68.0
> accept: */*
>
* stopped the pause stream!
* Connection #0 to host localhost left intact
curl: (16) Error in the HTTP2 framing layer
This error may posion the connection cache, causing future requests
which resolve to the same curl connection to go through the same error
path.
Closes#5643
Confusingly, nghttp2 has two different error code enums:
- nghttp2_error, to be used with nghttp2_strerror
- nghttp2_error_code, to be used with nghttp2_http2_strerror
Closes#5641
Since commit f3d501dc67, if proxy support is disabled, MSVC warns:
url.c : warning C4701: potentially uninitialized local variable
'hostaddr' used
url.c : error C4703: potentially uninitialized local pointer variable
'hostaddr' used
That could actually only happen if both `conn->bits.proxy` and
`CURL_DISABLE_PROXY` were enabled.
Initialize it to NULL to silence the warning.
Closes https://github.com/curl/curl/pull/5638
Updated terminology in docs, comments and phrases to refer to C strings
as "null-terminated". Done to unify with how most other C oriented docs
refer of them and what users in general seem to prefer (based on a
single highly unscientific poll on twitter).
Reported-by: coinhubs on github
Fixes#5598Closes#5608
Don't reference fields that do not exist. Fixes build failure:
vtls/mbedtls.c: In function 'mbed_connect_step1':
vtls/mbedtls.c:249:54: error: 'struct connectdata' has no member named 'http_proxy'
Closes#5615
...previously CURLINFO_EFFECTIVE_URL would report the URL of the
original "mother transfer", not the actually pushed resource.
Reported-by: Jonathan Cardoso Machado
Fixes#5589Closes#5591
- Include wincrypt before OpenSSL includes so that the latter can
properly handle any conflicts between the two.
Closes https://github.com/curl/curl/pull/5606
Replace "Failed writing body (X != Y)" with
"Failure writing output to destination". Possibly slightly less cryptic.
Reported-by: coinhubs on github
Fixes#5594Closes#5596
Follow-up to c4e6968127
When a new transfer is created, as a resuly of an acknowledged push,
that transfer needs a download buffer allocated.
Closes#5590
This commit changes the behavior of CURLSSLOPT_NATIVE_CA so that it does
not override CURLOPT_CAINFO / CURLOPT_CAPATH, or the hardcoded default
locations. Instead the CA store can now be used at the same time.
The change is due to the impending release. The issue is still being
discussed. The behavior of CURLSSLOPT_NATIVE_CA is subject to change and
is now documented as experimental.
Ref: bc052cc (parent commit)
Ref: https://github.com/curl/curl/issues/5585
For QUIC but also for regular TCP when the second family runs out of IPs
with a failure while the first family is still trying to connect.
Separated the timeout handling for IPv4 and IPv6 connections when they
both have a number of addresses to iterate over.
This avoids using a pair of TCP ports to provide wakeup functionality
for every multi instance on Windows, where socketpair() is emulated
using a TCP socket on loopback which could in turn lead to socket
resource exhaustion.
Reviewed-by: Gergely Nagy
Reviewed-by: Marc Hörsken
Closes#5397
When wolfSSL is built with its OpenSSL API layer, it fetures the same DES*
functions that OpenSSL has. This change take advantage of that.
Co-authored-by: Daniel Stenberg
Closes#5556Fixes#5548
Since the connection can be used by many independent requests (using
HTTP/2 or HTTP/3), things like user-agent and other transfer-specific
data MUST NOT be kept connection oriented as it could lead to requests
getting the wrong string for their requests. This struct data was
lingering like this due to old HTTP1 legacy thinking where it didn't
mattered..
Fixes#5566Closes#5567
When asking for a specific feature to be shared in the share object,
that bit was previously set unconditionally even if the shared feature
failed or otherwise wouldn't work.
Closes#5554
Instead of discussing if there's value or meaning (implied or not) in
the colors, let's use words without the same possibly negative
associations.
Closes#5546
The SOCKS4/5 state machines weren't properly terminated when the proxy
connection got closed, leading to a busy-loop.
Reported-By: zloi-user on github
Fixes#5532Closes#5542
To reduce the amount of allocations needed for creating a Curl_addrinfo
struct, make a single larger malloc instead of three separate smaller
ones.
Closes#5533
quiche now requires the application to explicitly set the keylog path
for each connection, rather than reading the environment variable
itself.
Closes#5541
Now that all functions in select.[ch] take timediff_t instead
of the limited int or long, we can remove type conversions
and related preprocessor checks to silence compiler warnings.
Avoiding conversions from time_t was already done in 842f73de.
Based upon #5262
Supersedes #5214, #5220 and #5221
Follow up to #5343 and #5479Closes#5490
On some systems, openssl 1.0.x is still the default, but it has been
patched to contain all the recent security fixes. As a result of this
patching, it is possible for macro X509_V_FLAG_NO_ALT_CHAINS to be
defined, while the previous behavior of openssl to not look at trusted
chains first, remains.
Fix it: ensure X509_V_FLAG_TRUSTED_FIRST is always set, do not try to
probe for the behavior of openssl based on the existence ofmacros.
Closes#5530
Commit 4a4b63d forgot to set the expected SOCKS5 reply length when the
reply ATYP is X'01'. This resulted in erroneously expecting more bytes
when the request length is greater than the reply length (e.g., when
remotely resolving the hostname).
Closes#5527
When the method is updated inside libcurl we must still not change the
method as set by the user as then repeated transfers with that same
handle might not execute the same operation anymore!
This fixes the libcurl part of #5462
Test 1633 added to verify.
Closes#5499
`http_proxy` will not be available in `conndata` if `CURL_DISABLE_PROXY`
is enabled. Repair the build with that configuration.
Follow-up to f3d501dc67Closes#5498
"Null-checking k->str suggests that it may be null, but it has already
been dereferenced on all paths leading to the check" - and it can't
legally be NULL at this point. Remove check.
Detected by Coverity CID 1463884
Closes#5495
Since Win32 almost always will also have USE_WINSOCK,
we can reduce complexity and always use Sleep there.
Assisted-by: Jay Satiro
Reviewed-by: Daniel Stenberg
Follow up to #5343Closes#5489
... and free it as soon as the transfer is done. It removes the extra
alloc when a new size is set with setopt() and reduces memory for unused
easy handles.
In addition: the closure_handle now doesn't use an allocated buffer at
all but the smallest supported size as a stack based one.
Closes#5472
Using time_t and suseconds_t if suseconds_t is available,
long on Windows (maybe others in the future) and int elsewhere.
Also handle case of ULONG_MAX being greater or equal to INFINITE.
Assisted-by: Jay Satiro
Reviewed-by: Daniel Stenberg
Part of #5343
Make all functions in select.[ch] take timeout_ms as timediff_t
which should always be large enough and signed on all platforms
to take all possible timeout values and avoid type conversions.
Reviewed-by: Jay Satiro
Reviewed-by: Daniel Stenberg
Replaces #5107 and partially #5262
Related to #5240 and #5286Closes#5343
Tested with ngtcp2 built against the OpenSSL library. Additionally
tested with MultiSSL (NSS for TLS and ngtcp2+OpenSSL for QUIC).
The TLS backend (independent of QUIC) may or may not already have opened
the keylog file before. Therefore Curl_tls_keylog_open is always called
to ensure the file is open.
Tested following the same curl and tshark commands as in commit
"vtls: Extract and simplify key log file handling from OpenSSL" using
WolfSSL v4.4.0-stable-128-g5179503e8 from git master built with
`./configure --enable-all --enable-debug CFLAGS=-DHAVE_SECRET_CALLBACK`.
Full support for this feature requires certain wolfSSL build options,
see "Availability note" in lib/vtls/wolfssl.c for details.
Closes#5327
Create a set of routines for TLS key log file handling to enable reuse
with other TLS backends. Simplify the OpenSSL backend as follows:
- Drop the ENABLE_SSLKEYLOGFILE macro as it is unconditionally enabled.
- Do not perform dynamic memory allocation when preparing a log entry.
Unless the TLS specifications change we can suffice with a reasonable
fixed-size buffer.
- Simplify state tracking when SSL_CTX_set_keylog_callback is
unavailable. My original sslkeylog.c code included this tracking in
order to handle multiple calls to SSL_connect and detect new keys
after renegotiation (via SSL_read/SSL_write). For curl however we can
be sure that a single master secret eventually becomes available
after SSL_connect, so a simple flag is sufficient. An alternative to
the flag is examining SSL_state(), but this seems more complex and is
not pursued. Capturing keys after server renegotiation was already
unsupported in curl and remains unsupported.
Tested with curl built against OpenSSL 0.9.8zh, 1.0.2u, and 1.1.1f
(`SSLKEYLOGFILE=keys.txt curl -vkso /dev/null https://localhost:4433`)
against an OpenSSL 1.1.1f server configured with:
# Force non-TLSv1.3, use TLSv1.0 since 0.9.8 fails with 1.1 or 1.2
openssl s_server -www -tls1
# Likewise, but fail the server handshake.
openssl s_server -www -tls1 -Verify 2
# TLS 1.3 test. No need to test the failing server handshake.
openssl s_server -www -tls1_3
Verify that all secrets (1 for TLS 1.0, 4 for TLS 1.3) are correctly
written using Wireshark. For the first and third case, expect four
matches per connection (decrypted Server Finished, Client Finished, HTTP
Request, HTTP Response). For the second case where the handshake fails,
expect a decrypted Server Finished only.
tshark -i lo -pf tcp -otls.keylog_file:keys.txt -Tfields \
-eframe.number -eframe.time -etcp.stream -e_ws.col.Info \
-dtls.port==4433,http -ohttp.desegment_body:FALSE \
-Y 'tls.handshake.verify_data or http'
A single connection can easily be identified via the `tcp.stream` field.
For HTTP 1.x, it's a protocol error when the server sends more bytes
than announced. If this happens, don't reuse the connection, because the
start position of the next response is undefined.
Closes#5440
When USE_RESOLVE_ON_IPS is set (defined on macOS), it means that
numerical IP addresses still need to get "resolved" - but not with DoH.
Reported-by: Viktor Szakats
Fixes#5454Closes#5459
They're only limited to the maximum string input restrictions, not to
256 bytes.
Added test 1178 to verify
Reported-by: Will Roberts
Fixes#5448Closes#5449
Fixed the alt-svc parser to treat a newline as end of line.
The unit tests in test 1654 were done without CRLF and thus didn't quite
match the real world. Now they use CRLF as well.
Reported-by: Peter Wu
Assisted-by: Peter Wu
Assisted-by: Jay Satiro
Fixes#5445Closes#5446
This reverts commit 74623551f3.
Instead mark the function call with (void). Getting the return code and
using it instead triggered Coverity warning CID 1463596 because
snprintf() can return a negative value...
Closes#5441
... as returning a "" is not a good idea as the string is supposed to be
allocated and returning a const string will cause issues.
Reported-by: Brian Carpenter
Follow-up to ed35d6590eCloses#5405
This change introduces a generic way to provide binary data in setopt
options, called BLOBs.
This change introduces these new setopts:
CURLOPT_ISSUERCERT_BLOB, CURLOPT_PROXY_SSLCERT_BLOB,
CURLOPT_PROXY_SSLKEY_BLOB, CURLOPT_SSLCERT_BLOB and CURLOPT_SSLKEY_BLOB.
Reviewed-by: Daniel Stenberg
Closes#5357
- Stick to a single unified way to use structs
- Make checksrc complain on 'typedef struct {'
- Allow them in tests, public headers and examples
- Let MD4_CTX, MD5_CTX, and SHA256_CTX typedefs remain as they actually
typedef different types/structs depending on build conditions.
Closes#5338
Previously, after PASV and immediately after the data connection has
connected, the function would only return the control socket to wait for
which then made the data connection simply timeout and not get polled
correctly. This become obvious when running test 1631 and 1632 event-
based.
Use them only if `_UNICODE` is defined, in which case command-line
arguments have been converted to UTF-8.
Closes https://github.com/curl/curl/pull/3784
- use `wmain` instead of `main` when `_UNICODE` is defined [0]
- define `argv_item_t` as `wchar_t *` in this case
- use the curl_multibyte gear to convert the command-line arguments to
UTF-8
This makes it possible to pass parameters with characters outside of
the current locale on Windows, which is required for some tests, e.g.
the IDN tests. Out of the box, this currently only works with the
Visual Studio project files, which default to Unicode, and winbuild
with the `ENABLE_UNICODE` option.
[0] https://devblogs.microsoft.com/oldnewthing/?p=40643
Ref: https://github.com/curl/curl/issues/3747
Closes https://github.com/curl/curl/pull/3784
Fix theoretical integer overflow in Curl_auth_create_plain_message.
The security impact of the overflow was discussed on hackerone. We
agreed this is more of a theoretical vulnerability, as the integer
overflow would only be triggerable on systems using 32-bits size_t with
over 4GB of available memory space for the process.
Closes#5391
Since input passed to libcurl with CURLOPT_USERPWD and
CURLOPT_PROXYUSERPWD circumvents the regular string length check we have
in Curl_setstropt(), the input length limit is enforced in
Curl_parse_login_details too, separately.
Reported-by: Thomas Bouzerar
Closes#5383
When looking for a protocol match among supported schemes, check the
most "popular" schemes first. It has zero functionality difference and
for all practical purposes a speed difference will not be measureable
but it still think it makes sense to put the least likely matches last.
"Popularity" based on the 2019 user survey.
Closes#5377
Add three new CMake Find modules (using the curl license, but I grant
others the right to apply the CMake BSD license instead).
This CMake config is simpler than the autotools one because it assumes
ngtcp2 and nghttp3 to be used together. Another difference is that this
CMake config checks whether QUIC is actually supported by the TLS
library (patched OpenSSL or boringssl) since this can be a common
configuration mistake that could result in build errors later.
Unlike autotools, CMake does not warn you that the features are
experimental. The user is supposed to already know that and read the
documentation. It requires a very special build environment anyway.
Tested with ngtcp2+OpenSSL+nghttp3 and quiche+boringssl, both built from
current git master. Use `LD_DEBUG=files src/curl |& grep need` to figure
out which features (libldap-2.4, libssh2) to disable due to conflicts
with boringssl.
Closes#5359
If the QLOGDIR environment variable is set, enable qlogging.
... and create Curl_qlogdir() in the new generic vquic/vquic.c file for
QUIC functions that are backend independent.
Closes#5353
That return code is reserved for build-time conditional code not being
present while this was a regular run-time error from a Windows API.
Reported-by: wangp on github
Fixes#5349Closes#5350
Triggered by a crash detected by OSS-Fuzz after the dynbuf introduction in
ed35d6590e. This should make the trailer handling more straight forward and
hopefully less error-prone.
Deliver the trailer header to the callback already at receive-time. No
longer caches the trailers to get delivered at end of stream.
Bug: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=22030Closes#5348
In my very basic test that lists sftp://127.0.0.1/tmp/, this patched
code makes 161 allocations compared to 194 in git master. A 17%
reduction.
Closes#5336
quiche has the potential to log qlog files. To enable this, you must
build quiche with the qlog feature enabled `cargo build --features
qlog`. curl then passes a file descriptor to quiche, which takes
ownership of the file. The FD transfer only works on UNIX.
The convention is to enable logging when the QLOGDIR environment is
set. This should be a path to a folder where files are written with the
naming template <SCID>.qlog.
Co-authored-by: Lucas Pardue
Replaces #5337Closes#5341
A common set of functions instead of many separate implementations for
creating buffers that can grow when appending data to them. Existing
functionality has been ported over.
In my early basic testing, the total number of allocations seem at
roughly the same amount as before, possibly a few less.
See docs/DYNBUF.md for a description of the API.
Closes#5300
- Check for NULL entry parameter before attempting to deref entry in
Curl_resolver_is_resolved, like is already done in asyn-ares.
This is to silence cppcheck which does not seem to understand that
asyn-ares and asyn-thread have separate Curl_resolver_is_resolved
and those units are mutually exclusive. Prior to this change it warned
of a scenario where asyn-thread's Curl_resolver_is_resolved is called
with a NULL entry from asyn-ares, but that couldn't happen.
Reported-by: rl1987@users.noreply.github.com
Fixes https://github.com/curl/curl/issues/5326
More connection cache accesses are protected by locks.
CONNCACHE_* is a beter prefix for the connection cache lock macros.
Curl_attach_connnection: now called as soon as there's a connection
struct available and before the connection is added to the connection
cache.
Curl_disconnect: now assumes that the connection is already removed from
the connection cache.
Ref: #4915Closes#5009
Regression since 7.69.0 and 68fb25fa3f.
The code wrongly assigned 'from' instead of 'auth' which probably was a
copy and paste mistake from other code, leading to that auth could
remain NULL and later cause an error to be returned.
Assisted-by: Eric Sauvageau
Fixes#5294Closes#5295
Previously, options set explicitly through command line options could be
overridden by the configuration files parsed automatically when
ssh_connect() was called.
By calling ssh_options_parse_config() explicitly, the configuration
files are parsed before setting the options, avoiding the options
override. Once the configuration files are parsed, the automatic
configuration parsing is not executed.
Fixes#4972Closes#5283
Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>
Coverity found CID 1461718:
Integer handling issues (CONSTANT_EXPRESSION_RESULT) "timeout_ms >
9223372036854775807L" is always false regardless of the values of its
operands. This occurs as the logical second operand of "||".
Closes#5240
Prior to this change if there was a 303 reply to a PUT request then
the subsequent request to respond to that redirect would also be a PUT.
It was determined that was most likely incorrect based on the language
of the RFCs. Basically 303 means "see other" resource, which implies it
is most likely not the same resource, therefore we should not try to PUT
to that different resource.
Refer to the discussions in #5237 and #5248 for more information.
Fixes https://github.com/curl/curl/issues/5237
Closes https://github.com/curl/curl/pull/5248
GnuTLS 3.1.10 added new functions we want to use. That version was
released on Mar 22, 2013. Removing support for older versions also
greatly simplifies the code.
Ref: #5271Closes#5276
Detected by Coverity. CID 1462319.
"The same code is executed when the condition result is true or false,
because the code in the if-then branch and after the if statement is
identical."
Closes#5275
When cURL is compiled with support for multiple SSL backends, it is
possible to configure an SSL backend via `curl_global_sslset()`, but
only *before* `curl_global_init()` was called.
If another SSL backend should be used after that, a user might be
tempted to call `curl_global_cleanup()` to start over. However, we did
not foresee that use case and forgot to reset the SSL backend in that
cleanup.
Let's allow that use case.
Fixes#5255Closes#5257
Reported-by: davidedec on github
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
From libssh 0.9.0, ssh_key_type() returns different key types for ECDSA
keys depending on the curve.
Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>
Fixes#5252Closes#5253
- Fix schannel_send for the case when no timeout was set.
Prior to this change schannel would error if the socket was not ready
to send data and no timeout was set.
This commit is similar to parent commit 89dc6e0 which recently made the
same change for SOCKS, for the same reason. Basically it was not well
understood that when Curl_timeleft returns 0 it is not a timeout of 0 ms
but actually means no timeout.
Fixes https://github.com/curl/curl/issues/5177
Closes https://github.com/curl/curl/pull/5221
- Document in Curl_timeleft's comment block that returning 0 signals no
timeout (ie there's infinite time left).
- Fix SOCKS' Curl_blockread_all for the case when no timeout was set.
Prior to this change if the timeout had a value of 0 and that was passed
to SOCKET_READABLE it would return right away instead of blocking. That
was likely because it was not well understood that when Curl_timeleft
returns 0 it is not a timeout of 0 ms but actually means no timeout.
Ref: https://github.com/curl/curl/pull/5214#issuecomment-612512360
Closes https://github.com/curl/curl/pull/5220
Prior to this change gopher's blocking code would block forever,
ignoring any set timeout value.
Assisted-by: Jay Satiro
Reviewed-by: Daniel Stenberg
Similar to #5220 and #5221Closes#5214
