clang-tidy <= v20 (as seen between 18.1.3 and 20.1.2) report
`readability-uppercase-literal-suffix` originating from mingw-w64 system
header `_mingw_mac.h` via `define __MSABI_LONG(x) x ## l`
Triggered by `SOCKENOMEM` (e.g. in tests/server/sockfilt.c):
```
warning: integer literal has suffix 'l', which is not uppercase [readability-uppercase-literal-suffix]
```
Work around by replacing Windows macro `WSA_NOT_ENOUGH_MEMORY`
with its literal value.
Bug: https://github.com/curl/curl/pull/20631#issuecomment-3930619868
Follow-up to c07c3cac74#20629
Cherry-picked from #20631Closes#20638
Fix bigger and smaller kinks in how clang-tidy is configured and used.
Sync behavior more between autotools and cmake, lib/src and tests. Bump
clang-tidy minimum version and prepare logic to allow using clang-tidy
to a fuller extent.
- move clang-tidy settings from builds to a new `.clang-tidy.yml`.
To make it easy to see and edit checks at one place. Also to allow
using the `--checks=` option internally to silence tests-specific
checks. (clang-tidy does not support multiple `--check=` options via
the command-line.)
Use explicit `--config-file=` option to point to the configuration.
- .clang-tidy.yml: link to documentation.
- suppress `clang-diagnostic-nullability-extension` due to a false
positive in libtests with `CURL_WERROR=ON` and `PICKY_COMPILER=OFF`.
- .clang-tidy.yml: enable `portability-*`, `misc-const-correctness`.
- drop `--quiet` clang-tidy option by default to make its working a bit
more transparent. The extra output is minimial.
- consistently use double-dashes in clang-tidy command-line options.
Supported by clang-tidy 9.0.0+ (2019-09-19). Before this patch single
and double were used arbitrarily.
- src/tool_parsecfg: silence false positive `clang-analyzer-unix.Stream`.
Seen with clang 18 + clang-tidy 19 and 20 (only with autotools.)
- INTERNALS: require clang-tidy 14.0.0+. For the `--config-file` option.
- INTERNALS: recommend clang-tidy 19.1.0+, to avoid bogus
`clang-analyzer-valist.Uninitialized` warnings. (bug details below)
autotools:
- allow configuring the clang-tidy tool via `CLANG_TIDY` env.
Also to use in GHA to point to a suffixed clang-tody tool.
- fix to pass CFLAGS to lib, src sources.
(keep omitting them when using a non-clang compiler.)
- fix to pass `--warnings-as-errors=*` in quotes to avoid globbing.
cmake:
- fix to not pass an empty `-I` to clang-tidy.
- fix to pass CFLAGS (picky warnings) to clang-tidy for test sources.
(keep omitting them when using a non-clang compiler.)
- fix to disable `clang-diagnostic-unused-function` for test sources.
(tests have static entry points, which trigger this check when
checking them as individidual sources.)
- fix forwarding `CURL_CLANG_TIDYFLAGS` to clang-tidy.
- force disable picky warnings when running clang-tidy with a non-clang
compiler. To not pass these flags when checking lib and src.
CI:
- GHA/linux: avoid clang-tidy bug by upgrading to v19, and drop the
workaround.
- GHA/linux: switch to clang from gcc in the clang-tidy job. Using gcc
doesn't allow passing CFLAGS to clang-tidy, making it less effective.
(My guess this was one factor contributing to this job often missing
to find certain issues compared to GHA/macos.)
I recomment using clang-tidy with a clang compiler, preferably the same
version or one that's compatible. Other cases are best effort, and may
fail if a C flag is passed to clang-tidy that it does not understand.
Picky warnings are mostly omitted when using a non-clang compiler,
reducing its usefulness.
Details and reproducer for the v18 (and earlier) clang-tidy bug,
previously affecting the GHA/linux job:
clang-tidy <=18 emits false warnings way when passing multiple C sources
at once (as done with autotools):
```sh
cat > src1.c <<EOF
#include <string.h>
static void dummy(void *p) { memcmp(p, p, 0); }
EOF
cat > src2.c <<EOF
#include <stdarg.h>
void vafunc(int option, ...)
{
va_list param;
va_start(param, option);
if(option)
(void)va_arg(param, int);
va_end(param);
}
EOF
/opt/homebrew/opt/llvm@18/bin/clang-tidy --checks=clang-analyzer-valist.Uninitialized src1.c src2.c
# src2.c:7:11: warning: va_arg() is called on an uninitialized va_list [clang-analyzer-valist.Uninitialized]
```
Follow-up to e86542038d#17047Closes#20605
- fix internal macro `AN_APPLE_OS` reused between sources without
resetting it. It may potentially have left the system sha256
function unused.
- fix to define `WOLFSSL_OPTIONS_IGNORE_SYS` so that it always applies
to wolfSSL headers, also during feature detection.
- md4, md5, sha256: simplify fallback logic.
- delete 20+ unused macros.
- scope or move macros to avoid `-Wunused-macros` warnings.
- examples: delete unused code.
The warning detects macros defined but not used within the same C
source. It does not warn for macros defined in headers. It also works
with unity builds, but to a lesser extent.
Closes#20593
Any failf() that fill the errorbuf need to be forgotten once happy
eyeballing finds a succssful winner. Because the errorbuf, once set, is
not overwritten with future error information.
Adds test_05_05 to verify.
Reported-by: Tim Friedrich Brüggemann
Fixes#20608Closes#20613
Refactor and simplify the Schannel code, primarily by reducing
duplicated buffer-management and credential-setup logic.
- split client certificate selection into get_client_cert() and SSPI
credential acquisition into acquire_sspi_handle()
- introduce a struct sbuffer for encrypted/decrypted buffering
- Add ensure_encoding_size() and ensure_decoding_size() helpers to
centralize buffer growth/realloc decisions
- Tighten variable scopes and tidy indentation/logging in the handshake
and receive/decrypt loops.
- Update comments and adjusts some receive error-condition handling to
better preserve buffered-data behavior.
Closes#20569
To reduce complexity.
- is_finished() checks if the individual transfer is done
- handle_completed() is the logic that runs for a completed
transfer
Closes#20573
To simplify setting BoringSSL version, using:
`-DBORINGSSL_VERSION=0.20260211.0`
or
`-DBORINGSSL_VERSION=${boringssl_version}`
Previously it could be set via C flags, using complicated shell quotes:
`-DCMAKE_C_FLAGS="-DCURL_BORINGSSL_VERSION=\\\"${boringssl_version}\\\""`
(the C flags method remains, also for autotools)
It'd be nice if BoringSSL published its version not just via
`MODULE.bazel` in its source tree, but from its public headers, to make
these workarounds unnecessary.
Also:
- GHA/http3-linux: test both options.
Closes#20571
- openssl: move and expand explanatory comment.
- openssl: drop duplicate workaround.
- schannel: drop workaround. Unnecessary, because OpenSSL headers are
not included in or after schannel code.
- schannel: drop explicit `wincrypt.h` include. It's indirectly
included by system `<schannel.h>`.
- ldap: drop explicit `wincrypt.h` include.
It isn't used there, and also not required for the workaround.
`winldap.h` keeps including it indirectly.
Tested with BoringSSL and AWS-LC (MultiSSL with Schannel), also LDAP
enabled, and H3, unity and non-unity, and all tested cases build fine.
In lib in general, the point is to have the `#undef`s between the first
`wincrypt.h` include [1] and the first OpenSSL include [2], within a
single compilation unit. For non-unity builds the only such source is
`openssl.c`. For unity ones, depending on batch size, in theory we
should `#undef` after each `wincrypt.h` include. In practice this is
overkill and most cases are covered by `#undef`-fing _first_ in
`vtls/openssl.c`, and `#undef` in `ldap.c`. It's not impossible that we
need to add more undefs after further `wincrypt.h` includes to cover so
far undiscovered build cases [3]. Though I could not find more with the
current sources and source order.
It's also an option to include OpenSSL first, then `wincrypt.h`, as
done in libtests, but for lib and `vtls/openssl.c` it's more practical
to do the opposite.
[1] can be indirect, e.g. via `iphlpapi.h`, `schannel.h`, `winldap.h`.
[2] in
- BoringSSL/AWS-LC: any include (due to `openssl/base.h`).
Original fix removed by BoringSSL in year
[2014](ded93581f1 (diff-878093ea6426091505b4c49c59b78924f42859af0eb4ce39b8089bda9577e013)).
- OpenSSL: `openssl/ssl.h`, `openssl/x509v3.h`, and some more affected,
and including `openssl/ossl_typ.h` does the `#undef` automatically.
Since [3.1.0+](fbb9a1f997)
each inclusion does the `#undef`, in 3.0.x (and earlier) only
the first inclusion did. Initially fixed in
[0.9.6d](1955b87423)
- LibreSSL [2.3.0+](0fa826d34f):
not affected, though to suppress another warning 3.8.2+ and
a [define](e7fe6caab2)
is necessary.
[3] `lib/Makefile.inc` defines the order of unity sources.
For libtests, the case is simpler: There is always one compilation unit,
with a fixed order, and at the moment `cli_hx_download.c` is including
OpenSSL first, then wincrypt, and in this order they don't bother each
other. Also, at the moment `lib758.c` is the only other OpenSSL header
user, but it's compiled after `cli_hx_download.c` so the include is
skipped there. We may need to revisit this if either header gets
included before it.
All this said it'd be nice if BoringSSL/AWS-LC restored the built-in
workaround to behave like LibreSSL and OpenSSL and not require local
workarounds like these.
Ref: https://github.com/curl/curl/pull/20556#issuecomment-3888425644
Follow-up to 4c46c829f5#9110
Follow-up to fbe07c6829#5669#5857Closes#20567
- define `SECURITY_WIN32` globally in `curl_setup.h`.
To make sure it applies to all includes.
- document which Windows headers require `SECURITY_WIN32`.
- stop suppressing MSVC warning:
`C4201 is: nonstandard extension used : nameless struct/union`
The warning is no longer seen in supported build envs with the current
codebase.
Follow-up to 8beff43559#8419
- document why `SCHANNEL_USE_BLACKLISTS` is needed.
- just define `SCHANNEL_USE_BLACKLISTS`, drop the unnecessary value `1`.
- stop defining unused `SCH_CRED_MAX_SUPPORTED*` fallback macros.
Follow-up to 8beff43559#8419
- document why `subauth.h` is included (where missing).
- move and de-dupe `subauth.h` include into `curl_setup.h`, limit to
Schannel builds.
- stop include `schnlsp.h`. It is a 1-to-1 compatibility wrapper for
`schannel.h`.
- curl_sspi.h: clarify comment about `SP_NAME_` macros.
They are local macros, their SDK names are different and curl does not
use them.
- curl_sspi.h: drop superfluous includes `security.h` and `rpc.h`.
Cherry-picked from #20556Closes#20564
Drop detecting it at configure time, along with the interim macro
`HAVE_MSG_NOSIGNAL`. There is no longer a reason for this workaround,
and allows to save the work at configure time and simplify.
Also say in a comment that `sys/socket.h` is defining this macro.
Follow-up to 77b3bc239dCloses#20559
Originally split in 2006, but the issues cited are no longer present in
current code. As of now both `curl_setup.h` and `curl_setup_once.h` are
included once per compiler invocation, without recursion. The latter is
a sub-header of the former with no clear distinction in their contents.
Merge them to avoid having to decide where to put new global PP logic.
Also to make it easier to overview what gets defined/included globally
and in what order. (Perhaps even allowing some tidying up here.)
Follow-up to 77b3bc239dCloses#20555
- For compatibility reasons send both ALPN ids http/1.0 and http/1.1 for
HTTP/1.0 requests.
Prior to this change for compatibility reasons curl would send ALPN
http/1.1 for HTTP/1.0 requests, since some servers do not recognize
ALPN http/1.0. However some servers may recognize only ALPN http/1.0 for
HTTP/1.0 requests. Therefore curl now sends both.
Reported-by: programmerlexi@users.noreply.github.com
Fixes https://github.com/curl/curl/issues/20487
Closes https://github.com/curl/curl/pull/20533
- move macro to `curl_setup.h` (from curlx), and rename.
It's required by src, test servers, libtests. Also used by unit/tunit,
(which is fixable but this patch doesn't touch it.)
- special-case it for Windows/Cygwin/MS-DOS.
- build: drop `setmode()`/`_setmode()` detection.
This also avoids detecting the different `setmode()` on BSDs,
and a lot of complexity and overhead.
- use `CURL_O_BINARY`.
Follow-up to 250d613763#15787
Follow-up to 5e70566094#15169Closes#20539
The included local header starts with this same guard. The original
commit added it for fixing VMS builds along with many other changes, but
without mention of this specific one in the commit message.
`curl_setup.h` is included once, which includes `curl_setup_once.h`
once, even if the latter wouldn't have it's own guard.
Ref: 25f351424bCloses#20544
Several comments were outdated and parameters to create_conn() and
ConnectionExists() were not needed. Give functions better names and
consistently use terms `needle` and `conn`.
No functional change.
Closes#20464
To comply with official documentation. Also to make code compile with
`NO_OLDNAMES` (mingw-w64) or `_CRT_DECLARE_NONSTDC_NAMES=0` (MSVC) set.
Ref: #15652Closes#20516
When removing an easy handle from a multi, there was an optimization
to update the timer only when the removed handle had any timers.
With the introduction of the "dirty" bitset, easy handles can now cause
a timeout of 0 to be set without having anything in their timer list.
Removing such a handle needs to update the timer now always, so that
it may get cleared when there is nothing more to wait for.
The previous "not clearing a 0 timer" should not have any effect on
application's logic. Without clearing, the timer will fire and then
adjust itself to the proper value. But it would cause one more timer
fire than necessary.
Reported-by: Jan Macku
Fixes https://github.com/curl/curl/issues/20498
Closes https://github.com/curl/curl/pull/20502
Stop detecting this function and drop the local fallback.
Let us know if this update is causing an issue.
Notes:
- on Windows `_strdup()` is required instead.
- `strdup()/_strdup()` were required before this patch to build one of
the examples: `block_ip`.
- `strdup()/_strdup()` were required in 8.18.0 and earlier to build
tests.
Closes#20505
- de-dupe lib/src strdup/memdup functions into curlx.
- introduce `CURLX_STRDUP_LOW()` for mapping `strdup()`, and to do it at
one place within the code, in `curl_setup.h`.
- tests/server: use `curlx_strdup()`. (Also to fix building without
a system `strdup()`.)
- curlx/curlx.h: shorten and tidy up.
- adjust Windows build path to not need `HAVE_STRDUP`.
- build: stop detecting `HAVE_STRDUP` on Windows.
Closes#20497
