memory: make function overrides work reliably in unity builds#17827
Closed
vszakats wants to merge 15 commits intocurl:masterfrom
Closed
memory: make function overrides work reliably in unity builds#17827vszakats wants to merge 15 commits intocurl:masterfrom
vszakats wants to merge 15 commits intocurl:masterfrom
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Jul 6, 2025
Fixing: - a local pair of malloc/free to use curl's memory allocators, and participate in memory tracing when enabled. - a raw free() in ECH code that's malloced in lib code, causing an invalid free, and reported by valgrind (in non-unity builds). Cherry-picked from curl#17827
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Jul 6, 2025
Fixing: - a local pair of malloc/free to use curl's memory allocators, and participate in memory tracing when enabled. - a raw free() in ECH code that's malloced in lib code, causing an invalid free, also reported by valgrind (in non-unity builds). Cherry-picked from curl#17827
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Jul 6, 2025
Follow-up to 3d02872 curl#16979 Cherry-picked from curl#17827
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Jul 6, 2025
Cherry-picked from curl#17827
This was referenced Jul 6, 2025
vszakats
added a commit
that referenced
this pull request
Jul 6, 2025
Just in case, and to match the pattern used for similar redefines. Cherry-picked from #17827
vszakats
added a commit
that referenced
this pull request
Jul 6, 2025
Fixing: - a raw `free()` in ECH code that's malloced in lib code, causing an invalid free, also reported by valgrind (in non-unity builds). And in unity builds adjusted to behave like non-unity via #17827: Ref: https://github.com/curl/curl/actions/runs/16093372427/job/45421778472?pr=17827#step:39:3321 - a local pair of `malloc()`/`free()` to use curl's memory allocators, and participate in memory tracking when enabled. Cherry-picked from #17827 Closes #17830
vszakats
added a commit
that referenced
this pull request
Jul 6, 2025
6b67c6a to
124a0b5
Compare
This way memdebug.h isn't required to use it.
finalize Alpine configs - formatting: tidy-up option order and job name. - enable unity mode again for the RR c-ares job. Follow-up to 17ab4d6 curl#16413
drop the undef/refer dance, it's unnecessary.
This reverts commit 9148ea6. No need: curl_setup.h is already included in each source and it does the undef already.
Member
Author
|
@bagder: This PR may also be necessary to avoid system symbol override issues in the handful (my manual count) of tests that include system headers. |
vszakats
added a commit
that referenced
this pull request
Sep 9, 2025
To avoid overriding the system symbol `accept`, which is a macro on some systems (AIX), and thus can't be called via the `(function)` PP trick. It's also problematic to reset such macro to its original value. Follow-up to 3bb5e58 #17827 Reported-by: Andrew Kirillov Fixes #18500 Closes #18501 Closes #18502
3 tasks
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Sep 10, 2025
…verrides Ref: curl#18502 Ref: curl@793a375 Follow-up to 3bb5e58 curl#17827 Reported-by: Andrew Kirillov Fixes curl#18510
vszakats
added a commit
that referenced
this pull request
Sep 10, 2025
vszakats
added a commit
that referenced
this pull request
Sep 20, 2025
Before this patch `accept4()`, `socket()`, `socketpair()`, `send()` and `recv()` system symbols were remapped via macros, using the same name, to local curl debug wrappers. This patch replaces these overrides by introducing curl-namespaced macros that map either to the system symbols or to their curl debug wrappers in `CURLDEBUG` (TrackMemory) builds. This follows a patch that implemented the same for `accept()`. The old method required tricks to make these redefines work in unity builds, and avoid them interfering with system headers. These tricks did not work for system symbols implemented as macros. The new method allows to setup these mappings once, without interfering with system headers, upstream macros, or unity builds. It makes builds more robust. Also: - checksrc: ban all mapped functions. - docs/examples: tidy up checksrc rules. Follow-up to 9863599 #18502 Follow-up to 3bb5e58 #17827 Closes #18503
7 tasks
vszakats
added a commit
that referenced
this pull request
Sep 29, 2025
By introducing wrappers for them in the curlx namespace: `curlx_fopen()`, `curlx_fdopen()`, `curlx_fclose()`. The undefine/redefine/`(function)()` methods broke on systems implementing these functions as macros. E.g. AIX 32-bit's `fopen()`. Also: - rename `lib/fopen.*` to `lib/curl_fopen.*` (for `Curl_fopen()`) to make room for the newly added `curlx/fopen.h`. - curlx: move file-related functions from `multibyte.c` to `fopen.c`. - tests/server: stop using the curl-specific `fopen()` implementation on Windows. Unicode isn't used by runtests, and it isn't critical to run tests on longs path. It can be re-enabled if this becomes necessary, or if the wrapper receives a feature that's critical for test servers. Reported-by: Andrew Kirillov Bug: #18510 (comment) Follow-up to bf7375e #18503 Follow-up to 9863599 #18502 Follow-up to 3bb5e58 #17827 Closes #18634
23 tasks
vszakats
added a commit
that referenced
this pull request
Nov 28, 2025
Before this patch curl used the C preprocessor to override standard memory allocation symbols: malloc, calloc, strdup, realloc, free. The goal of these is to replace them with curl's debug wrappers in `CURLDEBUG` builds, another was to replace them with the wrappers calling user-defined allocators in libcurl. This solution needed a bunch of workarounds to avoid breaking external headers: it relied on include order to do the overriding last. For "unity" builds it needed to reset overrides before external includes. Also in test apps, which are always built as single source files. It also needed the `(symbol)` trick to avoid overrides in some places. This would still not fix cases where the standard symbols were macros. It was also fragile and difficult to figure out which was the actual function behind an alloc or free call in a specific piece of code. This in turn caused bugs where the wrong allocator was accidentally called. To avoid these problems, this patch replaces this solution with `curlx_`-prefixed allocator macros, and mapping them _once_ to either the libcurl wrappers, the debug wrappers or the standard ones, matching the rest of the code in libtests. This concludes the long journey to avoid redefining standard functions in the curl codebase. Note: I did not update `packages/OS400/*.c` sources. They did not `#include` `curl_setup.h`, `curl_memory.h` or `memdebug.h`, meaning the overrides were never applied to them. This may or may not have been correct. For now I suppressed the direct use of standard allocators via a local `.checksrc`. Probably they (except for `curlcl.c`) should be updated to include `curl_setup.h` and use the `curlx_` macros. This patch changes mappings in two places: - `lib/curl_threads.c` in libtests: Before this patch it mapped to libcurl allocators. After, it maps to standard allocators, like the rest of libtests code. - `units`: before this patch it mapped to standard allocators. After, it maps to libcurl allocators. Also: - drop all position-dependent `curl_memory.h` and `memdebug.h` includes, and delete the now unnecessary headers. - rename `Curl_tcsdup` macro to `curlx_tcsdup` and define like the other allocators. - map `curlx_strdup()` to `_strdup()` on Windows (was: `strdup()`). To fix warnings silenced via `_CRT_NONSTDC_NO_DEPRECATE`. - multibyte: map `curlx_convert_*()` to `_strdup()` on Windows (was: `strdup()`). - src: do not reuse the `strdup` name for the local replacement. - lib509: call `_strdup()` on Windows (was: `strdup()`). - test1132: delete test obsoleted by this patch. - CHECKSRC.md: update text for `SNPRINTF`. - checksrc: ban standard allocator symbols. Follow-up to b12da22 #18866 Follow-up to db98daa #18844 Follow-up to 4deea93 #18814 Follow-up to 9678ff5 #18776 Follow-up to 10bac43 #18774 Follow-up to 20142f5 #18634 Follow-up to bf7375e #18503 Follow-up to 9863599 #18502 Follow-up to 3bb5e58 #17827 Closes #19626
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixing:
It should fix all other kinds of entaglement between curl's redefintions
of system symbols and system (or 3rd-party) headers sensitive to that.
It also syncs memory override behavior between unity & non-unity builds,
thus reducing build variations.
The idea is to define and declare everything once in
curl_setup.h,without overriding any system symbols with curl ones yet. Then, like
before this patch, override them, if necessary, in each source file via
curl_memory.handmemdebug.h, after including system headers.To ensure a clean slate with no overrides at the beginning of each
source file, reset all of them unconditionally at the end of
curl_setup.h, by includingcurl_mem_undef.h. (This assumescurl_setup.his always included first, which is already the casethroughout the codebase.)
curl_mem_undef.hcan also be included explicitly wherever overridesare causing problems. E.g. in tests which use unity-style builds and
a previously included
curl_memory.h/memdebug.hcan be spilling intoother source files.
The simplified role of the two override headers:
curl_memory.h: overrides system memory allocator functions tolibcurl ones, when memory tracing (aka
CURLDEBUG) is disabled.memdebug.h: overrides system memory allocator and some otherfunctions to curl debug functions, when memory tracing is enabled.
Changed made in this patch, step-by-step:
curl_setup.h.ALLOC_*macros tocurl_setup.h.curl_setup.h.Curl_safefree()macro tocurl_setup.h.(it's a regular macro, with a one-time, global, definition.)
curl_mem_undef.h.curl_mem_undef.hat the end, unconditionally,to reset system symbol macros after each inclusion.
sclose()andfake_sclose()incurl_setup.h. They are notsystem symbols, a one-time definition does the job.
Also:
Follow-up to 17ab4d6 GHA: add a build with c-ares + HTTP-RR on Alpine #16413
That said, I'd still find it better to avoid redefining system macros.
To communicate clearly the fact that they are not the original system
calls and they do behave differently. And, it would allow dropping the
undef/redef dance in each source file, and maintaining the logic with
it. The "last #include files should be in this order" comments in each
source would also become unnecessary. Also the trick of using
(func)(or interim macros) to call the non-overridden function whererequired. This method works for printf and most everything else already.
For
_tcsdup, socket and fopen functions this could work withoutdisturbing the codebase much.
Ref: #16428 (clean reboot of)
fake_sclose()is defined. move non-debug define tocurl_memory.h, or maybe move the logic tocfilters.c, its only user?CURL_EXTERNcleanup to separate PR → ws: drop redundantCURL_EXTERNfrom function definitions #17832CURL_MT_LOGFNAME_BUFSIZEtidy-up to separate PR. → memdebug.h: eliminate global macroCURL_MT_LOGFNAME_BUFSIZE#17833accept4→ curl_memory.h: fix to undefineaccept4#17831https://github.com/curl/curl/actions/runs/16093372427/job/45421778472?pr=17827#step:39:3321
scloseandfake_sclose.wcsdup/_wcsdupoverrides → memory: stop overriding unusedwcsdup()/_wcsdup()system functions #17840