From 17d50e737bedbaab20c16bce4c87c8dc9df8c30e Mon Sep 17 00:00:00 2001 From: Steve Karmeinsky Date: Thu, 8 Oct 2026 23:57:50 +0100 Subject: [PATCH 1/3] storage backends: check malloc results, fix NULL dereferences and leaks - 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 --- src/store_memcached.c | 5 +++++ src/store_rados.c | 12 +++++++++++- src/store_ro_composite.c | 39 +++++++++++++++++++++++++++++++++++++++ src/store_ro_http_proxy.c | 4 ++++ 4 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/store_memcached.c b/src/store_memcached.c index 981b8b10..2764d8bc 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 c057a804..4b528114 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); @@ -352,7 +362,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 8faf4b4d..b21f9f95 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 69925fb7..6380dbda 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; @@ -299,6 +301,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; @@ -308,6 +311,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 f0e79dd6a20d60c0a7521cd60a0fe7cbe065870a Mon Sep 17 00:00:00 2001 From: Steve Karmeinsky Date: Thu, 8 Oct 2026 23:57:51 +0100 Subject: [PATCH 2/3] storage backends: fix memcached expiry and HTTP proxy cache bugs - 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 Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV --- src/store_memcached.c | 7 +++---- src/store_ro_http_proxy.c | 23 ++++++++++++++++++++--- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/store_memcached.c b/src/store_memcached.c index 2764d8bc..07546f03 100644 --- a/src/store_memcached.c +++ b/src/store_memcached.c @@ -240,7 +240,6 @@ static int memcached_metatile_expire(struct storage_backend * store, const char char * buf; size_t len; uint32_t flags; - uint64_t cas; memcached_return_t rc; //TODO: deal with options @@ -251,11 +250,11 @@ static int memcached_metatile_expire(struct storage_backend * store, const char return -1; } - //cas = memcached_result_cas(&rc); - ((struct stat_info *)buf)->expired = 1; - 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); if (rc != MEMCACHED_SUCCESS) { free(buf); diff --git a/src/store_ro_http_proxy.c b/src/store_ro_http_proxy.c index 6380dbda..61b0e098 100644 --- a/src/store_ro_http_proxy.c +++ b/src/store_ro_http_proxy.c @@ -152,19 +152,31 @@ static int ro_http_proxy_tile_retrieve(struct storage_backend * store, const cha case 404: { if (ctx->cache.tile != NULL) { free(ctx->cache.tile); + ctx->cache.tile = NULL; } + free(chunk.memory); ctx->cache.st_stat.size = -1; ctx->cache.st_stat.expired = 0; break; } - } + default: { + // Do not cache anything for unexpected responses, otherwise the previously + // cached tile would be served for these coordinates + g_logger(G_LOG_LEVEL_ERROR, "ro_http_proxy_tile_fetch: unexpected HTTP response code %li", httpCode); + free(chunk.memory); + ctx->cache.x = -1; + ctx->cache.y = -1; + ctx->cache.z = -1; + return -1; + } + } ctx->cache.x = x; ctx->cache.y = y; ctx->cache.z = z; - strcpy(ctx->cache.xmlname, xmlconfig); + snprintf(ctx->cache.xmlname, sizeof(ctx->cache.xmlname), "%s", xmlconfig); return 1; } } @@ -174,7 +186,12 @@ static int ro_http_proxy_tile_read(struct storage_backend * store, const char *x struct ro_http_proxy_ctx * ctx = (struct ro_http_proxy_ctx *)(store->storage_ctx); if (ro_http_proxy_tile_retrieve(store, xmlconfig, options, x, y, z) > 0) { - if (ctx->cache.st_stat.size > sz) { + if (ctx->cache.st_stat.size < 0) { + // Cached 404 + return -1; + } + + if ((size_t)ctx->cache.st_stat.size > sz) { g_logger(G_LOG_LEVEL_ERROR, "ro_http_proxy_tile_read: size was too big, overrun %lu %li", sz, ctx->cache.st_stat.size); return -1; } From a933f8c1ef45546e3e85965ce35338baba249884 Mon Sep 17 00:00:00 2001 From: Steve Karmeinsky Date: Thu, 22 Oct 2026 10:42:03 +0100 Subject: [PATCH 3/3] store_ro_composite: free inner backends, fix crash closing an unused store Follow-up to #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 Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV --- src/store_ro_composite.c | 14 +++++++++++++- tests/gen_tile_test.cpp | 31 +++++++++++++++++++++++-------- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/src/store_ro_composite.c b/src/store_ro_composite.c index b21f9f95..959cf056 100644 --- a/src/store_ro_composite.c +++ b/src/store_ro_composite.c @@ -202,8 +202,12 @@ static int ro_composite_metatile_expire(struct storage_backend * store, const ch static int ro_composite_close_storage(struct storage_backend * store) { struct ro_composite_ctx * ctx = (struct ro_composite_ctx *)(store->storage_ctx); + // close_storage() no longer frees the backend itself (#524), so free the + // two inner backends here, as the callers do for the outer one ctx->store_primary->close_storage(ctx->store_primary); + free(ctx->store_primary); ctx->store_secondary->close_storage(ctx->store_secondary); + free(ctx->store_secondary); if (ctx->cache.tile) { free(ctx->cache.tile); @@ -226,7 +230,7 @@ struct storage_backend * init_storage_ro_composite(const char * connection_strin return NULL; #else struct storage_backend * store = malloc(sizeof(struct storage_backend)); - struct ro_composite_ctx * ctx = malloc(sizeof(struct ro_composite_ctx)); + struct ro_composite_ctx * ctx = calloc(1, sizeof(struct ro_composite_ctx)); char * connection_string_primary; char * connection_string_secondary; char * tmp; @@ -304,6 +308,7 @@ struct storage_backend * init_storage_ro_composite(const char * connection_strin free(connection_string_primary); free(connection_string_secondary); ctx->store_primary->close_storage(ctx->store_primary); + free(ctx->store_primary); free(ctx); free(store); return NULL; @@ -315,12 +320,19 @@ struct storage_backend * init_storage_ro_composite(const char * connection_strin if (ctx->store_secondary == NULL) { g_logger(G_LOG_LEVEL_ERROR, "init_storage_ro_composite: failed to initialise secondary storage backend"); + free(connection_string_primary); + free(connection_string_secondary); ctx->store_primary->close_storage(ctx->store_primary); + free(ctx->store_primary); free(ctx); free(store); return NULL; } + // The backends keep their own copies of what they need from these + free(connection_string_primary); + free(connection_string_secondary); + ctx->render_size = 256; store->storage_ctx = ctx; diff --git a/tests/gen_tile_test.cpp b/tests/gen_tile_test.cpp index cff851b4..d5c612eb 100644 --- a/tests/gen_tile_test.cpp +++ b/tests/gen_tile_test.cpp @@ -1188,20 +1188,35 @@ TEST_CASE("rados storage-backend", "RADOS Tile storage backend") TEST_CASE("ro_composite storage-backend", "RO Composite Tile storage backend") { -#ifndef HAVE_CAIRO - SECTION("storage/initialise", "should return NULL") { - int found; - std::string err_log_lines, out_log_lines; + int found; + std::string err_log_lines, out_log_lines; + std::string tile_dir = P_tmpdir; + std::string valid = "composite:{default," + tile_dir + "}{default," + tile_dir + "}"; - start_capture(); - REQUIRE(init_storage_backend("composite:{") == NULL); - std::tie(err_log_lines, out_log_lines) = end_capture(); + start_capture(); + struct storage_backend *store = init_storage_backend(valid.c_str()); + std::tie(err_log_lines, out_log_lines) = end_capture(); + if (store == NULL) { + // Composite support depends on HAVE_CAIRO as seen by the C sources, which can differ + // from what this C++ file sees (e.g. Autotools builds), so check at run time found = err_log_lines.find("init_storage_ro_coposite: Support for compositing storage has not been compiled into this program"); REQUIRE(found > -1); + return; } -#endif + // Closing must free the inner backends and not touch the (never used) tile cache + store->close_storage(store); + free(store); + + start_capture(); + // Missing "}{" separator + REQUIRE(init_storage_backend(("composite:{default," + tile_dir + "}").c_str()) == NULL); + // Primary backend fails to initialise + REQUIRE(init_storage_backend(("composite:{default,/does/not/exist}{default," + tile_dir + "}").c_str()) == NULL); + // Secondary backend fails to initialise + REQUIRE(init_storage_backend(("composite:{default," + tile_dir + "}{default,/does/not/exist}").c_str()) == NULL); + end_capture(); } TEST_CASE("ro_http_proxy storage-backend", "RO HTTP Proxy Tile storage backend")