Repository navigation
Storage backends: fix leaks, NULL dereferences, memcached expiry and HTTP proxy cache bugs - #504
Merged
hummeltech merged 3 commits intoOct 22, 2026
Conversation
This was referenced Oct 9, 2026
- 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
stevekennedyuk
force-pushed
the
pr/storage-backend-fixes
branch
from
October 22, 2026 09:42
51a88c8 to
a1df08e
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 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); |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #504 +/- ##
==========================================
+ Coverage 81.54% 82.23% +0.69%
==========================================
Files 24 24
Lines 5645 5411 -234
==========================================
- Hits 4603 4450 -153
+ Misses 1042 961 -81 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…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
stevekennedyuk
force-pushed
the
pr/storage-backend-fixes
branch
from
October 22, 2026 09:59
a1df08e to
a933f8c
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.
Fixes in the memcached, RADOS, composite and HTTP-proxy storage backends (split out of #497 as suggested there, plus two bugs found later).
mallocresults, a NULL dereference whenstrstr()finds no separator instore_ro_composite, and leaks on error paths in all four backends.store_memcached: tile expiry passed an uninitialised CAS token tomemcached_cas(), so it almost always failed; now usesmemcached_replace().store_ro_http_proxy: a 404 freed the cached tile without clearing the pointer (double free on the next fetch) and leaked the response body; responses other than 200/404 served the previous tile for the new coordinates; unboundedstrcpyof the map name.store_ro_composite(follow-up to Addressingfree(store) in renderd_list not safe for some storages#524):close_storage()now frees the primary and secondary backends it closes, asrender_list/render_expireddo for the outer one, and the init error paths free the primary too. The context wasmalloc'd, so closing a composite store that had never read a tilefree()d an uninitialisedcache.tilepointer (segfault); it's nowcalloc'd. The parsed connection strings are freed after use.gen_tile_testgains real composite tests (open/close, malformed strings, failing primary/secondary backend), which crashed or reported leaks under ASan before this and now pass cleanly.Rebased onto the current
master.Independent of the other PRs in this series; it applies to
masteron its own and merges cleanly with each of them in any order.🤖 Generated with Claude Code
https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV