Repository navigation
CI: add sanitizer and -Werror jobs - #513
Draft
stevekennedyuk wants to merge 15 commits into
Draft
stevekennedyuk wants to merge 15 commits into
stevekennedyuk wants to merge 15 commits into
Conversation
stevekennedyuk
force-pushed
the
pr/sanitizer-ci
branch
from
October 10, 2026 14:12
4974ccc to
6e53b46
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #513 +/- ##
==========================================
+ Coverage 81.54% 81.80% +0.26%
==========================================
Files 24 24
Lines 5645 5452 -193
==========================================
- Hits 4603 4460 -143
+ Misses 1042 992 -50 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
stevekennedyuk
force-pushed
the
pr/sanitizer-ci
branch
from
October 10, 2026 14:36
6e53b46 to
efba45d
Compare
stevekennedyuk
force-pushed
the
pr/sanitizer-ci
branch
from
October 11, 2026 00:19
efba45d to
1f13b03
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| while ((ret_size < sizeof(struct protocol)) && (retry > 0)) { | ||
| ret_size = recv(pfd, resp + ret_size, (sizeof(struct protocol) | ||
| - ret_size), 0); | ||
| ssize_t n = recv(pfd, (char *)resp + ret_size, |
| size_t file_offset, tile_size; | ||
|
|
||
| if (m == NULL) { | ||
| snprintf(log_msg, PATH_MAX - 1, "Failed to allocate memory for metatile header\n"); |
| char * buf_raw; | ||
|
|
||
| if (m == NULL) { | ||
| snprintf(log_msg, 1024, "Failed to allocate memory for metatile header\n"); |
| rc = memcached_cas(store->storage_ctx, meta_path, strlen(meta_path), buf, len, 0, flags, cas); | ||
| // memcached_get() does not return a CAS value, so a compare-and-swap cannot be used here | ||
| // (the previous code passed an uninitialised CAS token). Replace the existing item instead. | ||
| rc = memcached_replace(store->storage_ctx, meta_path, strlen(meta_path), buf, len, 0, flags); |
| char * buf_raw; | ||
|
|
||
| if (m == NULL) { | ||
| snprintf(log_msg, 1024, "Failed to allocate memory for metatile header\n"); |
| ctx->cache.y = y; | ||
| ctx->cache.z = z; | ||
| strcpy(ctx->cache.xmlname, xmlconfig); | ||
| snprintf(ctx->cache.xmlname, sizeof(ctx->cache.xmlname), "%s", xmlconfig); |
stevekennedyuk
force-pushed
the
pr/sanitizer-ci
branch
from
October 19, 2026 13:14
1f13b03 to
dd62d86
Compare
stevekennedyuk
force-pushed
the
pr/sanitizer-ci
branch
from
October 22, 2026 09:44
dd62d86 to
a1c3172
Compare
- Compile sources shared between targets once via internal OBJECT
libraries (PIC so they also link into mod_tile.so). A full build with
tests goes from 104 to 37 compile steps.
- Generate config.h into the build tree instead of includes/, so the
source tree stays clean and several build dirs can coexist.
- Bump cmake_minimum_required to 3.15 (list(PREPEND) needs it).
- find_package(PkgConfig) explicitly (pkg_get_variable is used directly).
- Drop link_directories(${CMAKE_LIBRARY_PATH}).
- Use option() for boolean switches.
- New options: ENABLE_WARNINGS, ENABLE_WERROR, ENABLE_SANITIZERS.
- TILE_LOAD_DIRECTORY / TILE_LOAD_FILENAME can be set to override the
build-host /etc/os-release detection (packaging, cross builds).
- Print a feature summary of which optional backends were found.
- Add CMakePresets.json: default, dev, asan, tsan.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
Log messages and storage IDs are deliberately truncated to PATH_MAX-sized buffers, so this heuristic only produces false positives here. Only added when the compiler supports the flag. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
- configure.ac reads the version from CMakeLists.txt so it is only defined in one place (and fails loudly if it cannot). - Bug report URL points at GitHub issues rather than the old trac. - Use pkg-config for iniparser when a .pc file is available, falling back to the previous AC_CHECK_LIB probe. - configure prints a deprecation warning pointing at CMake; README marks Autotools as deprecated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
- With ENABLE_SANITIZERS containing "address", tests run with the ASan runtime in LD_PRELOAD, so the system (uninstrumented) httpd can load the instrumented mod_tile.so. Without this every Apache-based test fails under ASan. - TSan cannot be preloaded into uninstrumented processes, so the tsan test preset only runs the unit test executables. - tests/tsan.supp suppresses a lock-order report inside GDAL's plugin initialisation (on renderd's main thread, before any rendering). - The asan test preset halts on UBSan errors instead of only printing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
unit_tests compiles metatile.cpp and parameterize_style.cpp, which use
ICU (via Mapnik) directly. At -O0 GCC does not inline those calls away,
so a Debug build failed to link:
undefined reference to symbol '_ZNK6icu_7413UnicodeString10doHashCodeEv'
libicuuc.so.74: error adding symbols: DSO missing from command line
Add ${ICU_LIBRARIES}, as renderd and gen_tile_test already do.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
Catch2 v2 is end-of-life. Vendor the official v3.16.0 amalgamated
build (tests/catch/catch_amalgamated.{hpp,cpp}, with LICENSE and a
README recording the upstream tag/commit) in place of catch.hpp.
- Catch's implementation is compiled once (catch2_o, with
CATCH_AMALGAMATED_CUSTOM_MAIN and third-party warnings suppressed)
instead of in every test translation unit.
- catch_main.cpp now defines main() explicitly; gen_tile_test keeps
its own main() and drops CATCH_CONFIG_RUNNER.
- Catch::Matchers::Contains -> ContainsSubstring (v3 rename), including the
new unit_test_*.cpp suites.
- Autotools: build the Catch2 implementation once (catch2.o) and link it
into every test binary.
- Explicit #include <fstream> previously pulled in by catch.hpp.
- Default CMAKE_CXX_STANDARD is now 14 (v3 minimum; mapnik-config
already reports -std=c++14 for Mapnik 3), with a guard that bumps an
old cached value of 11 rather than failing.
Every suite runs the same number of test cases and assertions as
under v2.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
- request_queue.c: check malloc return before bzero; destroy mutex/cond and free queue on failure - renderd.c (slave_thread): check malloc return for req_slave and resp; free req_slave if resp alloc fails - renderd.c: check malloc for render_threads and slave_threads arrays - store_file.c: check malloc return before snprintf; check rename return value and unlink temp file on failure; check malloc(header_len) before use Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
renderd.c slave_thread recv loop: apply the same fix as upstream commit fddb0a9 applied to mod_tile.c -- the loop was overwriting ret_size with each recv() return value instead of accumulating it, and using struct pointer arithmetic (resp + ret_size) instead of byte arithmetic ((char*)resp + ret_size), so partial reads wrote into the wrong memory location. Also handle EINTR and separate n==0 (EOF) from n<0 (error). renderd.c request_exit: remove g_logger/strerror calls from this signal handler. Both functions are non-async-signal-safe (they use mutexes and malloc internally). Only write(2) -- which is async-signal-safe -- is needed here. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
GCC ignores a (void) cast on functions declared warn_unused_result, so the result of write() in the signal handler is assigned to a variable and discarded instead. Nothing useful can be done on failure there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
- store_memcached.c: check malloc(header_len) before use - store_rados.c: check malloc(header_len), malloc(sz2); fix ctx-only NULL check to also cover store; log error and return on failure - store_ro_http_proxy.c: free ctx->baseurl on curl init error paths; free chunk.memory on curl error paths to prevent memory leak when transfer is partially completed before failure - store_ro_composite.c: check strstr() results before pointer arithmetic (NULL deref if separator missing); check malloc for primary connection string; free intermediate strings on all error paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- store_memcached: metatile_expire passed an uninitialised CAS token to memcached_cas(); use memcached_replace() instead. - store_ro_http_proxy: 404 freed the cached tile without clearing the pointer (double free on the next fetch) and leaked the response body; any status other than 200/404 cached the previous tile under the new coordinates; explicit handling of a cached 404 in tile_read; bounded copy of xmlconfig instead of strcpy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
…store Follow-up to openstreetmap#524, which made close_storage() leave freeing the backend itself to the caller: - ro_composite_close_storage() closed its primary and secondary backends but never freed them; it now frees both, as render_list and render_expired do for the outer backend. The init error paths that close the primary backend now free it too. - init_storage_ro_composite() allocated its context with malloc(), so cache.tile was uninitialised and closing a composite store that had never read a tile called free() on garbage (segfault). Use calloc(). - The parsed primary/secondary connection strings were never freed on success or when the secondary backend failed to initialise; the backends keep their own copies of what they need. - gen_tile_test: the composite test only covered builds without Cairo, and decided that from the C++ HAVE_CAIRO, which can differ from what store_ro_composite.c sees (Autotools). It now checks at run time, and when composite support is built in, opens and closes a store and checks malformed connection strings and failing primary/secondary backends. Under ASan with leak detection this previously segfaulted / reported the leaked backends, and now passes cleanly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
The tile_requested bitmap was sized in bytes (fourpow/8 + 1) but read as unsigned ints, causing heap reads past the end at low zooms; also 0x01 << 31 was signed-overflow UB. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
The per-zoom bitmaps allocated one bit per possible metatile at every zoom level, i.e. ~2 GiB of address space with --max-zoom 20, and pages were touched (and kept) for every region of the world that appeared in the expiry list. Use a GHashTable of packed (z, x, y) keys instead, so memory is proportional to the number of tiles actually seen. For 200k random z20 tiles peak RSS drops from 1.27 GB to 103 MB; verbose output is byte-for-byte identical to the old implementation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
- Warnings: GCC and Clang builds with ENABLE_WARNINGS and ENABLE_WERROR, running the full test suite. - ASan + UBSan: Debug build with ENABLE_SANITIZERS=address,undefined, running the full suite including the Apache/renderd integration tests; UBSan halts on error. - TSan: unit test executables (TSan cannot be injected into the system httpd). - The sanitizer jobs run on the host rather than in a container so vm.mmap_rnd_bits can be lowered for TSan; the apache2/memcached system services started by installing the packages are stopped, as memcached's default port collides with the memcached_default tests. - Services log sanitizer errors to their own logs and may still exit successfully, so a final step fails the job if any report appears in build/tests/logs. - The CMake build action accepts EXTRA_CMAKE_OPTIONS. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
stevekennedyuk
force-pushed
the
pr/sanitizer-ci
branch
from
October 22, 2026 10:00
a1c3172 to
2b27a26
Compare
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.
Adds a Sanitizers & Warnings workflow so the bugs fixed in this series don't creep back.
Draft — stacked on all the other PRs in this series (#503, #504, #508, #511 and #512; #502, #505, #506, #507, #509 and #510 are already merged). Its own change is the last commit; I'll rebase it as the others are merged. Rebased onto the current
master.ENABLE_WARNINGS+ENABLE_WERROR, running the full test suite.build/tests/logs.unit_tests(TSan can't be injected into the systemhttpd).vm.mmap_rnd_bitscan be lowered for TSan. The apache2/memcached services that installing the packages starts are stopped, because memcached's default port clashes with thememcached_defaulttests.EXTRA_CMAKE_OPTIONS.🤖 Generated with Claude Code
https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV