Viktor Szakats [Sun, 22 Sep 2024 15:48:17 +0000 (17:48 +0200)]
tests/server: round of tidy-ups (part 2)
General tidy-ups, to identify and reduce duplications and potential
issues, while also making the server modules compile as a single binary.
- ensure unique symbols and no shadowing across server sources, by
renaming variables.
- move globals common to multiple servers into shared `util` module.
- drop constants with a single use.
- undef macro before re-using them across server sources.
- move common functions into shared `util` module.
- drop redundant static declarations.
- disable IPv6 code when built without IPv6.
- start syncing the 3 almost identical copies of `sockdaemon` function.
- drop unused `timeval.h` header.
- drop `poll()` from `wait_ms()`, for macOS, following an earlier core
update.
Follow-up to c72cefea0fadaf4114a0036c86005ee5739ec30a #15096
Viktor Szakats [Tue, 25 Feb 2025 15:27:15 +0000 (16:27 +0100)]
cmake: `CURL_LIBDIRS` improvements (upstreamed from vcpkg)
Apply downstream patches from the vcpkg project:
- cmake: remove duplicates from `CURL_LIBDIRS`.
- cmake: set `CURL_LIBDIRS` as `INTERFACE_LINK_DIRECTORIES` for static
libcurl.
To support CMake <3.13, change downstream patch from:
```cmake
target_link_directories(${LIB_STATIC} INTERFACE ${CURL_LIBDIRS})
```
to:
```cmake
set_target_properties(${LIB_STATIC} PROPERTIES [...] INTERFACE_LINK_DIRECTORIES "${CURL_LIBDIRS}")
```
Co-authored-by: Kai Pastor
Ref: https://github.com/microsoft/vcpkg/pull/43819
Daniel Stenberg [Fri, 7 Mar 2025 08:32:57 +0000 (09:32 +0100)]
url: call protocol handler's disconnect in Curl_conn_free
For the case when the connection struct is all setup, the protocol
handler allocates data in its setup_connection function, but the
connection struct is discarded again before used further because a
connection reuse is prefered. Then the handler's disconnect function was
not previously called, which then would lead to a memory leak.
I added test case 698 that reproduces the leak and the fix.
Harry Sintonen [Thu, 6 Mar 2025 19:42:43 +0000 (21:42 +0200)]
doh: improve HTTPS RR svcparams parsing
Fixed a heap read overflow when parsing the HTTP RR svcparams. Also the
code failed to enforce the requirements of SvcParamKey order specified
in section 2.2 of the RFC 9460.
Viktor Szakats [Wed, 5 Mar 2025 23:17:08 +0000 (00:17 +0100)]
sectransp: add support for HTTP/2 in gcc builds
Before this patch `--http2` did not work in gcc builds with Secure
Transport, because ALPN relied on a compiler supporting the
`HAVE_BUILTIN_AVAILABLE` aka `__builtin_available()` feature. This
is clang-specific and missing from gcc (as of gcc v14).
Add support for ALPN and HTTP/2 when this compiler feature is missing.
Viktor Szakats [Thu, 6 Mar 2025 03:13:23 +0000 (04:13 +0100)]
tests: reformat error messages to avoid tripping MSBuild
Change the format of error messages sent to stderr from tests and test
servers. As a workaround to avoid triggering Visual Studio's MSBuild
tool's built-in regexp matcher, and making it mark builds failed for
reasons we don't want them to hard fail.
Roughly, the pattern to avoid is the word "error" (case-insensitive)
in the same line with a colon `:`.
It affected GHA/windows MSVC CI jobs, causing flakiness:
```
CUSTOMBUILD : fopen() failed with error : 13 Permission denied [D:\a\curl\curl\bld\tests\test-ci.vcxproj]
Error opening file: log/4/smtp_sockfilt.log
[...]
CUSTOMBUILD : fopen() failed with error : 13 Permission denied [D:\a\curl\curl\bld\tests\test-ci.vcxproj]
Error opening file: log/8/imap_sockfilt.log
Msg not logged: 00:18:10.656000 > 178 bytes data, server => client
[...]
TESTDONE: 1629 tests out of 1634 reported OK: 99%
Building Custom Rule D:/a/curl/curl/tests/CMakeLists.txt
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Microsoft\VC\v170\Microsoft.CppCommon.targets(254,5): error MSB8066: Custom build for 'D:\a\curl\curl\bld\CMakeFiles\621f80ddbb0fa48179f056ca77842ff0\test-ci.rule;D:\a\curl\curl\tests\CMakeLists.txt' exited with code -1. [D:\a\curl\curl\bld\tests\test-ci.vcxproj]
Error: Process completed with exit code 1.
```
Ref: https://github.com/curl/curl/actions/runs/13643149623/job/38137076210?pr=16490#step:14:3125
Ref: https://github.com/curl/curl/actions/runs/13688765792/job/38277961720?pr=16582#step:14:1717
The `IgnoreStandardErrorWarningFormat="true"` MSBuild Exec option
controls this behavior:
https://learn.microsoft.com/visualstudio/msbuild/exec-task#parameters
I couldn't figure out a way to apply it to CMake builds.
Note: There may be further error messages output from runtests scripts,
that use this format, which are not explicitly fatal. They may need
future fixes.
Thanks-to: Dion Williams
Ref: https://github.com/curl/curl/discussions/14854#discussioncomment-12382190
Ref: https://github.com/curl/curl/discussions/14854#discussioncomment-12395224
Viktor Szakats [Wed, 5 Mar 2025 17:06:37 +0000 (18:06 +0100)]
windows: do not use winsock2 `inet_ntop()`/`inet_pton()`
Disable these winsock2 functions on Windows to use the curl wrappers
and preserve `WSAGetLastError()` aka `SOCKERRNO` error codes.
curl sources uses `inet_pton()` and `inet_ntop()` via its own `Curl_`
prefixed wrappers. These wrappers promise to not overwrite
`WSAGetLastError()` aka `SOCKERRNO` error codes when calling them.
But, for Windows builds with these built-in winsock2 functions detected
(meaning all supported Windows versions, except Windows CE),
the wrappers were 1-to-1 mapped to the winsock2 functions, which broke
this promise.
These promises are old (a1d598399146984c99baa46db148e87c75261033) and
may not be valid anymore. In this case, the callers would have to be
updated to use `SOCKERRNO` to retrieve any error, instead of using
`errno` as they do now.
Daniel Stenberg [Tue, 4 Mar 2025 13:00:03 +0000 (14:00 +0100)]
multi: start the loop over when handles are removed
Since more than one handle can be removed in a single call to
multi_runsingle(), we cannot easily continue on the next node when a
node has been removed since that node migth ALSO have been removed.
Stefan Eissing [Thu, 6 Mar 2025 10:01:49 +0000 (11:01 +0100)]
tests/certs: cleanup
Keep only the generated files needed for tests. Place generated
intermediaries in `tests/certs/gen` where they are ignored by git. No
longer generated `*.dhp` files.
Have a shorter naming scheme: `test-ca` instead of `EdelCurlRoot-ca` and
`test-localhost` instead of `Server-localhost-sv`, etc.
Remove the `stunnel` certificate as it was nearly a duplicate of
`test-localhost`.
No longer copy a generated certificates to `tests/stunnel.pem`. Let test
server default to `certs/test-localhost.pem` instead.
Viktor Szakats [Thu, 6 Mar 2025 11:19:24 +0000 (12:19 +0100)]
GHA/windows: drop test exclusions from MSYS jobs
Re-enable running tests 19, 504, 704, 705, 1233 in CI MSYS jobs.
We carried over these exceptions from AppVeyor CI, where they have been
present for a long time. Cygwin jobs do not need these exceptions and
Cygwin and MSYS are similar envs. Time to re-evaluate if skipping them
is still necessary on MSYS.
Viktor Szakats [Thu, 6 Mar 2025 03:43:49 +0000 (04:43 +0100)]
tests: mark tests 1631, 1632 flaky
We already marked them flaky in GHA/macos CI. They are also flaky in
other CI jobs, in other OSes, with multiple TLS backends:
- MSVC/LibreSSL: https://github.com/curl/curl/actions/runs/13683996410/job/38262956317
- MSVC/wolfSSL: https://github.com/curl/curl/actions/runs/13680682695/job/38252047077
- FreeBSD/OpenSSL3: https://github.com/curl/curl/actions/runs/13690910863/job/38283867721#step:3:1
Viktor Szakats [Wed, 5 Mar 2025 17:38:57 +0000 (18:38 +0100)]
runtests: check and report if `diff` tool is missing
To make it apparent which CI jobs are missing this tool, so we can
install it to improve the runtests log.
Correction to the followed-up commit: `diff` is not installed via the
`gcc` package but via `automake`. Meaning it needs be installed manually
for MSYS cmake jobs.
Viktor Szakats [Wed, 5 Mar 2025 13:55:30 +0000 (14:55 +0100)]
GHA/windows: bump msys2 action, downgrade runtime for mingw tests
Bump msys2/setup-msys2 from 2.26.0 to 2.27.0. It brings the perf
regression experienced earlier with GfW and the pre-installed
MSYS2 on the GHA runner. Apply the runtime downgrade trick as
a workaround.
Viktor Szakats [Wed, 5 Mar 2025 10:50:02 +0000 (11:50 +0100)]
GHA/windows: stop ignoring most ignored test results
These tests seem to be running no less stable now than others.
Stop ignoring their results to catch real issues.
These are consistently failing and remain on the ignore list:
in MSVC / vcpkg jobs:
```
FAIL-IGNORED 2302: 'WebSockets via callback (frame mode) + curl_ws_send()' WebSockets
FAIL-IGNORED 2303: 'WebSockets but gets a 200 back' WebSockets
FAIL-IGNORED 2307: 'WebSockets, overlong PING payload' WebSockets
```
https://github.com/curl/curl/actions/runs/13674664461/job/38233949942?pr=16570#step:14:4089
- Likely curl issues either in tests, server, or in WebSockets support.
in tests running under MSYS, affecting native mingw Windows builds only:
```
FAIL-IGNORED 612: 'SFTP post-quote remove file' SFTP, post-quote
[...]
curl: (21) rm command failed: Operation failed
```
https://github.com/curl/curl/actions/runs/13674664461/job/38233952699?pr=16570#step:14:1378
in tests running under MSYS, affecting both MSYS and native mingw Windows builds:
```diff
FAIL-IGNORED 613: 'SFTP directory retrieval' SFTP, directory
[...]
--- log/7/check-expected 2025-03-05 11:19:54.119658000 +0000
+++ log/7/check-generated 2025-03-05 11:19:54.119658000 +0000
@@ -1,3 +1,3 @@
d????????? N U U N ??? N NN:NN asubdir[LF]
--rw?rw?rw? 1 U U 37 Jan 1 2000 plainfile.txt[LF]
+-rw?r-?r-? 1 U U 37 Jan 1 2000 plainfile.txt[LF]
-r-?r-?r-? 1 U U 47 Dec 31 2000 rofile.txt[LF]
```
https://github.com/curl/curl/actions/runs/13674664461/job/38233950866?pr=16570#step:14:1316
- Possibly a curl test portabibility, Perl or MSYS issue.
in Cygwin tests:
```
FAIL-IGNORED 615: 'SFTP put remote failure' SFTP, SFTP put, FAILURE
```
https://github.com/curl/curl/actions/runs/13674664461/job/38233949428?pr=16570#step:12:3817
Ondřej Hlavatý [Wed, 5 Mar 2025 13:09:26 +0000 (14:09 +0100)]
request: clear sendbuf_hds_len when resetting request bufq
Without this, any usage of sendbuf_hds_len on a retried request is
wrong. We noticed by getting debug callbacks with incorrect header len.
We did not figure out how to trigger the retries in a test environment
though.
Stefan Eissing [Tue, 4 Mar 2025 14:50:12 +0000 (15:50 +0100)]
gnutls: set priority via --ciphers
No longer ignore the `--ciphers` argument in gnutls curl builds, but use
it to set the gnutls priority string.
When the set ciphers start with '+', '-' or '!', it is *appended* to the
curl generated priority string. Otherwise it replaces the curl one
completely.
Stefan Eissing [Tue, 4 Mar 2025 11:31:32 +0000 (12:31 +0100)]
http2: detect session being closed on ingress handling
nghttp2 will on its own send GOAWAY frames, closing the connection, when
internal processing of frames runs into errors. This may not become
visible in a direct error code from a call to nghttp2.
Check for session being closed on ingress processing (on sending, we
already did that) and report an error if so. In addition, monitor
outgoing GOAWAY not initiated by us so that the user will get a fail
message when that happens.
Stefan Eissing [Tue, 4 Mar 2025 10:48:04 +0000 (11:48 +0100)]
http2: add on_invalid_frame callback for error detection
When the server sends HEADER/CONTINUATION frames that exceed nghttp2's
size, this error is being reported via the on_invalid_frame_recv
callback. Without registering there, it will go unnoticed.
Stefan Eissing [Mon, 3 Mar 2025 13:05:34 +0000 (14:05 +0100)]
http2: reset stream on response header error
We send a GOAWAY, but some servers ignore that and happily continue
sending the stream response. RST the stream when response header errors
are encountered.
Keep this job for build tests and drop running tests to improve the CI
experience and save CI time.
It's also a simple build with no dependencies. CI continues to build
a similar job with 9.5.0, which is more stable.
It remains a puzzle why builds with this toolchain (7.3.0 win32 threads
mingw-builds) is flakier and requires more test exceptions than the
indentical build with a slightly different build/version of
the toolchain (9.5.0 posix threads winlibs_mingw).
- Breaks out the existing print out of the LIBSSH2_DEBUG compile-time
flag
- Adds (single) quotation marks around the string to better expose the
actual value
- Adds a NULL print if not set, mirroring other verbose prints in
libssh2
Why was this done?
I was trying out the `sftp` option in `curl`, and found myself hitting
an issue where I was not able to get curl to tell me which username it
was using to connect to a host.
With this change, the `User: ` line is printed with `-v`, just like
other SSH verbose prints.
Instead of using the pattern used with *SSH MD5 public key*, where a
ternary is used to print `NULL` on NULL values, it is using a different
branch to add quotes around the string value.
The quotes around the string value are used to better expose to the user
an empty string value, compared to "no-value".
Viktor Szakats [Sun, 8 Sep 2024 14:18:57 +0000 (16:18 +0200)]
tests/server: round of tidy-ups
Dedupe, merge macros, globals, make symbols local where possible.
Drop unused macros and headers. Drop `DEFAULT_LOGFILE` macro in favour
of `--logfile` command-line option.
Viktor Szakats [Mon, 3 Mar 2025 23:26:45 +0000 (00:26 +0100)]
cmake: exclude `-MP` for `clang-cl` again
To avoid this warning/error (seen with Ninja generator):
```
clang-cl: warning: argument unused during compilation: '-MP' [-Wunused-command-line-argument]
```
Curious why CI missed it. Maybe due to using a Visual Studio generator.
Viktor Szakats [Fri, 28 Feb 2025 12:17:39 +0000 (13:17 +0100)]
cmake: allow `CURL_STATIC_CRT` with UCRT VS2015+ builds
After this patch, we're back to 8.12.1, but disallowing
`CURL_STATIC_CRT=ON` with shared curl exe built with VS2013 or older.
Because those may crash. A stable reprducer is with `ENABLE_DEBUG=ON`
and calling `curl.exe -V`.
You can pass the necessary CMake and MSVC linker options manually,
to get around this condition.
Shared build with static UCRT may be crashing too, depending on
conditions. Consult the documentation about limitations of static CRT:
https://learn.microsoft.com/cpp/c-runtime-library/crt-library-features
Daniel Stenberg [Mon, 3 Mar 2025 11:13:38 +0000 (12:13 +0100)]
docs/cmdline-opts: unify HTTP version style in --help output
$ curl -h all | grep -- --http
Now:
--http0.9 Allow HTTP/0.9 responses
-0, --http1.0 Use HTTP/1.0
--http1.1 Use HTTP/1.1
--http2 Use HTTP/2
--http2-prior-knowledge Use HTTP/2 without HTTP/1.1 Upgrade
--http3 Use HTTP/3
--http3-only Use HTTP/3 only
Before:
--http0.9 Allow HTTP 0.9 responses
-0, --http1.0 Use HTTP 1.0
--http1.1 Use HTTP 1.1
--http2 Use HTTP/2
--http2-prior-knowledge Use HTTP 2 without HTTP/1.1 Upgrade
--http3 Use HTTP v3
--http3-only Use HTTP v3 only
Stefan Eissing [Mon, 3 Mar 2025 13:39:47 +0000 (14:39 +0100)]
multi_ev: use `mid` instead of `id` for transfer hashes
`data->id` is unique in the same connection pool, but a multi may
involved more than one pool. `data->mid` is unique inside the multi and
since multi_ev lives inside one multi, the `mid` is the right thing to
use.
Daniel Stenberg [Mon, 3 Mar 2025 10:35:48 +0000 (11:35 +0100)]
lib: add CURLFOLLOW_OBEYCODE and CURLFOLLOW_FIRSTONLY
With this change, the argument passed to the CURLOPT_FOLLOWLOCATION
option is now instead a "mode" instead of just a boolean. Documentation
is extended to describe the two new modes.
Stefan Eissing [Thu, 27 Feb 2025 14:47:30 +0000 (15:47 +0100)]
shutdowns: split shutdown handling from connection pool
Further testing with timeouts in event based processing revealed that
our current shutdown handling in the connection pool was not clear
enough. Graceful shutdowns can only happen inside a multi handle and it
was confusing to track in the code which situation actually applies. It
seems better to split the shutdown handling off and have that code
always be part of a multi handle.
Add `cshutdn.[ch]` with its own struct to maintain connections being
shut down. A `cshutdn` always belongs to a multi handle and uses that
for socket/timeout monitoring.
The `cpool`, which can be part of a multi or share, either passes
connections to a `cshutdn` or terminates them with a one-time, best
effort.
Add an `admin` easy handle to each multi and share. This is used to
perform all maintenance operations where no "real" easy handle is
available. This solves the problem that the multi admin handle requires
some additional initialisation (e.g. timeout list).
The share needs its admin handle as it is often cleaned up when no other
transfer or multi handle exists any more. But we need a `data` in almost
every call.
Fix file:// handling of errors when adding a new connection to the pool.
Changes in `curl` itself:
- for parallel transfers, do not set a connection pool in the share,
rely on the multi's connection pool instead. While not a requirement
for the new `cshutdn` to work, this is
a) helpful in testing to trigger graceful shutdowns
b) a broader code coverage of libcurl via the curl tool
- on test_event with uv, cleanup the multi handle before returning from
parallel_event(). The uv struct is on the stack, cleanup of the multi
later will crash when it tries to register sockets. This is a "eat
your own dogfood" related fix.
Stefan Eissing [Sun, 23 Feb 2025 11:20:17 +0000 (12:20 +0100)]
hash_offt: standalone hash for curl_off_t
Add a standalong hash table for curl_offt_t as key. This allows a
smaller memory footprint and faster lookups as we do not need to deal
with variable key lengths.
Use in all places we had the standard hash for this purpose.
Stefan Eissing [Fri, 31 Jan 2025 10:25:15 +0000 (11:25 +0100)]
pytest: test negotiate with http proxy
Fixes #14973 Reported-by: stevenpackardblp on github
When curl negotiated with a http: proxy for a https: request, it
wrongly believed there must be an SSL filter present, which during
CONNECT, there is not.
25b445e fixed this. This PR adds a pytest case for the setup.
Viktor Szakats [Fri, 28 Feb 2025 22:56:01 +0000 (23:56 +0100)]
winbuild: reduce command-line length by dropping whitespace
Keep the `@for %%i in [...]` lines within limits by stripping whitespace
from the input `.c` source lists read from `Makefile.inc`. To avoid this
error after adding a new `.c` source:
```
configuration name: libcurl-vc14-x64-release-dll-ssl-dll-ipv6-sspi
NMAKE : fatal error U1095: expanded command line 'for %i in (altsvc.obj amigaos.obj
asyn-ares.obj asyn-thread.obj base64.obj bufq.obj
bufref.obj cf-h1-proxy.obj cf-h2-proxy.obj cf-haproxy.obj [...]
vssh/wolfssh.obj) do @echo ..\builds\libcurl-vc14-x64-release-dll-ssl-dll-ipv6-sspi-obj-lib/%i \
' too long
Stop.
Command exited with code 2
```
Ref: https://ci.appveyor.com/project/curlorg/curl/builds/51605338/job/dqg6qtebtscb279g#L44
Reported-by: Stefan Eissing
Bug: https://github.com/curl/curl/pull/16508#issuecomment-2690443409
Fixes #16521
Closes #16528
Viktor Szakats [Fri, 28 Feb 2025 02:50:29 +0000 (03:50 +0100)]
GHA/macos: use quictls in some jobs, other small improvements
- enable quictls in autotools and cmake jobs. autotools requires
a workaround due to wrong libpath in the quictls pkg-config.
nghttp3 is offered by Homebrew, but not ngtcp2, to enable H3.
- install `libnghttp2` rather than `nghttp2`.
`libnghttp2` is preinstalled and smaller. It also avoids detecting
`nghttpx`, which confuses `pytest`.
- limit `brew unlink openssl` to libressl/quictls jobs.
Stefan Eissing [Tue, 25 Feb 2025 09:31:43 +0000 (10:31 +0100)]
gnutls: fix use of pkcs11 urls for keys/certs
Fixes #16249 Forwarded-to-us-by: Carlos Henrique Lima Melara
Always use `gnutls_certificate_set_x509_key_file2()` for loading keys
and certificates, even without a password, since this function support
pkcs11 urls.
Thanks to @tatsuhiro-t for finding this out. Help-by: Tatsuhiro Tsujikawa
Closes #16472
Viktor Szakats [Thu, 27 Feb 2025 10:32:43 +0000 (11:32 +0100)]
tidy-up: prefer `return` over `exit()`, fix fallouts
To avoid breaking the control flow and align to majority of code
already using `return`.
`exit()` has the side-effect of suppressing leak detection in cases.
Fix fallouts detected after switching to `return`.
- configure:
- fix `getaddrinfo` run test to call `freeaddrinfo()` to pacify ASAN,
and call `WSACleanup()` to deinit winsock2.
- fix `getifaddrs` run test to call `freeifaddrs()` to pacify ASAN.
- tests/server:
- setup `atexit(win32_cleanup)` via `win32_init()`.
- return 2 instead of 1 on winsock2 init failures.
- sws: goto cleanup instead of `exit()` in `http_connect()`.
Follow-up to 02dfe7193704817184b522888ffa926e6b73f648 #7235
- tests/client/http:
- cleanup memory to pacify ASAN in `h2-upgrade-extreme`,
`tls-session-reuse`.
- examples:
- block_ip: fix memory leak reported by CI.
- http2-upload: avoid handle leaks.
Untouched `exit()` calls, made from callbacks:
- docs/examples: ephiperfifo.c, ghiper.c, hiperfifo.c
- tests/libtest: lib582.c, lib655.c, lib670.c
- tests/server: tftpd.c
Viktor Szakats [Thu, 27 Feb 2025 22:54:47 +0000 (23:54 +0100)]
cmake: drop `HAVE_C_FLAG_Wno_long_double` logic for ancient Apple gcc
The initial curl CMake commit introduced it in 2009-04-02 via 4c5307b45655ba75ab066564afdc0c111a8b9291. Suppressing a stray
`-Wlong-double` warning in `mprintf.c`. This was before Apple switched
to clang, and likely affected the Apple distributed GCC, version 4.2.1
at the time. It applied the workaround to CMake builds only, though
the issue seems build-tool agnostic. Yet, it was not suppressed or
reported for autotools builds.
For these reasons this logic seems obsolete and this patch drops it with
no replacement. It saves a feature detection for GCC builds for macOS.
In PR sub-commits I added (and reverted) in-source suppression. In case
it becomes necessary, that should fix it for all build tools.
projects / thirdparty / curl.git / log
