Repository navigation
Addressing free(store) in renderd_list not safe for some storages - #524
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #524 +/- ##
==========================================
+ Coverage 81.54% 81.72% +0.18%
==========================================
Files 24 24
Lines 5645 5353 -292
==========================================
- Hits 4603 4375 -228
+ Misses 1042 978 -64 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
stevekennedyuk
added a commit
to stevekennedyuk/mod_tile
that referenced
this pull request
Oct 22, 2026
…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: replace the composite test (which only covered builds without Cairo) with tests that open and close a composite store and check malformed connection strings and failing primary/secondary backends. Under ASan with leak detection these previously segfaulted / reported the leaked backends, and now pass cleanly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
stevekennedyuk
added a commit
to stevekennedyuk/mod_tile
that referenced
this pull request
Oct 22, 2026
…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: replace the composite test (which only covered builds without Cairo) with tests that open and close a composite store and check malformed connection strings and failing primary/secondary backends. Under ASan with leak detection these previously segfaulted / reported the leaked backends, and now pass cleanly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
Contributor
|
Thanks for this! One follow-up for |
stevekennedyuk
added a commit
to stevekennedyuk/mod_tile
that referenced
this pull request
Oct 22, 2026
…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
added a commit
to stevekennedyuk/mod_tile
that referenced
this pull request
Oct 22, 2026
…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
hummeltech
pushed a commit
that referenced
this pull request
Oct 22, 2026
…HTTP proxy cache bugs (#504) Fixes in the memcached, RADOS, composite and HTTP-proxy storage backends (split out of #497 as suggested there, plus two bugs found later). - Unchecked `malloc` results, a NULL dereference when `strstr()` finds no separator in `store_ro_composite`, and leaks on error paths in all four backends. - **`store_memcached`**: tile expiry passed an uninitialised CAS token to `memcached_cas()`, so it almost always failed; now uses `memcached_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; unbounded `strcpy` of the map name. - **`store_ro_composite`** (follow-up to #524): `close_storage()` now frees the primary and secondary backends it closes, as `render_list`/`render_expired` do for the outer one, and the init error paths free the primary too. The context was `malloc`'d, so closing a composite store that had never read a tile `free()`d an uninitialised `cache.tile` pointer (segfault); it's now `calloc`'d. The parsed connection strings are freed after use. `gen_tile_test` gains 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 `master` on its own and merges cleanly with each of them in any order. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01YForD7qSL9LT6FhMuAoKPV
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.
Ensures all
close_storagemethods do not also freestoreCloses #66