Skip to content

Commit f3e3ed6

Browse files
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 <noreply@anthropic.com>
1 parent 75dca9a commit f3e3ed6

5 files changed

Lines changed: 45 additions & 2 deletions

File tree

CLAUDE.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,12 @@ Tests use **Catch2** (v2.13.9) and live in `tests/`. The main suites:
129129

130130
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.
131131

132+
## Known Issues / Technical Debt
133+
134+
- **`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.
135+
- **`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.
136+
- **`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.
137+
132138
## Repository Layout
133139

134140
```

src/renderd.c

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -589,7 +589,20 @@ void *slave_thread(void * arg)
589589
struct protocol * req_slave;
590590

591591
req_slave = (struct protocol *)malloc(sizeof(struct protocol));
592+
593+
if (req_slave == NULL) {
594+
g_logger(G_LOG_LEVEL_ERROR, "slave_thread: failed to allocate memory for req_slave");
595+
return NULL;
596+
}
597+
592598
resp = (struct protocol *)malloc(sizeof(struct protocol));
599+
600+
if (resp == NULL) {
601+
g_logger(G_LOG_LEVEL_ERROR, "slave_thread: failed to allocate memory for resp");
602+
free(req_slave);
603+
return NULL;
604+
}
605+
593606
bzero(req_slave, sizeof(struct protocol));
594607
bzero(resp, sizeof(struct protocol));
595608

src/request_queue.c

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,15 @@ struct request_queue * request_queue_init()
491491
queue->renderHead.next = queue->renderHead.prev = &(queue->renderHead);
492492
queue->hashidxSize = HASHIDX_SIZE;
493493
queue->item_hashidx = (struct item_idx *) malloc(sizeof(struct item_idx) * queue->hashidxSize);
494+
495+
if (queue->item_hashidx == NULL) {
496+
g_logger(G_LOG_LEVEL_ERROR, "Failed to allocate memory for request queue hash index");
497+
pthread_cond_destroy(&(queue->qCond));
498+
pthread_mutex_destroy(&(queue->qLock));
499+
free(queue);
500+
return NULL;
501+
}
502+
494503
bzero(queue->item_hashidx, sizeof(struct item_idx) * queue->hashidxSize);
495504

496505
return queue;

src/store_file.c

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,13 @@ static int file_metatile_write(struct storage_backend * store, const char *xmlco
218218
xyzo_to_meta(meta_path, sizeof(meta_path), (char *)(store->storage_ctx), xmlconfig, options, x, y, z);
219219
g_logger(G_LOG_LEVEL_DEBUG, "Creating and writing a metatile to %s", meta_path);
220220

221-
tmp = malloc(sizeof(char) * strlen(meta_path) + 24);
221+
tmp = malloc(strlen(meta_path) + 24);
222+
223+
if (tmp == NULL) {
224+
g_logger(G_LOG_LEVEL_ERROR, "file_metatile_write: failed to allocate memory for temp path");
225+
return -1;
226+
}
227+
222228
snprintf(tmp, strlen(meta_path) + 24, "%s.%lu", meta_path, (unsigned long) pthread_self());
223229

224230
if (mkdirp(tmp)) {
@@ -245,7 +251,14 @@ static int file_metatile_write(struct storage_backend * store, const char *xmlco
245251
}
246252

247253
close(fd);
248-
rename(tmp, meta_path);
254+
255+
if (rename(tmp, meta_path) != 0) {
256+
g_logger(G_LOG_LEVEL_WARNING, "Error renaming temp file %s to %s: %s", tmp, meta_path, strerror(errno));
257+
unlink(tmp);
258+
free(tmp);
259+
return -1;
260+
}
261+
249262
free(tmp);
250263

251264
return sz;

src/store_ro_http_proxy.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,7 @@ struct storage_backend * init_storage_ro_http_proxy(const char * connection_stri
300300

301301
if (res != CURLE_OK) {
302302
g_logger(G_LOG_LEVEL_ERROR, "init_storage_ro_http_proxy: failed to initialise global curl: %s", curl_easy_strerror(res));
303+
free(ctx->baseurl);
303304
free(ctx);
304305
free(store);
305306
return NULL;
@@ -309,6 +310,7 @@ struct storage_backend * init_storage_ro_http_proxy(const char * connection_stri
309310

310311
if (!ctx->ctx) {
311312
g_logger(G_LOG_LEVEL_ERROR, "init_storage_ro_http_proxy: failed to initialise curl");
313+
free(ctx->baseurl);
312314
free(ctx);
313315
free(store);
314316
return NULL;

0 commit comments

Comments
 (0)