From 75dca9a72e426c4f666ed8212cd1cfd47601f01a Mon Sep 17 00:00:00 2001 From: Steve Karmeinsky Date: Tue, 7 Apr 2026 13:58:09 +0100 Subject: [PATCH 1/6] Add CLAUDE.md with build instructions and architecture overview Co-Authored-By: Claude Sonnet 4.6 --- CLAUDE.md | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 144 insertions(+) create mode 100644 CLAUDE.md diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 00000000..c8271853 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,144 @@ +# CLAUDE.md + +This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. + +## Project Overview + +**mod_tile** is a high-performance tile serving system with two components: +- **mod_tile**: Apache 2 HTTP module that serves map tiles +- **renderd**: Daemon that renders tiles using Mapnik + +## Build System + +CMake is the primary build system (Autotools is the alternative). The project requires C99 and C++11 (C++17 for Mapnik 4+). + +### Ubuntu dependencies + +```sh +sudo apt --no-install-recommends --yes install \ + apache2 apache2-dev cmake curl g++ gcc git \ + libcairo2-dev libcurl4-openssl-dev libglib2.0-dev \ + libiniparser-dev libmapnik-dev libmemcached-dev librados-dev +``` + +### CMake Build (recommended) + +```sh +export CMAKE_BUILD_PARALLEL_LEVEL=$(nproc) + +cmake -B /tmp/mod_tile_build -S . \ + -DCMAKE_BUILD_TYPE=Release \ + -DCMAKE_INSTALL_LOCALSTATEDIR=/var \ + -DCMAKE_INSTALL_PREFIX=/usr \ + -DCMAKE_INSTALL_RUNSTATEDIR=/run \ + -DCMAKE_INSTALL_SYSCONFDIR=/etc \ + -DENABLE_TESTS=ON + +cmake --build /tmp/mod_tile_build + +# Run all tests +cd /tmp/mod_tile_build && ctest + +# Run a single test by name +cd /tmp/mod_tile_build && ctest -R --output-on-failure + +# Install +sudo cmake --install /tmp/mod_tile_build --strip +``` + +### Autotools Build + +```sh +./autogen.sh +./configure +make +sudo make install +``` + +### Key CMake Options + +| Option | Default | Description | +|--------|---------|-------------| +| `ENABLE_TESTS` | OFF | Build test suite (Catch2) | +| `USE_CAIRO` | ON | Cairo composite backend | +| `USE_CURL` | ON | HTTP proxy storage backend | +| `USE_MEMCACHED` | ON | Memcached storage backend | +| `USE_RADOS` | ON | Ceph RADOS storage backend | +| `MALLOC_LIB` | libc | Memory allocator: libc/jemalloc/mimalloc/tcmalloc | + +### Linting / Static Analysis + +CI runs `flawfinder` for security scanning (`.github/workflows/flawfinder-analysis.yml`) and a lint workflow (`.github/workflows/lint.yml`). + +## Architecture + +### Communication Flow + +``` +HTTP Client → mod_tile (Apache module) + ├── Storage backends (tile cache hit) → Response + └── Unix socket → renderd daemon + └── Request queue (5 priority levels) + └── Thread pool → Mapnik → Metatile storage +``` + +### Key Components + +**`src/mod_tile.c`** — Apache module: request handling, cache expiry heuristics, delay pool rate limiting, statistics. Config structures: `tile_config_rec` (per-directory), `tile_server_conf` (server-wide). + +**`src/renderd.c`** — Rendering daemon main loop; manages worker threads and Unix socket listener. + +**`src/gen_tile.cpp`** — Mapnik tile generation. Called by renderd worker threads. + +**`src/request_queue.c`** — Priority queue with hash-indexed deduplication. 5 levels: Normal, Priority, Low, Bulk, Dirty. + +**`src/renderd_config.c`** — Parses `renderd.conf` (INI format) into `renderd_config` / `xmlconfigitem` structs. + +**Storage backends** (pluggable via `includes/store.h` function-pointer interface): +- `store_file.c` — filesystem (default), stores 8×8 metatiles +- `store_memcached.c` — Memcached +- `store_rados.c` — Ceph RADOS +- `store_ro_http_proxy.c` — HTTP proxy (read-only) +- `store_ro_composite.c` — composite read-only (requires Cairo) +- `store_null.c` — no-op + +### Protocol + +mod_tile and renderd communicate over a Unix socket (default: `/run/renderd/renderd.sock`, TCP fallback: `localhost:7654`) using the protocol defined in `includes/protocol.h`. Protocol version is v3. Commands: `cmdRender`, `cmdDirty`, `cmdRenderPrio`, `cmdRenderLow`, `cmdRenderBulk`, `cmdDone`, `cmdNotDone`. + +### Metatile Format + +Tiles are stored in 8×8 metatile bundles (`METATILE = 8`) in a hashed directory structure. File format: `"META"` magic header + index of per-tile offsets/sizes. `includes/metatile.h` defines the layout; `src/metatile.cpp` is the C++ wrapper. + +### Important Constants (`includes/render_config.h`, `includes/mod_tile.h`) + +- `MAX_ZOOM = 20` +- `HASHIDX_SIZE = 2213` (request deduplication hash) +- Default tile directory: `/var/cache/renderd/tiles` +- `MAX_LOAD_OLD = 16`, `MAX_LOAD_MISSING = 50` — re-render thresholds + +## Tests + +Tests use **Catch2** (v2.13.9) and live in `tests/`. The main suites: + +- `gen_tile_test.cpp` — Mapnik rendering pipeline (largest suite) +- `renderd_config_test.cpp` — configuration parsing +- `renderd_test.cpp` — daemon core +- `render_expired_test.cpp`, `render_list_test.cpp`, `render_old_test.cpp` — utility programs +- `render_speedtest_test.cpp` — performance tool + +Test infrastructure uses `tests/httpd.conf.in` and `tests/renderd.conf.in` templates to spin up live Apache + renderd for integration tests. `tests/tiles.sha256sum` holds expected checksums for tile output validation. + +## Repository Layout + +``` +includes/ — public headers (protocol, store interface, config structs) +src/ — C/C++ source for mod_tile, renderd, storage backends, utilities +tests/ — Catch2 test suites + fixtures +cmake/ — custom Find*.cmake modules +docs/build/ — per-distro build instructions +docs/man/ — man pages +etc/ — example Apache and renderd config files +utils/ — example map data +.github/ — CI workflows (build-and-test, lint, flawfinder, docker) +``` From f3e3ed6dd3a901e10c3e51814aabf53000060bfe Mon Sep 17 00:00:00 2001 From: Steve Karmeinsky Date: Tue, 7 Apr 2026 14:56:20 +0100 Subject: [PATCH 2/6] Fix memory leaks and unchecked malloc return values - store_ro_http_proxy.c: free ctx->baseurl on curl init error paths - 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 - store_file.c: check malloc return before snprintf; check rename return value and unlink temp file on failure; drop redundant sizeof(char) Document remaining known issues in CLAUDE.md: - request_queue_close does not free queued items on shutdown - strcpy in store_ro_http_proxy.c lacks a prior length check on xmlconfig - bzero usage should be replaced with memset Co-Authored-By: Claude Sonnet 4.6 --- CLAUDE.md | 6 ++++++ src/renderd.c | 13 +++++++++++++ src/request_queue.c | 9 +++++++++ src/store_file.c | 17 +++++++++++++++-- src/store_ro_http_proxy.c | 2 ++ 5 files changed, 45 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c8271853..474f5863 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -129,6 +129,12 @@ Tests use **Catch2** (v2.13.9) and live in `tests/`. The main suites: Test infrastructure uses `tests/httpd.conf.in` and `tests/renderd.conf.in` templates to spin up live Apache + renderd for integration tests. `tests/tiles.sha256sum` holds expected checksums for tile output validation. +## Known Issues / Technical Debt + +- **`src/request_queue.c:request_queue_close`** — queued render items are not freed on shutdown (items in all five priority lists leak). The TODO comment is present in the source. Safe in practice because renderd only shuts down at process exit, but should be fixed for clean valgrind runs. +- **`src/store_ro_http_proxy.c:strcpy` at line 165** — `xmlconfig` is copied into `ctx->cache.xmlname[XMLCONFIG_MAX]` (41 bytes) without a prior length check. The caller is the storage backend interface which in practice receives validated xmlconfig names, but the copy is not bounds-safe. +- **`src/renderd.c` / `src/mod_tile.c` — `bzero` usage** — several files use the deprecated `bzero()` instead of `memset(..., 0, ...)`. Functionally equivalent on Linux but not strictly portable. + ## Repository Layout ``` diff --git a/src/renderd.c b/src/renderd.c index 81e0bda1..d54ff3b9 100644 --- a/src/renderd.c +++ b/src/renderd.c @@ -589,7 +589,20 @@ void *slave_thread(void * arg) struct protocol * req_slave; req_slave = (struct protocol *)malloc(sizeof(struct protocol)); + + if (req_slave == NULL) { + g_logger(G_LOG_LEVEL_ERROR, "slave_thread: failed to allocate memory for req_slave"); + return NULL; + } + resp = (struct protocol *)malloc(sizeof(struct protocol)); + + if (resp == NULL) { + g_logger(G_LOG_LEVEL_ERROR, "slave_thread: failed to allocate memory for resp"); + free(req_slave); + return NULL; + } + bzero(req_slave, sizeof(struct protocol)); bzero(resp, sizeof(struct protocol)); diff --git a/src/request_queue.c b/src/request_queue.c index 91a1358a..6bee27c1 100644 --- a/src/request_queue.c +++ b/src/request_queue.c @@ -491,6 +491,15 @@ struct request_queue * request_queue_init() queue->renderHead.next = queue->renderHead.prev = &(queue->renderHead); queue->hashidxSize = HASHIDX_SIZE; queue->item_hashidx = (struct item_idx *) malloc(sizeof(struct item_idx) * queue->hashidxSize); + + if (queue->item_hashidx == NULL) { + g_logger(G_LOG_LEVEL_ERROR, "Failed to allocate memory for request queue hash index"); + pthread_cond_destroy(&(queue->qCond)); + pthread_mutex_destroy(&(queue->qLock)); + free(queue); + return NULL; + } + bzero(queue->item_hashidx, sizeof(struct item_idx) * queue->hashidxSize); return queue; diff --git a/src/store_file.c b/src/store_file.c index b1409aa7..18775172 100644 --- a/src/store_file.c +++ b/src/store_file.c @@ -218,7 +218,13 @@ static int file_metatile_write(struct storage_backend * store, const char *xmlco xyzo_to_meta(meta_path, sizeof(meta_path), (char *)(store->storage_ctx), xmlconfig, options, x, y, z); g_logger(G_LOG_LEVEL_DEBUG, "Creating and writing a metatile to %s", meta_path); - tmp = malloc(sizeof(char) * strlen(meta_path) + 24); + tmp = malloc(strlen(meta_path) + 24); + + if (tmp == NULL) { + g_logger(G_LOG_LEVEL_ERROR, "file_metatile_write: failed to allocate memory for temp path"); + return -1; + } + snprintf(tmp, strlen(meta_path) + 24, "%s.%lu", meta_path, (unsigned long) pthread_self()); if (mkdirp(tmp)) { @@ -245,7 +251,14 @@ static int file_metatile_write(struct storage_backend * store, const char *xmlco } close(fd); - rename(tmp, meta_path); + + if (rename(tmp, meta_path) != 0) { + g_logger(G_LOG_LEVEL_WARNING, "Error renaming temp file %s to %s: %s", tmp, meta_path, strerror(errno)); + unlink(tmp); + free(tmp); + return -1; + } + free(tmp); return sz; diff --git a/src/store_ro_http_proxy.c b/src/store_ro_http_proxy.c index dabcf6ca..85c4ac3a 100644 --- a/src/store_ro_http_proxy.c +++ b/src/store_ro_http_proxy.c @@ -300,6 +300,7 @@ struct storage_backend * init_storage_ro_http_proxy(const char * connection_stri if (res != CURLE_OK) { g_logger(G_LOG_LEVEL_ERROR, "init_storage_ro_http_proxy: failed to initialise global curl: %s", curl_easy_strerror(res)); + free(ctx->baseurl); free(ctx); free(store); return NULL; @@ -309,6 +310,7 @@ struct storage_backend * init_storage_ro_http_proxy(const char * connection_stri if (!ctx->ctx) { g_logger(G_LOG_LEVEL_ERROR, "init_storage_ro_http_proxy: failed to initialise curl"); + free(ctx->baseurl); free(ctx); free(store); return NULL; From 7d379dc1a5588dc3bffedf08022955f211b79051 Mon Sep 17 00:00:00 2001 From: Steve Karmeinsky Date: Tue, 7 Apr 2026 15:09:16 +0100 Subject: [PATCH 3/6] Cache parameterized Mapnik maps per render thread to fix PostGIS memory leak When parameterize_style is configured, render() previously created a full copy of the Mapnik Map object and called set_datasource() on each affected layer for every single tile render. Each set_datasource() call creates a new PostGIS datasource, which registers a new connection pool entry in Mapnik's global ConnectionManager singleton. These pools were never released, causing renderd to exhaust memory rapidly under load. SQLite-backed styles were unaffected because the SQLite datasource has no global connection pool. Fix: add a std::map parameterized_map_cache to xmlmapconfig. On the first render for a given options string the parameterized Map is built as before and then moved into the cache. Subsequent renders with the same options reuse the cached Map directly, calling set_datasource() only once per (thread, style, options) combination. Document the remaining render_thread startup allocation leaks in CLAUDE.md. Co-Authored-By: Claude Sonnet 4.6 --- CLAUDE.md | 5 +++++ src/gen_tile.cpp | 53 ++++++++++++++++++++++++++++++++---------------- 2 files changed, 40 insertions(+), 18 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 474f5863..dbdf2871 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -134,6 +134,11 @@ Test infrastructure uses `tests/httpd.conf.in` and `tests/renderd.conf.in` templ - **`src/request_queue.c:request_queue_close`** — queued render items are not freed on shutdown (items in all five priority lists leak). The TODO comment is present in the source. Safe in practice because renderd only shuts down at process exit, but should be fixed for clean valgrind runs. - **`src/store_ro_http_proxy.c:strcpy` at line 165** — `xmlconfig` is copied into `ctx->cache.xmlname[XMLCONFIG_MAX]` (41 bytes) without a prior length check. The caller is the storage backend interface which in practice receives validated xmlconfig names, but the copy is not bounds-safe. - **`src/renderd.c` / `src/mod_tile.c` — `bzero` usage** — several files use the deprecated `bzero()` instead of `memset(..., 0, ...)`. Functionally equivalent on Linux but not strictly portable. +- **`src/gen_tile.cpp:render_thread` — startup `strndup`/`malloc` leaks** — `output_format`, `xmlfile`, `xmlname` (`strndup`), `prj` (`malloc`), and `store` (`init_storage_backend`) are allocated once per thread at startup and never freed. The render thread runs in an infinite loop and never exits, so these do not accumulate in practice. + +## Parameterized rendering cache + +When `parameterize_style` is configured (e.g. for multilingual maps), `render()` in `src/gen_tile.cpp` maintains a per-`xmlmapconfig` cache (`parameterized_map_cache`) mapping options strings to pre-built `mapnik::Map` copies. This prevents Mapnik's PostGIS datasource plugin from creating a new PostgreSQL connection pool entry on every render, which previously caused rapid memory exhaustion. The cache is per render-thread and unbounded in size; in practice the number of distinct options values is small (e.g. one per requested language). ## Repository Layout diff --git a/src/gen_tile.cpp b/src/gen_tile.cpp index d91769d5..87a6082f 100644 --- a/src/gen_tile.cpp +++ b/src/gen_tile.cpp @@ -89,6 +89,11 @@ struct projectionconfig { struct xmlmapconfig { Map map; + /* Cache of parameterized Map copies keyed by the options string. + * Avoids recreating PostGIS datasource objects (and their connection + * pools) on every render, which would accumulate in Mapnik's global + * ConnectionManager and exhaust memory under sustained load. */ + std::map parameterized_map_cache; const char *host; const char *htcphost; const char *output_format; @@ -247,31 +252,43 @@ static enum protoCmd render(struct xmlmapconfig *map, int x, int y, int z, char unsigned int render_size_tx = MIN(METATILE, map->prj->aspect_x * (1 << z)); unsigned int render_size_ty = MIN(METATILE, map->prj->aspect_y * (1 << z)); - map->map.resize(render_size_tx * map->tilesize, render_size_ty * map->tilesize); - map->map.zoom_to_box(tile2prjbounds(map->prj, x, y, z)); + /* Select the map to render with: a cached parameterized copy, or the base map. */ + mapnik::Map *render_map; + + if (map->parameterize_function) { + /* Look up or create a parameterized map for this options string. + * Re-using a cached copy avoids calling set_datasource() on every + * render, which would create a new PostGIS connection pool entry in + * Mapnik's global ConnectionManager each time and leak memory. */ + auto it = map->parameterized_map_cache.find(std::string(options)); + + if (it == map->parameterized_map_cache.end()) { + mapnik::Map parameterized = map->map; + map->parameterize_function(parameterized, options); + parameterized.load_fonts(); + auto inserted = map->parameterized_map_cache.emplace(std::string(options), std::move(parameterized)); + it = inserted.first; + g_logger(G_LOG_LEVEL_DEBUG, "Cached new parameterized map for options '%s' (cache size: %zu)", + options, map->parameterized_map_cache.size()); + } - if (map->map.buffer_size() == 0) { // Only set buffer size if the buffer size isn't explicitly set in the mapnik stylesheet. - map->map.set_buffer_size((map->tilesize >> 1) * map->scale); + render_map = &it->second; + } else { + render_map = &map->map; } - // m.zoom(size+1); + render_map->resize(render_size_tx * map->tilesize, render_size_ty * map->tilesize); + render_map->zoom_to_box(tile2prjbounds(map->prj, x, y, z)); + + if (render_map->buffer_size() == 0) { // Only set buffer size if the buffer size isn't explicitly set in the mapnik stylesheet. + render_map->set_buffer_size((map->tilesize >> 1) * map->scale); + } mapnik::image_32 buf(render_size_tx * map->tilesize, render_size_ty * map->tilesize); try { - if (map->parameterize_function) { - Map map_parameterized = map->map; - - map->parameterize_function(map_parameterized, options); - - map_parameterized.load_fonts(); - - mapnik::agg_renderer ren(map_parameterized, buf, map->scale); - ren.apply(); - } else { - mapnik::agg_renderer ren(map->map, buf, map->scale); - ren.apply(); - } + mapnik::agg_renderer ren(*render_map, buf, map->scale); + ren.apply(); } catch (std::exception const &ex) { g_logger(G_LOG_LEVEL_ERROR, "failed to render TILE %s %d %d-%d %d-%d", map->xmlname, z, x, x + render_size_tx - 1, y, y + render_size_ty - 1); g_logger(G_LOG_LEVEL_ERROR, " reason: %s", ex.what()); From 3bc3c5eda06f352ed83577f0044802152dcf4af7 Mon Sep 17 00:00:00 2001 From: Steve Karmeinsky Date: Wed, 8 Apr 2026 12:36:54 +0100 Subject: [PATCH 4/6] Fix unchecked malloc returns, NULL dereferences, and resource leaks - store_file.c: check malloc(header_len) before use - 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 - renderd.c: check malloc for render_threads and slave_threads arrays - store_ro_http_proxy.c: 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 --- src/renderd.c | 12 ++++++++++++ src/store_file.c | 5 +++++ src/store_memcached.c | 5 +++++ src/store_rados.c | 12 +++++++++++- src/store_ro_composite.c | 39 +++++++++++++++++++++++++++++++++++++++ src/store_ro_http_proxy.c | 2 ++ 6 files changed, 74 insertions(+), 1 deletion(-) diff --git a/src/renderd.c b/src/renderd.c index d54ff3b9..02441982 100644 --- a/src/renderd.c +++ b/src/renderd.c @@ -888,6 +888,12 @@ int main(int argc, char **argv) render_threads = (pthread_t *) malloc(sizeof(pthread_t) * config.num_threads); + if (render_threads == NULL) { + g_logger(G_LOG_LEVEL_CRITICAL, "Failed to allocate memory for render threads"); + close(fd); + return 7; + } + for (i = 0; i < config.num_threads; i++) { if (pthread_create(&render_threads[i], NULL, render_thread, (void *)maps)) { g_logger(G_LOG_LEVEL_CRITICAL, "Could not spawn rendering thread"); @@ -901,6 +907,12 @@ int main(int argc, char **argv) k = 0; slave_threads = (pthread_t *) malloc(sizeof(pthread_t) * num_slave_threads); + if (slave_threads == NULL) { + g_logger(G_LOG_LEVEL_CRITICAL, "Failed to allocate memory for slave threads"); + close(fd); + return 7; + } + for (i = 1; i < MAX_SLAVES; i++) { for (j = 0; j < config_slaves[i].num_threads; j++) { if (pthread_create(&slave_threads[k++], NULL, slave_thread, (void *) &config_slaves[i])) { diff --git a/src/store_file.c b/src/store_file.c index 18775172..91c7bfaa 100644 --- a/src/store_file.c +++ b/src/store_file.c @@ -74,6 +74,11 @@ static int file_tile_read(struct storage_backend * store, const char *xmlconfig, struct meta_layout *m = (struct meta_layout *)malloc(header_len); size_t file_offset, tile_size; + if (m == NULL) { + snprintf(log_msg, PATH_MAX - 1, "Failed to allocate memory for metatile header\n"); + return -1; + } + meta_offset = xyzo_to_meta(path, sizeof(path), store->storage_ctx, xmlconfig, options, x, y, z); fd = open(path, O_RDONLY); diff --git a/src/store_memcached.c b/src/store_memcached.c index dd296717..e9710496 100644 --- a/src/store_memcached.c +++ b/src/store_memcached.c @@ -80,6 +80,11 @@ static int memcached_tile_read(struct storage_backend * store, const char *xmlco memcached_return_t rc; char * buf_raw; + if (m == NULL) { + snprintf(log_msg, 1024, "Failed to allocate memory for metatile header\n"); + return -1; + } + mask = METATILE - 1; meta_offset = (x & mask) * METATILE + (y & mask); diff --git a/src/store_rados.c b/src/store_rados.c index 4d3cd16a..aae7c838 100644 --- a/src/store_rados.c +++ b/src/store_rados.c @@ -134,6 +134,11 @@ static int rados_tile_read(struct storage_backend * store, const char *xmlconfig int err; char * buf_raw; + if (m == NULL) { + snprintf(log_msg, 1024, "Failed to allocate memory for metatile header\n"); + return -1; + } + mask = METATILE - 1; meta_offset = (x & mask) * METATILE + (y & mask); @@ -234,6 +239,11 @@ static int rados_metatile_write(struct storage_backend * store, const char *xmlc char * buf2 = malloc(sz2); int err; + if (buf2 == NULL) { + g_logger(G_LOG_LEVEL_ERROR, "rados_metatile_write: failed to allocate memory for write buffer"); + return -1; + } + tile_stat.expired = 0; tile_stat.size = sz; tile_stat.mtime = time(NULL); @@ -351,7 +361,7 @@ struct storage_backend * init_storage_rados(const char * connection_string) int err; int i; - if (ctx == NULL) { + if (ctx == NULL || store == NULL) { free(ctx); free(store); return NULL; diff --git a/src/store_ro_composite.c b/src/store_ro_composite.c index 998be917..a11acc13 100644 --- a/src/store_ro_composite.c +++ b/src/store_ro_composite.c @@ -248,7 +248,23 @@ struct storage_backend * init_storage_ro_composite(const char * connection_strin } connection_string_secondary = strstr(connection_string, "}{"); + + if (connection_string_secondary == NULL) { + g_logger(G_LOG_LEVEL_ERROR, "init_storage_ro_composite: malformed connection string, missing '}{' separator: %s", connection_string); + free(ctx); + free(store); + return NULL; + } + connection_string_primary = malloc(strlen(connection_string) - strlen("composite:{") - strlen(connection_string_secondary) + 1); + + if (connection_string_primary == NULL) { + g_logger(G_LOG_LEVEL_ERROR, "init_storage_ro_composite: failed to allocate memory for primary connection string"); + free(ctx); + free(store); + return NULL; + } + memcpy(connection_string_primary, connection_string + strlen("composite:{"), strlen(connection_string) - strlen("composite:{") - strlen(connection_string_secondary)); connection_string_primary[strlen(connection_string) - strlen("composite:{") - strlen(connection_string_secondary)] = 0; connection_string_secondary = strdup(connection_string_secondary + 2); @@ -258,18 +274,41 @@ struct storage_backend * init_storage_ro_composite(const char * connection_strin g_logger(G_LOG_LEVEL_DEBUG, "init_storage_ro_composite: Secondary storage backend: %s", connection_string_secondary); tmp = strstr(connection_string_primary, ","); + + if (tmp == NULL) { + g_logger(G_LOG_LEVEL_ERROR, "init_storage_ro_composite: malformed primary connection string, missing ',' separator: %s", connection_string_primary); + free(connection_string_primary); + free(connection_string_secondary); + free(ctx); + free(store); + return NULL; + } + memcpy(ctx->xmlconfig_primary, connection_string_primary, tmp - connection_string_primary); ctx->xmlconfig_primary[tmp - connection_string_primary] = 0; ctx->store_primary = init_storage_backend(tmp + 1); if (ctx->store_primary == NULL) { g_logger(G_LOG_LEVEL_ERROR, "init_storage_ro_composite: failed to initialise primary storage backend"); + free(connection_string_primary); + free(connection_string_secondary); free(ctx); free(store); return NULL; } tmp = strstr(connection_string_secondary, ","); + + if (tmp == NULL) { + g_logger(G_LOG_LEVEL_ERROR, "init_storage_ro_composite: malformed secondary connection string, missing ',' separator: %s", connection_string_secondary); + free(connection_string_primary); + free(connection_string_secondary); + ctx->store_primary->close_storage(ctx->store_primary); + free(ctx); + free(store); + return NULL; + } + memcpy(ctx->xmlconfig_secondary, connection_string_secondary, tmp - connection_string_secondary); ctx->xmlconfig_secondary[tmp - connection_string_secondary] = 0; ctx->store_secondary = init_storage_backend(tmp + 1); diff --git a/src/store_ro_http_proxy.c b/src/store_ro_http_proxy.c index 85c4ac3a..aeb0183d 100644 --- a/src/store_ro_http_proxy.c +++ b/src/store_ro_http_proxy.c @@ -116,6 +116,7 @@ static int ro_http_proxy_tile_retrieve(struct storage_backend * store, const cha if (res != CURLE_OK) { g_logger(G_LOG_LEVEL_ERROR, "ro_http_proxy_tile_fetch: failed to retrieve file: %s", curl_easy_strerror(res)); + free(chunk.memory); ctx->cache.x = -1; ctx->cache.y = -1; ctx->cache.z = -1; @@ -126,6 +127,7 @@ static int ro_http_proxy_tile_retrieve(struct storage_backend * store, const cha if (res != CURLE_OK) { g_logger(G_LOG_LEVEL_ERROR, "ro_http_proxy_tile_fetch: failed to retrieve HTTP code: %s", curl_easy_strerror(res)); + free(chunk.memory); ctx->cache.x = -1; ctx->cache.y = -1; ctx->cache.z = -1; From 62ee90c25be292a2f6c02d17548f3a5822db3261 Mon Sep 17 00:00:00 2001 From: Steve Karmeinsky Date: Wed, 8 Apr 2026 12:37:24 +0100 Subject: [PATCH 5/6] Update CLAUDE.md with newly found and fixed issues Co-Authored-By: Claude Sonnet 4.6 --- CLAUDE.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CLAUDE.md b/CLAUDE.md index dbdf2871..cd06dd8a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -135,6 +135,7 @@ Test infrastructure uses `tests/httpd.conf.in` and `tests/renderd.conf.in` templ - **`src/store_ro_http_proxy.c:strcpy` at line 165** — `xmlconfig` is copied into `ctx->cache.xmlname[XMLCONFIG_MAX]` (41 bytes) without a prior length check. The caller is the storage backend interface which in practice receives validated xmlconfig names, but the copy is not bounds-safe. - **`src/renderd.c` / `src/mod_tile.c` — `bzero` usage** — several files use the deprecated `bzero()` instead of `memset(..., 0, ...)`. Functionally equivalent on Linux but not strictly portable. - **`src/gen_tile.cpp:render_thread` — startup `strndup`/`malloc` leaks** — `output_format`, `xmlfile`, `xmlname` (`strndup`), `prj` (`malloc`), and `store` (`init_storage_backend`) are allocated once per thread at startup and never freed. The render thread runs in an infinite loop and never exits, so these do not accumulate in practice. +- **`src/store_ro_composite.c` — `connection_string_secondary` (strdup) not freed on late error paths** — after the `strdup` on the secondary connection string, several subsequent error paths (store_primary init failure, store_secondary init failure) free it, but if `store_secondary` init succeeds and a later step fails the pointer may still leak depending on the code path. Low risk: composite storage is rarely used and init failures abort the process. ## Parameterized rendering cache From 510f589148319b6d0ca45ea99b71870d53e89c0b Mon Sep 17 00:00:00 2001 From: Steve Karmeinsky Date: Wed, 8 Apr 2026 12:43:55 +0100 Subject: [PATCH 6/6] Fix threading bugs: recv pointer arithmetic, signal handler safety, dead macro 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. render_config.h: remove dead HASHIDX_SIZE 22123 definition in the #else branch of #ifdef METATILE. METATILE is defined unconditionally two lines above, so this branch is unreachable. The authoritative value 2213 lives in request_queue.h. The dead definition caused confusion about which value would be used if METATILE were ever undefined. Co-Authored-By: Claude Sonnet 4.6 --- includes/render_config.h | 1 - src/renderd.c | 36 +++++++++++++++++++++++++----------- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/includes/render_config.h b/includes/render_config.h index cbadae0f..084db6f4 100644 --- a/includes/render_config.h +++ b/includes/render_config.h @@ -121,7 +121,6 @@ #define QUEUE_MAX (1024) #define REQ_LIMIT (512) #define DIRTY_LIMIT (10000) -#define HASHIDX_SIZE 22123 #endif // Penalty for client making an invalid request (in seconds) diff --git a/src/renderd.c b/src/renderd.c index 02441982..06c6be50 100644 --- a/src/renderd.c +++ b/src/renderd.c @@ -170,14 +170,11 @@ enum protoCmd rx_request(struct protocol *req, int fd) void request_exit(void) { - // Any write to the exit pipe will trigger a graceful exit + // Any write to the exit pipe will trigger a graceful exit. + // This function is called from a signal handler, so only async-signal-safe + // functions (write(2)) may be used here — no g_logger, no strerror. char c = 0; - - g_logger(G_LOG_LEVEL_INFO, "Sending exit request"); - - if (write(exit_pipe_fd, &c, sizeof(c)) < 0) { - g_logger(G_LOG_LEVEL_ERROR, "Failed to write to the exit pipe: %s", strerror(errno)); - } + (void)write(exit_pipe_fd, &c, sizeof(c)); } void process_loop(int listen_fd) @@ -683,10 +680,27 @@ void *slave_thread(void * arg) retry = 10; 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, + sizeof(struct protocol) - ret_size, 0); + + if (n < 0) { + if (errno == EINTR) { + continue; + } + + if (errno == EPIPE) { + close(pfd); + pfd = FD_INVALID; + ret_size = 0; + g_logger(G_LOG_LEVEL_ERROR, "Pipe to Renderd slave closed"); + break; + } + + retry--; + continue; + } - if ((errno == EPIPE) || ret_size == 0) { + if (n == 0) { close(pfd); pfd = FD_INVALID; ret_size = 0; @@ -694,7 +708,7 @@ void *slave_thread(void * arg) break; } - retry--; + ret_size += n; } if (ret_size < sizeof(struct protocol)) {