Storing a stream error in the per-connection struct was an error that lead to
race conditions as subsequent stream handling could overwrite the error code
before it was used for the stream with the actual problem.
Closes#6910
This was this one condition where the stream could be closed due to an
error and the function would still wrongly just return 0 for it.
Reported-by: Gergely Nagy
Fixes#6862Closes#6910
Both were used for the same purposes and there was no logical separation
between them. Combined, this also saves 16 bytes in less holes in my
test build.
Closes#6798
... but instead use a private alternative that points to the "driving
transfer" from the connection. We set the "user data" associated with
the connection to be the connectdata struct, but when we drive transfers
the code still needs to know the pointer to the transfer. We can change
the user data to become the Curl_easy handle, but with older nghttp2
version we cannot dynamically update that pointer properly when
different transfers are used over the same connection.
Closes#6520
... in most cases instead of 'struct connectdata *' but in some cases in
addition to.
- We mostly operate on transfers and not connections.
- We need the transfer handle to log, store data and more. Everything in
libcurl is driven by a transfer (the CURL * in the public API).
- This work clarifies and separates the transfers from the connections
better.
- We should avoid "conn->data". Since individual connections can be used
by many transfers when multiplexing, making sure that conn->data
points to the current and correct transfer at all times is difficult
and has been notoriously error-prone over the years. The goal is to
ultimately remove the conn->data pointer for this reason.
Closes#6425
... as the socket might be readable all the time when paused and thus
causing a busy-loop.
Reported-by: Harry Sintonen
Reviewed-by: Jay Satiro
Fixes#6356Closes#6357
Previously there were several locations that called
nghttp2_session_mem_recv and handled responses slightly differently.
Those have been converted to call the existing
h2_process_pending_input() function.
Moved the end-of-session check to h2_process_pending_input() since the
only place the end-of-session state can change is after nghttp2
processes additional input frames.
This will likely fix the fuzzing error. While I don't have a root cause
the out-of-bounds read seems like a use after free, so moving the
nghttp2_session_check_request_allowed() call to a location with a
guaranteed nghttp2 session seems reasonable.
Also updated a few nghttp2 callsites to include error messages and added
a few additional error checks.
Closes#5648
The previous h2 trailer fix in 54a2b63 was wrong and caused a
regression: it cannot deal with trailers immediately when read since
they may be read off the connection by the wrong 'data' owner.
This change reverts the logic back to gathering all trailers into a
single buffer, like before 54a2b63.
Reported-by: Tadej Vengust
Fixes#5663Closes#5769
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
...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
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
... 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
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
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
Prior to this change in libcurl debug builds http2 stream closure was
erroneously referred to as connection closure.
Before:
* nread <= 0, server closed connection, bailing
After:
* nread == 0, stream closed, bailing
Closes https://github.com/curl/curl/pull/5118
This reduces the HTTP/2 window size to 32 MB since libcurl might have to
buffer up to this amount of data in memory and yet we don't want it set
lower to potentially impact tranfer performance on high speed networks.
Requires nghttp2 commit b3f85e2daa629
(https://github.com/nghttp2/nghttp2/pull/1444) to work properly, to end
up in the next release after 1.40.0.
Fixes#4939Closes#4940
To simplify our code and since earlier versions lack important function
calls libcurl needs to function correctly.
nghttp2 1.12.0 was relased on June 26, 2016.
Closes#4961
A regression made the code use 'multiplexed' as a boolean instead of the
counter it is intended to be. This made curl try to "over-populate"
connections with new streams.
This regression came with 41fcdf71a1, shipped in curl 7.65.0.
Also, respect the CURLMOPT_MAX_CONCURRENT_STREAMS value in the same
check.
Reported-by: Kunal Ekawde
Fixes#4779Closes#4784
It could accidentally let the connection get used by more than one
thread, leading to double-free and more.
Reported-by: Christopher Reid
Fixes#4544Closes#4557
- Disable warning C4127 "conditional expression is constant" globally
in curl_setup.h for when building with Microsoft's compiler.
This mainly affects building with the Visual Studio project files found
in the projects dir.
Prior to this change the cmake and winbuild build systems already
disabled 4127 globally for when building with Microsoft's compiler.
Also, 4127 was already disabled for all build systems in the limited
circumstance of the WHILE_FALSE macro which disabled the warning
specifically for while(0). This commit removes the WHILE_FALSE macro and
all other cruft in favor of disabling globally in curl_setup.
Background:
We have various macros that cause 0 or 1 to be evaluated, which would
cause warning C4127 in Visual Studio. For example this causes it:
#define Curl_resolver_asynch() 1
Full behavior is not clearly defined and inconsistent across versions.
However it is documented that since VS 2015 Update 3 Microsoft has
addressed this somewhat but not entirely, not warning on while(true) for
example.
Prior to this change some C4127 warnings occurred when I built with
Visual Studio using the generated projects in the projects dir.
Closes https://github.com/curl/curl/pull/4658
To make sure that transfer is being dealt with. Streams without
Content-Length need a final read to notice the end-of-stream state.
Reported-by: Tom van der Woerdt
Fixes#4496
To make sure that the HTTP/2 state is initialized correctly for
duplicated handles. It would otherwise easily generate "spurious"
PRIORITY frames to get sent over HTTP/2 connections when duplicated easy
handles were used.
Reported-by: Daniel Silverstone
Fixes#4303Closes#4442
If the :authority pseudo header field doesn't contain an explicit port,
we assume it is valid for the default port, instead of rejecting the
request for all ports.
Ref: https://curl.haxx.se/mail/lib-2019-09/0041.htmlCloses#4365
It could otherwise return an error even when closed correctly if GOAWAY
had been received previously.
Reported-by: Tom van der Woerdt
Fixes#4267Closes#4268
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
There were a leftover few prototypes of Curl_ functions that we used to
export but no longer do, this removes those prototypes and cleans up any
comments still referring to them.
Curl_write32_le(), Curl_strcpy_url(), Curl_strlen_url(), Curl_up_free()
Curl_concat_url(), Curl_detach_connnection(), Curl_http_setup_conn()
were made static in 05b100aee2.
Curl_http_perhapsrewind() made static in 574aecee20.
For the remainder, I didn't trawl the Git logs hard enough to capture
their exact time of deletion, but they were all gone: Curl_splayprint(),
Curl_http2_send_request(), Curl_global_host_cache_dtor(),
Curl_scan_cache_used(), Curl_hostcache_destroy(), Curl_second_connect(),
Curl_http_auth_stage() and Curl_close_connections().
Closes#4096
Reviewed-by: Daniel Stenberg <daniel@haxx.se>
To make sure a HTTP/2 stream registers the end of stream.
Bug #4043 made me find this problem but this fix doesn't correct the
reported issue.
Closes#4068
