Skip to content

Changes to fix memory leaks in renderd and mod_tile when using Postgres - #497

Closed
stevekennedyuk wants to merge 6 commits into
openstreetmap:masterfrom
stevekennedyuk:master
Closed

Changes to fix memory leaks in renderd and mod_tile when using Postgres #497
stevekennedyuk wants to merge 6 commits into
openstreetmap:masterfrom
stevekennedyuk:master

Conversation

@stevekennedyuk

@stevekennedyuk stevekennedyuk commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

I had a look and also used Claude to help so included the Claude.md file. OSM UK tried running on Ubuntu with Postgres and the virtual server ran out of memory and I isolated this in renderd ... it works (but is very slow) with single threaded sqlite

stevekennedyuk and others added 6 commits April 8, 2026 12:28
@stevekennedyuk @claude
Add CLAUDE.md with build instructions and architecture overview
75dca9a 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@stevekennedyuk @claude
Fix memory leaks and unchecked malloc return values
f3e3ed6 
- 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 <noreply@anthropic.com>
@stevekennedyuk @claude
Cache parameterized Mapnik maps per render thread to fix PostGIS memo…
7d379dc 
…ry 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<string, mapnik::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 <noreply@anthropic.com>
@stevekennedyuk @claude
Fix unchecked malloc returns, NULL dereferences, and resource leaks
3bc3c5e 
- 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 <noreply@anthropic.com>
@stevekennedyuk @claude
Update CLAUDE.md with newly found and fixed issues
62ee90c 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@stevekennedyuk @claude
Fix threading bugs: recv pointer arithmetic, signal handler safety, d…
510f589 
…ead 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 <noreply@anthropic.com>
@stevekennedyuk

stevekennedyuk commented Apr 14, 2026

Copy link
Copy Markdown
Contributor Author

I should say the original code pre fixes caused memory issues

@hummeltech

hummeltech commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Thanks, this is pretty great. Can you split these up into multiple pull requests, each targeting a specific goal?

I.e., there are a few files you/Claude modified which I suspect are not actually being used for your use-case that would better fit under a different pull request.

  • src/store_memcached.c
  • src/store_rados.c
  • src/store_ro_composite.c
  • src/store_ro_http_proxy.c

@stevekennedyuk

stevekennedyuk commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

Sorry I'm new to this Giithub stuff, I used to program C many years ago and at OSM UK tried to run this with Postgres and kept running out of memory so had a look and found some issues and then got Claude to help fixes.

@hummeltech

hummeltech commented Sep 27, 2026

Copy link
Copy Markdown
Collaborator

Sure thing, don't worry about it. It's more just to make things easier for review. Thanks!

This was referenced Oct 9, 2026
Merged
Merged
Merged
@stevekennedyuk

stevekennedyuk commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @hummeltech. As you suggested, I've split this up into focused pull requests. Each one applies to master on its own and merges cleanly with the others in any order, except where noted:

CLAUDE.md is left out of all of them. Closing this one in favour of those.

hummeltech pushed a commit that referenced this pull request Oct 22, 2026
@stevekennedyuk
Storage backends: fix leaks, NULL dereferences, memcached expiry and …
b36a84a 
…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
hummeltech pushed a commit that referenced this pull request Oct 22, 2026
@stevekennedyuk
renderd: fix partial-read, signal-handler and malloc-check bugs (#503)
93fafdb 
renderd memory and threading fixes (split out of #497).

- **slave_thread partial reads**: the same bug #469 fixed in `mod_tile.c` — the recv loop overwrote rather than accumulated the byte count and used struct pointer arithmetic, so partial reads wrote to the wrong place. Also handles `EINTR` and EOF.
- **Signal handler safety**: `request_exit()` called `g_logger`/`strerror`, which are not async-signal-safe; it now only calls `write(2)`.
- Unchecked `malloc` results and leaks on error paths in `renderd.c`, `request_queue.c` and `store_file.c`.

_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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@stevekennedyuk @hummeltech