{"thread":{"id":"62694","subject":"[PATCH 0/4] reftable: fix realloc error handling","startedAt":"2024-12-25T18:33:18Z","lastAt":"2024-12-30T07:25:25Z","messageCount":25,"participants":["René Scharfe","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"509557","messageId":"2b9fba8d-be63-4145-9d25-a2151e422cfa@web.de","threadId":"62694","inReplyTo":null,"subject":"[PATCH 0/4] reftable: fix realloc error handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-25T18:33:07Z","receivedAt":"2024-12-25T18:33:18Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The current handling of reallocation errors leaks the original\nallocation in most cases and corrupts the capacity variable.  Fix\nthat in REFTABLE_ALLOC_GROW and by providing a new macro\nREFTABLE_ALLOC_GROW_OR_NULL -- solve this somewhat tricky issue\ncentrally, with minimal impact to calling code.\n\nAnd the last two patches add error handling to the remaining\nplaces that still lack it.\n\n  reftable: avoid leaks on realloc error\n  reftable: fix allocation count on realloc error\n  reftable: handle realloc error in parse_names()\n  t-reftable-merged: handle realloc errors\n\n reftable/basics.c                | 14 ++++-------\n reftable/basics.h                | 41 +++++++++++++++++++++++++-------\n reftable/block.c                 | 10 ++++----\n reftable/pq.c                    |  2 +-\n reftable/record.c                | 12 +++++-----\n reftable/stack.c                 |  8 ++++---\n reftable/writer.c                |  5 ++--\n t/unit-tests/t-reftable-merged.c |  4 ++--\n 8 files changed, 60 insertions(+), 36 deletions(-)\n\n--\n2.47.1\n\n"},{"id":"509558","messageId":"9b2f4baa-b602-4cc5-8dfc-dd941b1d7af6@web.de","threadId":"62694","inReplyTo":"2b9fba8d-be63-4145-9d25-a2151e422cfa@web.de","subject":"[PATCH 1/4] reftable: avoid leaks on realloc error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-25T18:38:29Z","receivedAt":"2024-12-25T18:38:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"When realloc(3) fails, it returns NULL and keeps the original allocation\nintact.  REFTABLE_ALLOC_GROW overwrites both the original pointer and\nthe allocation count variable in that case, simultaneously leaking the\noriginal allocation and misrepresenting the number of storable items.\n\nparse_names() and reftable_buf_add() avoid leaking by restoring the\noriginal pointer value on failure, but all other callers seem to be OK\nwith losing the old allocation.  Add a new variant of the macro,\nREFTABLE_ALLOC_GROW_OR_NULL, which plugs the leak and zeros the\nallocation counter.  Use it for those callers.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n reftable/basics.h | 10 ++++++++++\n reftable/block.c  | 10 ++++++----\n reftable/pq.c     |  2 +-\n reftable/record.c | 12 ++++++------\n reftable/stack.c  |  8 +++++---\n reftable/writer.c |  5 +++--\n 6 files changed, 31 insertions(+), 16 deletions(-)\n\ndiff --git a/reftable/basics.h b/reftable/basics.h\nindex 36beda2c25..259f4c274c 100644\n--- a/reftable/basics.h\n+++ b/reftable/basics.h\n@@ -129,6 +129,16 @@ char *reftable_strdup(const char *str);\n \t\t\tREFTABLE_REALLOC_ARRAY(x, alloc); \\\n \t\t} \\\n \t} while (0)\n+\n+#define REFTABLE_ALLOC_GROW_OR_NULL(x, nr, alloc) do { \\\n+\tvoid *reftable_alloc_grow_or_null_orig_ptr = (x); \\\n+\tREFTABLE_ALLOC_GROW((x), (nr), (alloc)); \\\n+\tif (!(x)) { \\\n+\t\treftable_free(reftable_alloc_grow_or_null_orig_ptr); \\\n+\t\talloc = 0; \\\n+\t} \\\n+} while (0)\n+\n #define REFTABLE_FREE_AND_NULL(p) do { reftable_free(p); (p) = NULL; } while (0)\n\n #ifndef REFTABLE_ALLOW_BANNED_ALLOCATORS\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 0198078485..9858bbc7c5 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -53,7 +53,8 @@ static int block_writer_register_restart(struct block_writer *w, int n,\n \tif (2 + 3 * rlen + n > w->block_size - w->next)\n \t\treturn -1;\n \tif (is_restart) {\n-\t\tREFTABLE_ALLOC_GROW(w->restarts, w->restart_len + 1, w->restart_cap);\n+\t\tREFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,\n+\t\t\t\t\t    w->restart_cap);\n \t\tif (!w->restarts)\n \t\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\tw->restarts[w->restart_len++] = w->next;\n@@ -176,7 +177,8 @@ int block_writer_finish(struct block_writer *w)\n \t\t * is guaranteed to return `Z_STREAM_END`.\n \t\t */\n \t\tcompressed_len = deflateBound(w->zstream, src_len);\n-\t\tREFTABLE_ALLOC_GROW(w->compressed, compressed_len, w->compressed_cap);\n+\t\tREFTABLE_ALLOC_GROW_OR_NULL(w->compressed, compressed_len,\n+\t\t\t\t\t    w->compressed_cap);\n \t\tif (!w->compressed) {\n \t\t\tret = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\t\treturn ret;\n@@ -235,8 +237,8 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,\n \t\tuLong src_len = block->len - block_header_skip;\n\n \t\t/* Log blocks specify the *uncompressed* size in their header. */\n-\t\tREFTABLE_ALLOC_GROW(br->uncompressed_data, sz,\n-\t\t\t\t    br->uncompressed_cap);\n+\t\tREFTABLE_ALLOC_GROW_OR_NULL(br->uncompressed_data, sz,\n+\t\t\t\t\t    br->uncompressed_cap);\n \t\tif (!br->uncompressed_data) {\n \t\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\t\tgoto done;\ndiff --git a/reftable/pq.c b/reftable/pq.c\nindex 6ee1164dd3..5591e875e1 100644\n--- a/reftable/pq.c\n+++ b/reftable/pq.c\n@@ -49,7 +49,7 @@ int merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry\n {\n \tsize_t i = 0;\n\n-\tREFTABLE_ALLOC_GROW(pq->heap, pq->len + 1, pq->cap);\n+\tREFTABLE_ALLOC_GROW_OR_NULL(pq->heap, pq->len + 1, pq->cap);\n \tif (!pq->heap)\n \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n \tpq->heap[pq->len++] = *e;\ndiff --git a/reftable/record.c b/reftable/record.c\nindex fb5652ed57..04429d23fe 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -246,8 +246,8 @@ static int reftable_ref_record_copy_from(void *rec, const void *src_rec,\n \tif (src->refname) {\n \t\tsize_t refname_len = strlen(src->refname);\n\n-\t\tREFTABLE_ALLOC_GROW(ref->refname, refname_len + 1,\n-\t\t\t\t    ref->refname_cap);\n+\t\tREFTABLE_ALLOC_GROW_OR_NULL(ref->refname, refname_len + 1,\n+\t\t\t\t\t    ref->refname_cap);\n \t\tif (!ref->refname) {\n \t\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\t\tgoto out;\n@@ -385,7 +385,7 @@ static int reftable_ref_record_decode(void *rec, struct reftable_buf key,\n \tSWAP(r->refname, refname);\n \tSWAP(r->refname_cap, refname_cap);\n\n-\tREFTABLE_ALLOC_GROW(r->refname, key.len + 1, r->refname_cap);\n+\tREFTABLE_ALLOC_GROW_OR_NULL(r->refname, key.len + 1, r->refname_cap);\n \tif (!r->refname) {\n \t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\tgoto done;\n@@ -839,7 +839,7 @@ static int reftable_log_record_decode(void *rec, struct reftable_buf key,\n \tif (key.len <= 9 || key.buf[key.len - 9] != 0)\n \t\treturn REFTABLE_FORMAT_ERROR;\n\n-\tREFTABLE_ALLOC_GROW(r->refname, key.len - 8, r->refname_cap);\n+\tREFTABLE_ALLOC_GROW_OR_NULL(r->refname, key.len - 8, r->refname_cap);\n \tif (!r->refname) {\n \t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\tgoto done;\n@@ -947,8 +947,8 @@ static int reftable_log_record_decode(void *rec, struct reftable_buf key,\n \t}\n \tstring_view_consume(&in, n);\n\n-\tREFTABLE_ALLOC_GROW(r->value.update.message, scratch->len + 1,\n-\t\t\t    r->value.update.message_cap);\n+\tREFTABLE_ALLOC_GROW_OR_NULL(r->value.update.message, scratch->len + 1,\n+\t\t\t\t    r->value.update.message_cap);\n \tif (!r->value.update.message) {\n \t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\tgoto done;\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 634f0c5425..531660a49f 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -317,7 +317,9 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\t\t\t * thus need to keep them alive here, which we\n \t\t\t\t * do by bumping their refcount.\n \t\t\t\t */\n-\t\t\t\tREFTABLE_ALLOC_GROW(reused, reused_len + 1, reused_alloc);\n+\t\t\t\tREFTABLE_ALLOC_GROW_OR_NULL(reused,\n+\t\t\t\t\t\t\t    reused_len + 1,\n+\t\t\t\t\t\t\t    reused_alloc);\n \t\t\t\tif (!reused) {\n \t\t\t\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\t\t\t\tgoto done;\n@@ -949,8 +951,8 @@ int reftable_addition_add(struct reftable_addition *add,\n \tif (err < 0)\n \t\tgoto done;\n\n-\tREFTABLE_ALLOC_GROW(add->new_tables, add->new_tables_len + 1,\n-\t\t\t    add->new_tables_cap);\n+\tREFTABLE_ALLOC_GROW_OR_NULL(add->new_tables, add->new_tables_len + 1,\n+\t\t\t\t    add->new_tables_cap);\n \tif (!add->new_tables) {\n \t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\tgoto done;\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 624e90fb53..740c98038e 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -254,7 +254,8 @@ static int writer_index_hash(struct reftable_writer *w, struct reftable_buf *has\n \tif (key->offset_len > 0 && key->offsets[key->offset_len - 1] == off)\n \t\treturn 0;\n\n-\tREFTABLE_ALLOC_GROW(key->offsets, key->offset_len + 1, key->offset_cap);\n+\tREFTABLE_ALLOC_GROW_OR_NULL(key->offsets, key->offset_len + 1,\n+\t\t\t\t    key->offset_cap);\n \tif (!key->offsets)\n \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n \tkey->offsets[key->offset_len++] = off;\n@@ -820,7 +821,7 @@ static int writer_flush_nonempty_block(struct reftable_writer *w)\n \t * Note that this also applies when flushing index blocks, in which\n \t * case we will end up with a multi-level index.\n \t */\n-\tREFTABLE_ALLOC_GROW(w->index, w->index_len + 1, w->index_cap);\n+\tREFTABLE_ALLOC_GROW_OR_NULL(w->index, w->index_len + 1, w->index_cap);\n \tif (!w->index)\n \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n\n--\n2.47.1\n"},{"id":"509559","messageId":"33bbacc7-1727-4efd-9cbf-3c9abfa94d8c@web.de","threadId":"62694","inReplyTo":"2b9fba8d-be63-4145-9d25-a2151e422cfa@web.de","subject":"[PATCH 2/4] reftable: fix allocation count on realloc error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-25T18:38:35Z","receivedAt":"2024-12-25T18:38:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"When realloc(3) fails, it returns NULL and keeps the original allocation\nintact.  REFTABLE_ALLOC_GROW overwrites both the original pointer and\nthe allocation count variable in that case, simultaneously leaking the\noriginal allocation and misrepresenting the number of storable items.\n\nparse_names() avoids the leak by keeping the original pointer if\nreallocation fails, but still increase the allocation count in such a\ncase as if it succeeded.  That's OK, because the error handling code\njust frees everything and doesn't look at names_cap anymore.\n\nreftable_buf_add() does the same, but here it is a problem as it leaves\nthe reftable_buf in a broken state, with ->alloc being roughly twice as\nbig as the actually allocated memory, allowing out-of-bounds writes in\nsubsequent calls.\n\nReimplement REFTABLE_ALLOC_GROW to avoid leaks, keep allocation counts\nin sync and still signal failures to callers while avoiding code\nduplication in callers.  Make it an expression that evaluates to 0 if no\nreallocation is needed or it succeeded and 1 on failure while keeping\nthe original pointer and allocation counter values.\n\nAdjust REFTABLE_ALLOC_GROW_OR_NULL to the new calling convention for\nREFTABLE_ALLOC_GROW, but keep its support for non-size_t alloc variables\nfor now.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n reftable/basics.c | 11 +++--------\n reftable/basics.h | 39 ++++++++++++++++++++++++++-------------\n 2 files changed, 29 insertions(+), 21 deletions(-)\n\ndiff --git a/reftable/basics.c b/reftable/basics.c\nindex 70b1091d14..cd6b39dbe9 100644\n--- a/reftable/basics.c\n+++ b/reftable/basics.c\n@@ -124,11 +124,8 @@ int reftable_buf_add(struct reftable_buf *buf, const void *data, size_t len)\n \tsize_t newlen = buf->len + len;\n\n \tif (newlen + 1 > buf->alloc) {\n-\t\tchar *reallocated = buf->buf;\n-\t\tREFTABLE_ALLOC_GROW(reallocated, newlen + 1, buf->alloc);\n-\t\tif (!reallocated)\n+\t\tif (REFTABLE_ALLOC_GROW(buf->buf, newlen + 1, buf->alloc))\n \t\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tbuf->buf = reallocated;\n \t}\n\n \tmemcpy(buf->buf + buf->len, data, len);\n@@ -233,11 +230,9 @@ char **parse_names(char *buf, int size)\n \t\t\tnext = end;\n \t\t}\n \t\tif (p < next) {\n-\t\t\tchar **names_grown = names;\n-\t\t\tREFTABLE_ALLOC_GROW(names_grown, names_len + 1, names_cap);\n-\t\t\tif (!names_grown)\n+\t\t\tif (REFTABLE_ALLOC_GROW(names, names_len + 1,\n+\t\t\t\t\t\tnames_cap))\n \t\t\t\tgoto err;\n-\t\t\tnames = names_grown;\n\n \t\t\tnames[names_len] = reftable_strdup(p);\n \t\t\tif (!names[names_len++])\ndiff --git a/reftable/basics.h b/reftable/basics.h\nindex 259f4c274c..fa5d75868b 100644\n--- a/reftable/basics.h\n+++ b/reftable/basics.h\n@@ -120,22 +120,35 @@ char *reftable_strdup(const char *str);\n #define REFTABLE_ALLOC_ARRAY(x, alloc) (x) = reftable_malloc(st_mult(sizeof(*(x)), (alloc)))\n #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x)))\n #define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))\n-#define REFTABLE_ALLOC_GROW(x, nr, alloc) \\\n-\tdo { \\\n-\t\tif ((nr) > alloc) { \\\n-\t\t\talloc = 2 * (alloc) + 1; \\\n-\t\t\tif (alloc < (nr)) \\\n-\t\t\t\talloc = (nr); \\\n-\t\t\tREFTABLE_REALLOC_ARRAY(x, alloc); \\\n-\t\t} \\\n-\t} while (0)\n+\n+static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,\n+\t\t\t\t\tsize_t *allocp)\n+{\n+\tvoid *new_p;\n+\tsize_t alloc = *allocp * 2 + 1;\n+\tif (alloc < nelem)\n+\t\talloc = nelem;\n+\tnew_p = reftable_realloc(p, st_mult(elsize, alloc));\n+\tif (!new_p)\n+\t\treturn p;\n+\t*allocp = alloc;\n+\treturn new_p;\n+}\n+\n+#define REFTABLE_ALLOC_GROW(x, nr, alloc) ( \\\n+\t(nr) > (alloc) && ( \\\n+\t\t(x) = reftable_alloc_grow((x), (nr), sizeof(*(x)), &(alloc)), \\\n+\t\t(nr) > (alloc) \\\n+\t) \\\n+)\n\n #define REFTABLE_ALLOC_GROW_OR_NULL(x, nr, alloc) do { \\\n-\tvoid *reftable_alloc_grow_or_null_orig_ptr = (x); \\\n-\tREFTABLE_ALLOC_GROW((x), (nr), (alloc)); \\\n-\tif (!(x)) { \\\n-\t\treftable_free(reftable_alloc_grow_or_null_orig_ptr); \\\n+\tsize_t reftable_alloc_grow_or_null_alloc = alloc; \\\n+\tif (REFTABLE_ALLOC_GROW((x), (nr), reftable_alloc_grow_or_null_alloc)) { \\\n+\t\treftable_free(x); \\\n \t\talloc = 0; \\\n+\t} else { \\\n+\t\talloc = reftable_alloc_grow_or_null_alloc; \\\n \t} \\\n } while (0)\n\n--\n2.47.1\n"},{"id":"509560","messageId":"3ca514b7-63f8-49cb-9816-627d706c6145@web.de","threadId":"62694","inReplyTo":"2b9fba8d-be63-4145-9d25-a2151e422cfa@web.de","subject":"[PATCH 3/4] reftable: handle realloc error in parse_names()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-25T18:38:40Z","receivedAt":"2024-12-25T18:38:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Check the final reallocation for adding the terminating NULL and handle\nit just like those in the loop.  Simply use REFTABLE_ALLOC_GROW instead\nof keeping the REFTABLE_REALLOC_ARRAY call and adding code to preserve\nthe original pointer value around it.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n reftable/basics.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/reftable/basics.c b/reftable/basics.c\nindex cd6b39dbe9..fe2b83ff83 100644\n--- a/reftable/basics.c\n+++ b/reftable/basics.c\n@@ -241,7 +241,8 @@ char **parse_names(char *buf, int size)\n \t\tp = next + 1;\n \t}\n\n-\tREFTABLE_REALLOC_ARRAY(names, names_len + 1);\n+\tif (REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap))\n+\t\tgoto err;\n \tnames[names_len] = NULL;\n\n \treturn names;\n--\n2.47.1\n"},{"id":"509561","messageId":"6084c017-9557-478b-b485-a1c1a21842e7@web.de","threadId":"62694","inReplyTo":"2b9fba8d-be63-4145-9d25-a2151e422cfa@web.de","subject":"[PATCH 4/4] t-reftable-merged: check realloc errors","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-25T18:38:49Z","receivedAt":"2024-12-25T18:38:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Report reallocation errors in unit tests, like everywhere else.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/unit-tests/t-reftable-merged.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\nindex a12bd0e1a3..60836f80d6 100644\n--- a/t/unit-tests/t-reftable-merged.c\n+++ b/t/unit-tests/t-reftable-merged.c\n@@ -178,7 +178,7 @@ static void t_merged_refs(void)\n \t\tif (err > 0)\n \t\t\tbreak;\n\n-\t\tREFTABLE_ALLOC_GROW(out, len + 1, cap);\n+\t\tcheck(!REFTABLE_ALLOC_GROW(out, len + 1, cap));\n \t\tout[len++] = ref;\n \t}\n \treftable_iterator_destroy(&it);\n@@ -459,7 +459,7 @@ static void t_merged_logs(void)\n \t\tif (err > 0)\n \t\t\tbreak;\n\n-\t\tREFTABLE_ALLOC_GROW(out, len + 1, cap);\n+\t\tcheck(!REFTABLE_ALLOC_GROW(out, len + 1, cap));\n \t\tout[len++] = log;\n \t}\n \treftable_iterator_destroy(&it);\n--\n2.47.1\n"},{"id":"509581","messageId":"xmqqttapvg42.fsf@gitster.g","threadId":"62694","inReplyTo":"9b2f4baa-b602-4cc5-8dfc-dd941b1d7af6@web.de","subject":"Re: [PATCH 1/4] reftable: avoid leaks on realloc error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-27T05:39:57Z","receivedAt":"2024-12-27T05:40:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> +#define REFTABLE_ALLOC_GROW_OR_NULL(x, nr, alloc) do { \\\n> +\tvoid *reftable_alloc_grow_or_null_orig_ptr = (x); \\\n> +\tREFTABLE_ALLOC_GROW((x), (nr), (alloc)); \\\n> +\tif (!(x)) { \\\n> +\t\treftable_free(reftable_alloc_grow_or_null_orig_ptr); \\\n> +\t\talloc = 0; \\\n> +\t} \\\n> +} while (0)\n\nIt is tricky what (x) means at each reference site ;-) but the above\nlooks good.\n\n>  #define REFTABLE_FREE_AND_NULL(p) do { reftable_free(p); (p) = NULL; } while (0)\n>\n>  #ifndef REFTABLE_ALLOW_BANNED_ALLOCATORS\n> diff --git a/reftable/block.c b/reftable/block.c\n> index 0198078485..9858bbc7c5 100644\n> --- a/reftable/block.c\n> +++ b/reftable/block.c\n> @@ -53,7 +53,8 @@ static int block_writer_register_restart(struct block_writer *w, int n,\n>  \tif (2 + 3 * rlen + n > w->block_size - w->next)\n>  \t\treturn -1;\n>  \tif (is_restart) {\n> -\t\tREFTABLE_ALLOC_GROW(w->restarts, w->restart_len + 1, w->restart_cap);\n> +\t\tREFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,\n> +\t\t\t\t\t    w->restart_cap);\n>  \t\tif (!w->restarts)\n>  \t\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n>  \t\tw->restarts[w->restart_len++] = w->next;\n> @@ -176,7 +177,8 @@ int block_writer_finish(struct block_writer *w)\n>  \t\t * is guaranteed to return `Z_STREAM_END`.\n>  \t\t */\n>  \t\tcompressed_len = deflateBound(w->zstream, src_len);\n> -\t\tREFTABLE_ALLOC_GROW(w->compressed, compressed_len, w->compressed_cap);\n> +\t\tREFTABLE_ALLOC_GROW_OR_NULL(w->compressed, compressed_len,\n> +\t\t\t\t\t    w->compressed_cap);\n>  \t\tif (!w->compressed) {\n>  \t\t\tret = REFTABLE_OUT_OF_MEMORY_ERROR;\n>  \t\t\treturn ret;\n> @@ -235,8 +237,8 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,\n>  \t\tuLong src_len = block->len - block_header_skip;\n>\n>  \t\t/* Log blocks specify the *uncompressed* size in their header. */\n> -\t\tREFTABLE_ALLOC_GROW(br->uncompressed_data, sz,\n> -\t\t\t\t    br->uncompressed_cap);\n> +\t\tREFTABLE_ALLOC_GROW_OR_NULL(br->uncompressed_data, sz,\n> +\t\t\t\t\t    br->uncompressed_cap);\n>  \t\tif (!br->uncompressed_data) {\n>  \t\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n>  \t\t\tgoto done;\n\nThese all have \"has the preceding realloc() return NULL?\" check and\nerror handling, so they are strict improvement whose only effect is\nto plug the leak (and clearing of the allocation).\n\n> diff --git a/reftable/pq.c b/reftable/pq.c\n> index 6ee1164dd3..5591e875e1 100644\n> --- a/reftable/pq.c\n> +++ b/reftable/pq.c\n> @@ -49,7 +49,7 @@ int merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry\n>  {\n>  \tsize_t i = 0;\n>\n> -\tREFTABLE_ALLOC_GROW(pq->heap, pq->len + 1, pq->cap);\n> +\tREFTABLE_ALLOC_GROW_OR_NULL(pq->heap, pq->len + 1, pq->cap);\n>  \tif (!pq->heap)\n>  \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n>  \tpq->heap[pq->len++] = *e;\n\nDitto.  And the same can be said to all hunks that follow (omitted).\n\nMakes one wonder what remaining callers of REFTABLE_ALLOC_GROW()\n(other than the one in REFTABLE_ALLOC_GROW_OR_NULL()) are getting;\nhopefully the next steps will deal with them?\n\n\n"},{"id":"509582","messageId":"xmqqplldvfsk.fsf@gitster.g","threadId":"62694","inReplyTo":"6084c017-9557-478b-b485-a1c1a21842e7@web.de","subject":"Re: [PATCH 4/4] t-reftable-merged: check realloc errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-27T05:46:51Z","receivedAt":"2024-12-27T05:46:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Report reallocation errors in unit tests, like everywhere else.\n\nOK.  That's good for consistency if anything else.\n\nWe have a test framework for doing unit test at such low level, yet\nwe cannot really write tests that validates that the right thing\nhappens when a particular realloc() call returns NULL, which feels\nsomewhat disappointing, but that is not a fault of this series.\n\nThanks.\n\n\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  t/unit-tests/t-reftable-merged.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\n> index a12bd0e1a3..60836f80d6 100644\n> --- a/t/unit-tests/t-reftable-merged.c\n> +++ b/t/unit-tests/t-reftable-merged.c\n> @@ -178,7 +178,7 @@ static void t_merged_refs(void)\n>  \t\tif (err > 0)\n>  \t\t\tbreak;\n>\n> -\t\tREFTABLE_ALLOC_GROW(out, len + 1, cap);\n> +\t\tcheck(!REFTABLE_ALLOC_GROW(out, len + 1, cap));\n>  \t\tout[len++] = ref;\n>  \t}\n>  \treftable_iterator_destroy(&it);\n> @@ -459,7 +459,7 @@ static void t_merged_logs(void)\n>  \t\tif (err > 0)\n>  \t\t\tbreak;\n>\n> -\t\tREFTABLE_ALLOC_GROW(out, len + 1, cap);\n> +\t\tcheck(!REFTABLE_ALLOC_GROW(out, len + 1, cap));\n>  \t\tout[len++] = log;\n>  \t}\n>  \treftable_iterator_destroy(&it);\n> --\n> 2.47.1\n"},{"id":"509587","messageId":"Z26CjQpewcyLwfs4@pks.im","threadId":"62694","inReplyTo":"9b2f4baa-b602-4cc5-8dfc-dd941b1d7af6@web.de","subject":"Re: [PATCH 1/4] reftable: avoid leaks on realloc error","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:33:55Z","receivedAt":"2024-12-27T10:34:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 25, 2024 at 07:38:29PM +0100, René Scharfe wrote:\n> When realloc(3) fails, it returns NULL and keeps the original allocation\n> intact.  REFTABLE_ALLOC_GROW overwrites both the original pointer and\n> the allocation count variable in that case, simultaneously leaking the\n> original allocation and misrepresenting the number of storable items.\n> \n> parse_names() and reftable_buf_add() avoid leaking by restoring the\n> original pointer value on failure, but all other callers seem to be OK\n> with losing the old allocation.  Add a new variant of the macro,\n> REFTABLE_ALLOC_GROW_OR_NULL, which plugs the leak and zeros the\n> allocation counter.  Use it for those callers.\n\nHm, okay. I find it a bit curious to discern those two macros from each\nother as all callers need to handle OOM errors anyway, so doing the safe\nthing should likely be our default here and all callsites that don't\nshould be adapted, shouldn't they?\n\nIn the case of `reftable_buf_add()` I kind of doubt the usefulness of\nhandling the error just to keep the old pointer intact, as all callsites\nwill ultimately error out anyway. But in the case of `parse_names()` we\ndo in fact want to handle the case specially so that we can free any\nnames we have already parsed, so that case makes sense indeed.\n\nSo there is merit in having two separate wrappers, but it would be nice\nif `REFTABLE_ALLOC_GROW()` would be doing the \"right thing\" for most\ncases while the above two callsites would be adapted to use a wrapper\nthat requires a bit more thought to use correctly. For example something\nlike `REFTABLE_TRY_ALLOC_GROW()` or similar.\n\nPatrick\n"},{"id":"509588","messageId":"Z26ClnvVH1cg3CGQ@pks.im","threadId":"62694","inReplyTo":"33bbacc7-1727-4efd-9cbf-3c9abfa94d8c@web.de","subject":"Re: [PATCH 2/4] reftable: fix allocation count on realloc error","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:33:58Z","receivedAt":"2024-12-27T10:34:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 25, 2024 at 07:38:35PM +0100, René Scharfe wrote:\n> When realloc(3) fails, it returns NULL and keeps the original allocation\n> intact.  REFTABLE_ALLOC_GROW overwrites both the original pointer and\n> the allocation count variable in that case, simultaneously leaking the\n> original allocation and misrepresenting the number of storable items.\n> \n> parse_names() avoids the leak by keeping the original pointer if\n> reallocation fails, but still increase the allocation count in such a\n> case as if it succeeded.  That's OK, because the error handling code\n> just frees everything and doesn't look at names_cap anymore.\n> \n> reftable_buf_add() does the same, but here it is a problem as it leaves\n> the reftable_buf in a broken state, with ->alloc being roughly twice as\n> big as the actually allocated memory, allowing out-of-bounds writes in\n> subsequent calls.\n> \n> Reimplement REFTABLE_ALLOC_GROW to avoid leaks, keep allocation counts\n> in sync and still signal failures to callers while avoiding code\n> duplication in callers.  Make it an expression that evaluates to 0 if no\n> reallocation is needed or it succeeded and 1 on failure while keeping\n> the original pointer and allocation counter values.\n> \n> Adjust REFTABLE_ALLOC_GROW_OR_NULL to the new calling convention for\n> REFTABLE_ALLOC_GROW, but keep its support for non-size_t alloc variables\n> for now.\n\nOkay, this change goes into the direction I was wondering about with the\npreceding commit, good.\n\n> diff --git a/reftable/basics.h b/reftable/basics.h\n> index 259f4c274c..fa5d75868b 100644\n> --- a/reftable/basics.h\n> +++ b/reftable/basics.h\n> @@ -120,22 +120,35 @@ char *reftable_strdup(const char *str);\n>  #define REFTABLE_ALLOC_ARRAY(x, alloc) (x) = reftable_malloc(st_mult(sizeof(*(x)), (alloc)))\n>  #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x)))\n>  #define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))\n> -#define REFTABLE_ALLOC_GROW(x, nr, alloc) \\\n> -\tdo { \\\n> -\t\tif ((nr) > alloc) { \\\n> -\t\t\talloc = 2 * (alloc) + 1; \\\n> -\t\t\tif (alloc < (nr)) \\\n> -\t\t\t\talloc = (nr); \\\n> -\t\t\tREFTABLE_REALLOC_ARRAY(x, alloc); \\\n> -\t\t} \\\n> -\t} while (0)\n> +\n> +static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,\n> +\t\t\t\t\tsize_t *allocp)\n> +{\n> +\tvoid *new_p;\n> +\tsize_t alloc = *allocp * 2 + 1;\n> +\tif (alloc < nelem)\n> +\t\talloc = nelem;\n> +\tnew_p = reftable_realloc(p, st_mult(elsize, alloc));\n> +\tif (!new_p)\n> +\t\treturn p;\n> +\t*allocp = alloc;\n> +\treturn new_p;\n> +}\n> +\n> +#define REFTABLE_ALLOC_GROW(x, nr, alloc) ( \\\n> +\t(nr) > (alloc) && ( \\\n> +\t\t(x) = reftable_alloc_grow((x), (nr), sizeof(*(x)), &(alloc)), \\\n> +\t\t(nr) > (alloc) \\\n> +\t) \\\n> +)\n\nDo we even need this macro? I don't really think it serves much of a\npurpose anymore.\n\nPatrick\n"},{"id":"509589","messageId":"Z26CmZEMQeqGFuEs@pks.im","threadId":"62694","inReplyTo":"xmqqplldvfsk.fsf@gitster.g","subject":"Re: [PATCH 4/4] t-reftable-merged: check realloc errors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:34:01Z","receivedAt":"2024-12-27T10:34:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 26, 2024 at 09:46:51PM -0800, Junio C Hamano wrote:\n> René Scharfe <l.s.r@web.de> writes:\n> \n> > Report reallocation errors in unit tests, like everywhere else.\n> \n> OK.  That's good for consistency if anything else.\n> \n> We have a test framework for doing unit test at such low level, yet\n> we cannot really write tests that validates that the right thing\n> happens when a particular realloc() call returns NULL, which feels\n> somewhat disappointing, but that is not a fault of this series.\n\nIn the context of the reftable library we can because we've got\npluggable allocators. We could in theory swap them out against variants\nthat fail, but it's not easy to make them fail in one specific code\npath. For the case at hand though it would work alright.\n\nPatrick\n"},{"id":"509590","messageId":"Z26CnRsw0yqQblMO@pks.im","threadId":"62694","inReplyTo":"2b9fba8d-be63-4145-9d25-a2151e422cfa@web.de","subject":"Re: [PATCH 0/4] reftable: fix realloc error handling","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:34:05Z","receivedAt":"2024-12-27T10:34:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 25, 2024 at 07:33:07PM +0100, René Scharfe wrote:\n> The current handling of reallocation errors leaks the original\n> allocation in most cases and corrupts the capacity variable.  Fix\n> that in REFTABLE_ALLOC_GROW and by providing a new macro\n> REFTABLE_ALLOC_GROW_OR_NULL -- solve this somewhat tricky issue\n> centrally, with minimal impact to calling code.\n> \n> And the last two patches add error handling to the remaining\n> places that still lack it.\n\nThanks a lot for working on this!\n\nPatrick\n"},{"id":"509640","messageId":"xmqqr05tt8qo.fsf@gitster.g","threadId":"62694","inReplyTo":"Z26CnRsw0yqQblMO@pks.im","subject":"Re: [PATCH 0/4] reftable: fix realloc error handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-27T16:02:07Z","receivedAt":"2024-12-27T16:02:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Wed, Dec 25, 2024 at 07:33:07PM +0100, René Scharfe wrote:\n>> The current handling of reallocation errors leaks the original\n>> allocation in most cases and corrupts the capacity variable.  Fix\n>> that in REFTABLE_ALLOC_GROW and by providing a new macro\n>> REFTABLE_ALLOC_GROW_OR_NULL -- solve this somewhat tricky issue\n>> centrally, with minimal impact to calling code.\n>> \n>> And the last two patches add error handling to the remaining\n>> places that still lack it.\n>\n> Thanks a lot for working on this!\n\nAnd thank you for reviewing.\n"},{"id":"509649","messageId":"f53ad79f-14a7-460b-9fbf-ba3d0e75ba35@web.de","threadId":"62694","inReplyTo":"33bbacc7-1727-4efd-9cbf-3c9abfa94d8c@web.de","subject":"Re: [PATCH 2/4] reftable: fix allocation count on realloc error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-27T20:16:10Z","receivedAt":"2024-12-27T20:16:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.12.24 um 19:38 schrieb René Scharfe:\n> When realloc(3) fails, it returns NULL and keeps the original allocation\n> intact.  REFTABLE_ALLOC_GROW overwrites both the original pointer and\n> the allocation count variable in that case, simultaneously leaking the\n> original allocation and misrepresenting the number of storable items.\n>\n> parse_names() avoids the leak by keeping the original pointer if\n> reallocation fails, but still increase the allocation count in such a\n> case as if it succeeded.  That's OK, because the error handling code\n> just frees everything and doesn't look at names_cap anymore.\n>\n> reftable_buf_add() does the same, but here it is a problem as it leaves\n> the reftable_buf in a broken state, with ->alloc being roughly twice as\n> big as the actually allocated memory, allowing out-of-bounds writes in\n> subsequent calls.\n>\n> Reimplement REFTABLE_ALLOC_GROW to avoid leaks, keep allocation counts\n> in sync and still signal failures to callers while avoiding code\n> duplication in callers.  Make it an expression that evaluates to 0 if no\n> reallocation is needed or it succeeded and 1 on failure while keeping\n> the original pointer and allocation counter values.\n>\n> Adjust REFTABLE_ALLOC_GROW_OR_NULL to the new calling convention for\n> REFTABLE_ALLOC_GROW, but keep its support for non-size_t alloc variables\n> for now.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  reftable/basics.c | 11 +++--------\n>  reftable/basics.h | 39 ++++++++++++++++++++++++++-------------\n>  2 files changed, 29 insertions(+), 21 deletions(-)\n>\n> diff --git a/reftable/basics.c b/reftable/basics.c\n> index 70b1091d14..cd6b39dbe9 100644\n> --- a/reftable/basics.c\n> +++ b/reftable/basics.c\n> @@ -124,11 +124,8 @@ int reftable_buf_add(struct reftable_buf *buf, const void *data, size_t len)\n>  \tsize_t newlen = buf->len + len;\n>\n>  \tif (newlen + 1 > buf->alloc) {\n> -\t\tchar *reallocated = buf->buf;\n> -\t\tREFTABLE_ALLOC_GROW(reallocated, newlen + 1, buf->alloc);\n> -\t\tif (!reallocated)\n> +\t\tif (REFTABLE_ALLOC_GROW(buf->buf, newlen + 1, buf->alloc))\n>  \t\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n> -\t\tbuf->buf = reallocated;\n>  \t}\n>\n>  \tmemcpy(buf->buf + buf->len, data, len);\n> @@ -233,11 +230,9 @@ char **parse_names(char *buf, int size)\n>  \t\t\tnext = end;\n>  \t\t}\n>  \t\tif (p < next) {\n> -\t\t\tchar **names_grown = names;\n> -\t\t\tREFTABLE_ALLOC_GROW(names_grown, names_len + 1, names_cap);\n> -\t\t\tif (!names_grown)\n> +\t\t\tif (REFTABLE_ALLOC_GROW(names, names_len + 1,\n> +\t\t\t\t\t\tnames_cap))\n>  \t\t\t\tgoto err;\n> -\t\t\tnames = names_grown;\n>\n>  \t\t\tnames[names_len] = reftable_strdup(p);\n>  \t\t\tif (!names[names_len++])\n> diff --git a/reftable/basics.h b/reftable/basics.h\n> index 259f4c274c..fa5d75868b 100644\n> --- a/reftable/basics.h\n> +++ b/reftable/basics.h\n> @@ -120,22 +120,35 @@ char *reftable_strdup(const char *str);\n>  #define REFTABLE_ALLOC_ARRAY(x, alloc) (x) = reftable_malloc(st_mult(sizeof(*(x)), (alloc)))\n>  #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x)))\n>  #define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))\n> -#define REFTABLE_ALLOC_GROW(x, nr, alloc) \\\n> -\tdo { \\\n> -\t\tif ((nr) > alloc) { \\\n> -\t\t\talloc = 2 * (alloc) + 1; \\\n> -\t\t\tif (alloc < (nr)) \\\n> -\t\t\t\talloc = (nr); \\\n> -\t\t\tREFTABLE_REALLOC_ARRAY(x, alloc); \\\n> -\t\t} \\\n> -\t} while (0)\n> +\n> +static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,\n> +\t\t\t\t\tsize_t *allocp)\n> +{\n> +\tvoid *new_p;\n> +\tsize_t alloc = *allocp * 2 + 1;\n> +\tif (alloc < nelem)\n> +\t\talloc = nelem;\n> +\tnew_p = reftable_realloc(p, st_mult(elsize, alloc));\n> +\tif (!new_p)\n> +\t\treturn p;\n> +\t*allocp = alloc;\n> +\treturn new_p;\n> +}\n> +\n> +#define REFTABLE_ALLOC_GROW(x, nr, alloc) ( \\\n> +\t(nr) > (alloc) && ( \\\n> +\t\t(x) = reftable_alloc_grow((x), (nr), sizeof(*(x)), &(alloc)), \\\n> +\t\t(nr) > (alloc) \\\n> +\t) \\\n> +)\n>\n>  #define REFTABLE_ALLOC_GROW_OR_NULL(x, nr, alloc) do { \\\n> -\tvoid *reftable_alloc_grow_or_null_orig_ptr = (x); \\\n> -\tREFTABLE_ALLOC_GROW((x), (nr), (alloc)); \\\n> -\tif (!(x)) { \\\n> -\t\treftable_free(reftable_alloc_grow_or_null_orig_ptr); \\\n> +\tsize_t reftable_alloc_grow_or_null_alloc = alloc; \\\n> +\tif (REFTABLE_ALLOC_GROW((x), (nr), reftable_alloc_grow_or_null_alloc)) { \\\n> +\t\treftable_free(x); \\\n>  \t\talloc = 0; \\\n\nThis forgets to set x = NULL, which is bad since that's what callers check. :-O\nIt was not necessary without this patch, because realloc(3) did it.\n\n> +\t} else { \\\n> +\t\talloc = reftable_alloc_grow_or_null_alloc; \\\n>  \t} \\\n>  } while (0)\n>\n> --\n> 2.47.1\n>\n\n"},{"id":"509650","messageId":"6112daa0-190e-4d15-9b00-98fbc2a69fa3@web.de","threadId":"62694","inReplyTo":"Z26CmZEMQeqGFuEs@pks.im","subject":"Re: [PATCH 4/4] t-reftable-merged: check realloc errors","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-27T20:16:16Z","receivedAt":"2024-12-27T20:16:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.12.24 um 11:34 schrieb Patrick Steinhardt:\n> On Thu, Dec 26, 2024 at 09:46:51PM -0800, Junio C Hamano wrote:\n>> René Scharfe <l.s.r@web.de> writes:\n>>\n>>> Report reallocation errors in unit tests, like everywhere else.\n>>\n>> OK.  That's good for consistency if anything else.\n>>\n>> We have a test framework for doing unit test at such low level, yet\n>> we cannot really write tests that validates that the right thing\n>> happens when a particular realloc() call returns NULL, which feels\n>> somewhat disappointing, but that is not a fault of this series.\n>\n> In the context of the reftable library we can because we've got\n> pluggable allocators. We could in theory swap them out against variants\n> that fail, but it's not easy to make them fail in one specific code\n> path. For the case at hand though it would work alright.\n\nIt should be easy and safe to provide allocator stubs that just fail\nand swap them in and out with reftable_set_alloc() before and after\nan operation that performs a single allocation.  Will add basic\ntests in the next round.\n\nInjecting an allocation error in the middle of parse_names() would\nrequire a version that starts failing after a certain number of\nallocations (or allocated bytes).  Possible, but not in scope for\nthis series.\n\nRené\n\n"},{"id":"509651","messageId":"68bd4e99-ed59-443a-a3c5-bb06653cfad5@web.de","threadId":"62694","inReplyTo":"Z26ClnvVH1cg3CGQ@pks.im","subject":"Re: [PATCH 2/4] reftable: fix allocation count on realloc error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-27T20:16:22Z","receivedAt":"2024-12-27T20:16:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.12.24 um 11:33 schrieb Patrick Steinhardt:\n> On Wed, Dec 25, 2024 at 07:38:35PM +0100, René Scharfe wrote:\n>\n>> diff --git a/reftable/basics.h b/reftable/basics.h\n>> index 259f4c274c..fa5d75868b 100644\n>> --- a/reftable/basics.h\n>> +++ b/reftable/basics.h\n>> @@ -120,22 +120,35 @@ char *reftable_strdup(const char *str);\n>>  #define REFTABLE_ALLOC_ARRAY(x, alloc) (x) = reftable_malloc(st_mult(sizeof(*(x)), (alloc)))\n>>  #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x)))\n>>  #define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))\n\n>> +#define REFTABLE_ALLOC_GROW(x, nr, alloc) ( \\\n>> +\t(nr) > (alloc) && ( \\\n>> +\t\t(x) = reftable_alloc_grow((x), (nr), sizeof(*(x)), &(alloc)), \\\n>> +\t\t(nr) > (alloc) \\\n>> +\t) \\\n>> +)\n>\n> Do we even need this macro? I don't really think it serves much of a\n> purpose anymore.\n\nIt provides the same value as the *ALLOC_ARRAY macros above: automatic\nsizeof handling.  Plus it returns whether the allocation succeeded,\navoiding repetition of its arguments in callers.\n\nRené\n\n"},{"id":"509652","messageId":"039f15a8-aef7-41a0-a396-c395a8416c3c@web.de","threadId":"62694","inReplyTo":"Z26CjQpewcyLwfs4@pks.im","subject":"Re: [PATCH 1/4] reftable: avoid leaks on realloc error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-27T20:16:27Z","receivedAt":"2024-12-27T20:16:37Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.12.24 um 11:33 schrieb Patrick Steinhardt:\n> On Wed, Dec 25, 2024 at 07:38:29PM +0100, René Scharfe wrote:\n>> When realloc(3) fails, it returns NULL and keeps the original allocation\n>> intact.  REFTABLE_ALLOC_GROW overwrites both the original pointer and\n>> the allocation count variable in that case, simultaneously leaking the\n>> original allocation and misrepresenting the number of storable items.\n>>\n>> parse_names() and reftable_buf_add() avoid leaking by restoring the\n>> original pointer value on failure, but all other callers seem to be OK\n>> with losing the old allocation.  Add a new variant of the macro,\n>> REFTABLE_ALLOC_GROW_OR_NULL, which plugs the leak and zeros the\n>> allocation counter.  Use it for those callers.\n>\n> Hm, okay. I find it a bit curious to discern those two macros from each\n> other as all callers need to handle OOM errors anyway, so doing the safe\n> thing should likely be our default here and all callsites that don't\n> should be adapted, shouldn't they?\n\nI agree, and I my first version only had REFTABLE_ALLOC_GROW.  Keeping\nstuff unchanged if we cannot grow should be safer, right?  But it would\nintroduce a leak if the caller exits without cleaning up, so each of\nthem needs to be audited.  I was too lazy for that.  And it's work that\ncan be parallelized..\n\n> In the case of `reftable_buf_add()` I kind of doubt the usefulness of\n> handling the error just to keep the old pointer intact, as all callsites\n> will ultimately error out anyway.\n\nI can imagine use cases where an object is built piece by piece, one\npart is too large and then you still want to keep all the rest and just\nreplace the huge thing with a placeholder or entirely ignore it.  Could\nbe a case of YAGNI, though.\n\n> But in the case of `parse_names()` we\n> do in fact want to handle the case specially so that we can free any\n> names we have already parsed, so that case makes sense indeed.\n\nYes.  But that leads me on a tangent: Is it really a good idea to load\na file into lots of individual string objects instead of loading into\na single big buffer and pointing directly into it?  Do those strings\nneed to have individual lifetimes?\n\n> So there is merit in having two separate wrappers, but it would be nice\n> if `REFTABLE_ALLOC_GROW()` would be doing the \"right thing\" for most\n> cases while the above two callsites would be adapted to use a wrapper\n> that requires a bit more thought to use correctly. For example something\n> like `REFTABLE_TRY_ALLOC_GROW()` or similar.\n\nSo this is about naming?  And with \"right thing\" you mean failing to\ngrow should lead to destruction?\n\nRené\n\n"},{"id":"509671","messageId":"f4677194-0a3a-4f07-b003-c0295b51c100@web.de","threadId":"62694","inReplyTo":"2b9fba8d-be63-4145-9d25-a2151e422cfa@web.de","subject":"[PATCH v2 0/4] reftable: fix realloc error handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-28T09:43:41Z","receivedAt":"2024-12-28T09:43:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Changes since v1:\n- added unit tests\n- explicitly set pointer to NULL on REFTABLE_ALLOC_GROW_OR_NULL failure\n  in patch 2; omission found by unit test\n\n  reftable: avoid leaks on realloc error\n  reftable: fix allocation count on realloc error\n  reftable: handle realloc error in parse_names()\n  t-reftable-merged: handle realloc errors\n\n reftable/basics.c                | 14 +++-----\n reftable/basics.h                | 41 ++++++++++++++++++-----\n reftable/block.c                 | 10 +++---\n reftable/pq.c                    |  2 +-\n reftable/record.c                | 12 +++----\n reftable/stack.c                 |  8 +++--\n reftable/writer.c                |  5 +--\n t/unit-tests/t-reftable-basics.c | 56 ++++++++++++++++++++++++++++++++\n t/unit-tests/t-reftable-merged.c |  4 +--\n 9 files changed, 116 insertions(+), 36 deletions(-)\n\nRange-diff against v1:\n1:  b41547720d ! 1:  b3cad92038 reftable: avoid leaks on realloc error\n    @@ reftable/writer.c: static int writer_flush_nonempty_block(struct reftable_writer\n      \tif (!w->index)\n      \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n\n    +\n    + ## t/unit-tests/t-reftable-basics.c ##\n    +@@ t/unit-tests/t-reftable-basics.c: static int integer_needle_lesseq(size_t i, void *_args)\n    + \treturn args->needle <= args->haystack[i];\n    + }\n    +\n    ++static void *realloc_stub(void *p UNUSED, size_t size UNUSED)\n    ++{\n    ++\treturn NULL;\n    ++}\n    ++\n    + int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n    + {\n    + \tif_test (\"binary search with binsearch works\") {\n    +@@ t/unit-tests/t-reftable-basics.c: int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n    + \t\tcheck_int(in, ==, out);\n    + \t}\n    +\n    ++\tif_test (\"REFTABLE_ALLOC_GROW_OR_NULL works\") {\n    ++\t\tint *arr = NULL;\n    ++\t\tsize_t alloc = 0, old_alloc;\n    ++\n    ++\t\tREFTABLE_ALLOC_GROW_OR_NULL(arr, 1, alloc);\n    ++\t\tcheck(arr != NULL);\n    ++\t\tcheck_uint(alloc, >=, 1);\n    ++\t\tarr[0] = 42;\n    ++\n    ++\t\told_alloc = alloc;\n    ++\t\tREFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);\n    ++\t\tcheck(arr != NULL);\n    ++\t\tcheck_uint(alloc, >, old_alloc);\n    ++\t\tarr[alloc - 1] = 42;\n    ++\n    ++\t\told_alloc = alloc;\n    ++\t\treftable_set_alloc(malloc, realloc_stub, free);\n    ++\t\tREFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);\n    ++\t\tcheck(arr == NULL);\n    ++\t\tcheck_uint(alloc, ==, 0);\n    ++\t\treftable_set_alloc(malloc, realloc, free);\n    ++\n    ++\t\treftable_free(arr);\n    ++\t}\n    ++\n    + \treturn test_done();\n    + }\n2:  bde2f0e4a5 ! 2:  62a1042825 reftable: fix allocation count on realloc error\n    @@ reftable/basics.h: char *reftable_strdup(const char *str);\n     -\t\treftable_free(reftable_alloc_grow_or_null_orig_ptr); \\\n     +\tsize_t reftable_alloc_grow_or_null_alloc = alloc; \\\n     +\tif (REFTABLE_ALLOC_GROW((x), (nr), reftable_alloc_grow_or_null_alloc)) { \\\n    -+\t\treftable_free(x); \\\n    ++\t\tREFTABLE_FREE_AND_NULL(x); \\\n      \t\talloc = 0; \\\n     +\t} else { \\\n     +\t\talloc = reftable_alloc_grow_or_null_alloc; \\\n      \t} \\\n      } while (0)\n\n    +\n    + ## t/unit-tests/t-reftable-basics.c ##\n    +@@ t/unit-tests/t-reftable-basics.c: int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n    + \t\tcheck_int(in, ==, out);\n    + \t}\n    +\n    ++\tif_test (\"REFTABLE_ALLOC_GROW works\") {\n    ++\t\tint *arr = NULL, *old_arr;\n    ++\t\tsize_t alloc = 0, old_alloc;\n    ++\n    ++\t\tcheck(!REFTABLE_ALLOC_GROW(arr, 1, alloc));\n    ++\t\tcheck(arr != NULL);\n    ++\t\tcheck_uint(alloc, >=, 1);\n    ++\t\tarr[0] = 42;\n    ++\n    ++\t\told_alloc = alloc;\n    ++\t\told_arr = arr;\n    ++\t\treftable_set_alloc(malloc, realloc_stub, free);\n    ++\t\tcheck(REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));\n    ++\t\tcheck(arr == old_arr);\n    ++\t\tcheck_uint(alloc, ==, old_alloc);\n    ++\n    ++\t\told_alloc = alloc;\n    ++\t\treftable_set_alloc(malloc, realloc, free);\n    ++\t\tcheck(!REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));\n    ++\t\tcheck(arr != NULL);\n    ++\t\tcheck_uint(alloc, >, old_alloc);\n    ++\t\tarr[alloc - 1] = 42;\n    ++\n    ++\t\treftable_free(arr);\n    ++\t}\n    ++\n    + \tif_test (\"REFTABLE_ALLOC_GROW_OR_NULL works\") {\n    + \t\tint *arr = NULL;\n    + \t\tsize_t alloc = 0, old_alloc;\n3:  7c9f044813 = 3:  ed1a292622 reftable: handle realloc error in parse_names()\n4:  3d9493f48a = 4:  916032657e t-reftable-merged: handle realloc errors\n--\n2.47.1\n\n"},{"id":"509672","messageId":"2f12efca-8b38-446a-a4a6-f80898275acc@web.de","threadId":"62694","inReplyTo":"f4677194-0a3a-4f07-b003-c0295b51c100@web.de","subject":"[PATCH v2 1/4] reftable: avoid leaks on realloc error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-28T09:47:05Z","receivedAt":"2024-12-28T09:47:10Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"When realloc(3) fails, it returns NULL and keeps the original allocation\nintact.  REFTABLE_ALLOC_GROW overwrites both the original pointer and\nthe allocation count variable in that case, simultaneously leaking the\noriginal allocation and misrepresenting the number of storable items.\n\nparse_names() and reftable_buf_add() avoid leaking by restoring the\noriginal pointer value on failure, but all other callers seem to be OK\nwith losing the old allocation.  Add a new variant of the macro,\nREFTABLE_ALLOC_GROW_OR_NULL, which plugs the leak and zeros the\nallocation counter.  Use it for those callers.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n reftable/basics.h                | 10 ++++++++++\n reftable/block.c                 | 10 ++++++----\n reftable/pq.c                    |  2 +-\n reftable/record.c                | 12 ++++++------\n reftable/stack.c                 |  8 +++++---\n reftable/writer.c                |  5 +++--\n t/unit-tests/t-reftable-basics.c | 30 ++++++++++++++++++++++++++++++\n 7 files changed, 61 insertions(+), 16 deletions(-)\n\ndiff --git a/reftable/basics.h b/reftable/basics.h\nindex 36beda2c25..259f4c274c 100644\n--- a/reftable/basics.h\n+++ b/reftable/basics.h\n@@ -129,6 +129,16 @@ char *reftable_strdup(const char *str);\n \t\t\tREFTABLE_REALLOC_ARRAY(x, alloc); \\\n \t\t} \\\n \t} while (0)\n+\n+#define REFTABLE_ALLOC_GROW_OR_NULL(x, nr, alloc) do { \\\n+\tvoid *reftable_alloc_grow_or_null_orig_ptr = (x); \\\n+\tREFTABLE_ALLOC_GROW((x), (nr), (alloc)); \\\n+\tif (!(x)) { \\\n+\t\treftable_free(reftable_alloc_grow_or_null_orig_ptr); \\\n+\t\talloc = 0; \\\n+\t} \\\n+} while (0)\n+\n #define REFTABLE_FREE_AND_NULL(p) do { reftable_free(p); (p) = NULL; } while (0)\n\n #ifndef REFTABLE_ALLOW_BANNED_ALLOCATORS\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 0198078485..9858bbc7c5 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -53,7 +53,8 @@ static int block_writer_register_restart(struct block_writer *w, int n,\n \tif (2 + 3 * rlen + n > w->block_size - w->next)\n \t\treturn -1;\n \tif (is_restart) {\n-\t\tREFTABLE_ALLOC_GROW(w->restarts, w->restart_len + 1, w->restart_cap);\n+\t\tREFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,\n+\t\t\t\t\t    w->restart_cap);\n \t\tif (!w->restarts)\n \t\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\tw->restarts[w->restart_len++] = w->next;\n@@ -176,7 +177,8 @@ int block_writer_finish(struct block_writer *w)\n \t\t * is guaranteed to return `Z_STREAM_END`.\n \t\t */\n \t\tcompressed_len = deflateBound(w->zstream, src_len);\n-\t\tREFTABLE_ALLOC_GROW(w->compressed, compressed_len, w->compressed_cap);\n+\t\tREFTABLE_ALLOC_GROW_OR_NULL(w->compressed, compressed_len,\n+\t\t\t\t\t    w->compressed_cap);\n \t\tif (!w->compressed) {\n \t\t\tret = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\t\treturn ret;\n@@ -235,8 +237,8 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,\n \t\tuLong src_len = block->len - block_header_skip;\n\n \t\t/* Log blocks specify the *uncompressed* size in their header. */\n-\t\tREFTABLE_ALLOC_GROW(br->uncompressed_data, sz,\n-\t\t\t\t    br->uncompressed_cap);\n+\t\tREFTABLE_ALLOC_GROW_OR_NULL(br->uncompressed_data, sz,\n+\t\t\t\t\t    br->uncompressed_cap);\n \t\tif (!br->uncompressed_data) {\n \t\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\t\tgoto done;\ndiff --git a/reftable/pq.c b/reftable/pq.c\nindex 6ee1164dd3..5591e875e1 100644\n--- a/reftable/pq.c\n+++ b/reftable/pq.c\n@@ -49,7 +49,7 @@ int merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry\n {\n \tsize_t i = 0;\n\n-\tREFTABLE_ALLOC_GROW(pq->heap, pq->len + 1, pq->cap);\n+\tREFTABLE_ALLOC_GROW_OR_NULL(pq->heap, pq->len + 1, pq->cap);\n \tif (!pq->heap)\n \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n \tpq->heap[pq->len++] = *e;\ndiff --git a/reftable/record.c b/reftable/record.c\nindex fb5652ed57..04429d23fe 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -246,8 +246,8 @@ static int reftable_ref_record_copy_from(void *rec, const void *src_rec,\n \tif (src->refname) {\n \t\tsize_t refname_len = strlen(src->refname);\n\n-\t\tREFTABLE_ALLOC_GROW(ref->refname, refname_len + 1,\n-\t\t\t\t    ref->refname_cap);\n+\t\tREFTABLE_ALLOC_GROW_OR_NULL(ref->refname, refname_len + 1,\n+\t\t\t\t\t    ref->refname_cap);\n \t\tif (!ref->refname) {\n \t\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\t\tgoto out;\n@@ -385,7 +385,7 @@ static int reftable_ref_record_decode(void *rec, struct reftable_buf key,\n \tSWAP(r->refname, refname);\n \tSWAP(r->refname_cap, refname_cap);\n\n-\tREFTABLE_ALLOC_GROW(r->refname, key.len + 1, r->refname_cap);\n+\tREFTABLE_ALLOC_GROW_OR_NULL(r->refname, key.len + 1, r->refname_cap);\n \tif (!r->refname) {\n \t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\tgoto done;\n@@ -839,7 +839,7 @@ static int reftable_log_record_decode(void *rec, struct reftable_buf key,\n \tif (key.len <= 9 || key.buf[key.len - 9] != 0)\n \t\treturn REFTABLE_FORMAT_ERROR;\n\n-\tREFTABLE_ALLOC_GROW(r->refname, key.len - 8, r->refname_cap);\n+\tREFTABLE_ALLOC_GROW_OR_NULL(r->refname, key.len - 8, r->refname_cap);\n \tif (!r->refname) {\n \t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\tgoto done;\n@@ -947,8 +947,8 @@ static int reftable_log_record_decode(void *rec, struct reftable_buf key,\n \t}\n \tstring_view_consume(&in, n);\n\n-\tREFTABLE_ALLOC_GROW(r->value.update.message, scratch->len + 1,\n-\t\t\t    r->value.update.message_cap);\n+\tREFTABLE_ALLOC_GROW_OR_NULL(r->value.update.message, scratch->len + 1,\n+\t\t\t\t    r->value.update.message_cap);\n \tif (!r->value.update.message) {\n \t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\tgoto done;\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 634f0c5425..531660a49f 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -317,7 +317,9 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\t\t\t * thus need to keep them alive here, which we\n \t\t\t\t * do by bumping their refcount.\n \t\t\t\t */\n-\t\t\t\tREFTABLE_ALLOC_GROW(reused, reused_len + 1, reused_alloc);\n+\t\t\t\tREFTABLE_ALLOC_GROW_OR_NULL(reused,\n+\t\t\t\t\t\t\t    reused_len + 1,\n+\t\t\t\t\t\t\t    reused_alloc);\n \t\t\t\tif (!reused) {\n \t\t\t\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\t\t\t\tgoto done;\n@@ -949,8 +951,8 @@ int reftable_addition_add(struct reftable_addition *add,\n \tif (err < 0)\n \t\tgoto done;\n\n-\tREFTABLE_ALLOC_GROW(add->new_tables, add->new_tables_len + 1,\n-\t\t\t    add->new_tables_cap);\n+\tREFTABLE_ALLOC_GROW_OR_NULL(add->new_tables, add->new_tables_len + 1,\n+\t\t\t\t    add->new_tables_cap);\n \tif (!add->new_tables) {\n \t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n \t\tgoto done;\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 624e90fb53..740c98038e 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -254,7 +254,8 @@ static int writer_index_hash(struct reftable_writer *w, struct reftable_buf *has\n \tif (key->offset_len > 0 && key->offsets[key->offset_len - 1] == off)\n \t\treturn 0;\n\n-\tREFTABLE_ALLOC_GROW(key->offsets, key->offset_len + 1, key->offset_cap);\n+\tREFTABLE_ALLOC_GROW_OR_NULL(key->offsets, key->offset_len + 1,\n+\t\t\t\t    key->offset_cap);\n \tif (!key->offsets)\n \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n \tkey->offsets[key->offset_len++] = off;\n@@ -820,7 +821,7 @@ static int writer_flush_nonempty_block(struct reftable_writer *w)\n \t * Note that this also applies when flushing index blocks, in which\n \t * case we will end up with a multi-level index.\n \t */\n-\tREFTABLE_ALLOC_GROW(w->index, w->index_len + 1, w->index_cap);\n+\tREFTABLE_ALLOC_GROW_OR_NULL(w->index, w->index_len + 1, w->index_cap);\n \tif (!w->index)\n \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n\ndiff --git a/t/unit-tests/t-reftable-basics.c b/t/unit-tests/t-reftable-basics.c\nindex 65d50df091..5bf79c9976 100644\n--- a/t/unit-tests/t-reftable-basics.c\n+++ b/t/unit-tests/t-reftable-basics.c\n@@ -20,6 +20,11 @@ static int integer_needle_lesseq(size_t i, void *_args)\n \treturn args->needle <= args->haystack[i];\n }\n\n+static void *realloc_stub(void *p UNUSED, size_t size UNUSED)\n+{\n+\treturn NULL;\n+}\n+\n int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n {\n \tif_test (\"binary search with binsearch works\") {\n@@ -141,5 +146,30 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \t\tcheck_int(in, ==, out);\n \t}\n\n+\tif_test (\"REFTABLE_ALLOC_GROW_OR_NULL works\") {\n+\t\tint *arr = NULL;\n+\t\tsize_t alloc = 0, old_alloc;\n+\n+\t\tREFTABLE_ALLOC_GROW_OR_NULL(arr, 1, alloc);\n+\t\tcheck(arr != NULL);\n+\t\tcheck_uint(alloc, >=, 1);\n+\t\tarr[0] = 42;\n+\n+\t\told_alloc = alloc;\n+\t\tREFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);\n+\t\tcheck(arr != NULL);\n+\t\tcheck_uint(alloc, >, old_alloc);\n+\t\tarr[alloc - 1] = 42;\n+\n+\t\told_alloc = alloc;\n+\t\treftable_set_alloc(malloc, realloc_stub, free);\n+\t\tREFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);\n+\t\tcheck(arr == NULL);\n+\t\tcheck_uint(alloc, ==, 0);\n+\t\treftable_set_alloc(malloc, realloc, free);\n+\n+\t\treftable_free(arr);\n+\t}\n+\n \treturn test_done();\n }\n--\n2.47.1\n"},{"id":"509673","messageId":"8bc6592f-f589-4c72-84d7-bd16ea9d753c@web.de","threadId":"62694","inReplyTo":"f4677194-0a3a-4f07-b003-c0295b51c100@web.de","subject":"[PATCH v2 2/4] reftable: fix allocation count on realloc error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-28T09:48:00Z","receivedAt":"2024-12-28T09:48:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"When realloc(3) fails, it returns NULL and keeps the original allocation\nintact.  REFTABLE_ALLOC_GROW overwrites both the original pointer and\nthe allocation count variable in that case, simultaneously leaking the\noriginal allocation and misrepresenting the number of storable items.\n\nparse_names() avoids the leak by keeping the original pointer if\nreallocation fails, but still increase the allocation count in such a\ncase as if it succeeded.  That's OK, because the error handling code\njust frees everything and doesn't look at names_cap anymore.\n\nreftable_buf_add() does the same, but here it is a problem as it leaves\nthe reftable_buf in a broken state, with ->alloc being roughly twice as\nbig as the actually allocated memory, allowing out-of-bounds writes in\nsubsequent calls.\n\nReimplement REFTABLE_ALLOC_GROW to avoid leaks, keep allocation counts\nin sync and still signal failures to callers while avoiding code\nduplication in callers.  Make it an expression that evaluates to 0 if no\nreallocation is needed or it succeeded and 1 on failure while keeping\nthe original pointer and allocation counter values.\n\nAdjust REFTABLE_ALLOC_GROW_OR_NULL to the new calling convention for\nREFTABLE_ALLOC_GROW, but keep its support for non-size_t alloc variables\nfor now.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n reftable/basics.c                | 11 +++------\n reftable/basics.h                | 39 +++++++++++++++++++++-----------\n t/unit-tests/t-reftable-basics.c | 26 +++++++++++++++++++++\n 3 files changed, 55 insertions(+), 21 deletions(-)\n\ndiff --git a/reftable/basics.c b/reftable/basics.c\nindex 70b1091d14..cd6b39dbe9 100644\n--- a/reftable/basics.c\n+++ b/reftable/basics.c\n@@ -124,11 +124,8 @@ int reftable_buf_add(struct reftable_buf *buf, const void *data, size_t len)\n \tsize_t newlen = buf->len + len;\n\n \tif (newlen + 1 > buf->alloc) {\n-\t\tchar *reallocated = buf->buf;\n-\t\tREFTABLE_ALLOC_GROW(reallocated, newlen + 1, buf->alloc);\n-\t\tif (!reallocated)\n+\t\tif (REFTABLE_ALLOC_GROW(buf->buf, newlen + 1, buf->alloc))\n \t\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tbuf->buf = reallocated;\n \t}\n\n \tmemcpy(buf->buf + buf->len, data, len);\n@@ -233,11 +230,9 @@ char **parse_names(char *buf, int size)\n \t\t\tnext = end;\n \t\t}\n \t\tif (p < next) {\n-\t\t\tchar **names_grown = names;\n-\t\t\tREFTABLE_ALLOC_GROW(names_grown, names_len + 1, names_cap);\n-\t\t\tif (!names_grown)\n+\t\t\tif (REFTABLE_ALLOC_GROW(names, names_len + 1,\n+\t\t\t\t\t\tnames_cap))\n \t\t\t\tgoto err;\n-\t\t\tnames = names_grown;\n\n \t\t\tnames[names_len] = reftable_strdup(p);\n \t\t\tif (!names[names_len++])\ndiff --git a/reftable/basics.h b/reftable/basics.h\nindex 259f4c274c..4bf71b0954 100644\n--- a/reftable/basics.h\n+++ b/reftable/basics.h\n@@ -120,22 +120,35 @@ char *reftable_strdup(const char *str);\n #define REFTABLE_ALLOC_ARRAY(x, alloc) (x) = reftable_malloc(st_mult(sizeof(*(x)), (alloc)))\n #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x)))\n #define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))\n-#define REFTABLE_ALLOC_GROW(x, nr, alloc) \\\n-\tdo { \\\n-\t\tif ((nr) > alloc) { \\\n-\t\t\talloc = 2 * (alloc) + 1; \\\n-\t\t\tif (alloc < (nr)) \\\n-\t\t\t\talloc = (nr); \\\n-\t\t\tREFTABLE_REALLOC_ARRAY(x, alloc); \\\n-\t\t} \\\n-\t} while (0)\n+\n+static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,\n+\t\t\t\t\tsize_t *allocp)\n+{\n+\tvoid *new_p;\n+\tsize_t alloc = *allocp * 2 + 1;\n+\tif (alloc < nelem)\n+\t\talloc = nelem;\n+\tnew_p = reftable_realloc(p, st_mult(elsize, alloc));\n+\tif (!new_p)\n+\t\treturn p;\n+\t*allocp = alloc;\n+\treturn new_p;\n+}\n+\n+#define REFTABLE_ALLOC_GROW(x, nr, alloc) ( \\\n+\t(nr) > (alloc) && ( \\\n+\t\t(x) = reftable_alloc_grow((x), (nr), sizeof(*(x)), &(alloc)), \\\n+\t\t(nr) > (alloc) \\\n+\t) \\\n+)\n\n #define REFTABLE_ALLOC_GROW_OR_NULL(x, nr, alloc) do { \\\n-\tvoid *reftable_alloc_grow_or_null_orig_ptr = (x); \\\n-\tREFTABLE_ALLOC_GROW((x), (nr), (alloc)); \\\n-\tif (!(x)) { \\\n-\t\treftable_free(reftable_alloc_grow_or_null_orig_ptr); \\\n+\tsize_t reftable_alloc_grow_or_null_alloc = alloc; \\\n+\tif (REFTABLE_ALLOC_GROW((x), (nr), reftable_alloc_grow_or_null_alloc)) { \\\n+\t\tREFTABLE_FREE_AND_NULL(x); \\\n \t\talloc = 0; \\\n+\t} else { \\\n+\t\talloc = reftable_alloc_grow_or_null_alloc; \\\n \t} \\\n } while (0)\n\ndiff --git a/t/unit-tests/t-reftable-basics.c b/t/unit-tests/t-reftable-basics.c\nindex 5bf79c9976..990dc1a244 100644\n--- a/t/unit-tests/t-reftable-basics.c\n+++ b/t/unit-tests/t-reftable-basics.c\n@@ -146,6 +146,32 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \t\tcheck_int(in, ==, out);\n \t}\n\n+\tif_test (\"REFTABLE_ALLOC_GROW works\") {\n+\t\tint *arr = NULL, *old_arr;\n+\t\tsize_t alloc = 0, old_alloc;\n+\n+\t\tcheck(!REFTABLE_ALLOC_GROW(arr, 1, alloc));\n+\t\tcheck(arr != NULL);\n+\t\tcheck_uint(alloc, >=, 1);\n+\t\tarr[0] = 42;\n+\n+\t\told_alloc = alloc;\n+\t\told_arr = arr;\n+\t\treftable_set_alloc(malloc, realloc_stub, free);\n+\t\tcheck(REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));\n+\t\tcheck(arr == old_arr);\n+\t\tcheck_uint(alloc, ==, old_alloc);\n+\n+\t\told_alloc = alloc;\n+\t\treftable_set_alloc(malloc, realloc, free);\n+\t\tcheck(!REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));\n+\t\tcheck(arr != NULL);\n+\t\tcheck_uint(alloc, >, old_alloc);\n+\t\tarr[alloc - 1] = 42;\n+\n+\t\treftable_free(arr);\n+\t}\n+\n \tif_test (\"REFTABLE_ALLOC_GROW_OR_NULL works\") {\n \t\tint *arr = NULL;\n \t\tsize_t alloc = 0, old_alloc;\n--\n2.47.1\n"},{"id":"509674","messageId":"c2cd277b-ba9d-41cb-b9bd-c519b445d179@web.de","threadId":"62694","inReplyTo":"f4677194-0a3a-4f07-b003-c0295b51c100@web.de","subject":"[PATCH v2 3/4] reftable: handle realloc error in parse_names()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-28T09:48:50Z","receivedAt":"2024-12-28T09:48:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Check the final reallocation for adding the terminating NULL and handle\nit just like those in the loop.  Simply use REFTABLE_ALLOC_GROW instead\nof keeping the REFTABLE_REALLOC_ARRAY call and adding code to preserve\nthe original pointer value around it.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n reftable/basics.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/reftable/basics.c b/reftable/basics.c\nindex cd6b39dbe9..fe2b83ff83 100644\n--- a/reftable/basics.c\n+++ b/reftable/basics.c\n@@ -241,7 +241,8 @@ char **parse_names(char *buf, int size)\n \t\tp = next + 1;\n \t}\n\n-\tREFTABLE_REALLOC_ARRAY(names, names_len + 1);\n+\tif (REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap))\n+\t\tgoto err;\n \tnames[names_len] = NULL;\n\n \treturn names;\n--\n2.47.1\n"},{"id":"509675","messageId":"e6274b50-e826-4150-9a9f-4f7a2214a134@web.de","threadId":"62694","inReplyTo":"f4677194-0a3a-4f07-b003-c0295b51c100@web.de","subject":"[PATCH v2 4/4] t-reftable-merged: handle realloc errors","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-28T09:49:38Z","receivedAt":"2024-12-28T09:49:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Check reallocation errors in unit tests, like everywhere else.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/unit-tests/t-reftable-merged.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c\nindex a12bd0e1a3..60836f80d6 100644\n--- a/t/unit-tests/t-reftable-merged.c\n+++ b/t/unit-tests/t-reftable-merged.c\n@@ -178,7 +178,7 @@ static void t_merged_refs(void)\n \t\tif (err > 0)\n \t\t\tbreak;\n\n-\t\tREFTABLE_ALLOC_GROW(out, len + 1, cap);\n+\t\tcheck(!REFTABLE_ALLOC_GROW(out, len + 1, cap));\n \t\tout[len++] = ref;\n \t}\n \treftable_iterator_destroy(&it);\n@@ -459,7 +459,7 @@ static void t_merged_logs(void)\n \t\tif (err > 0)\n \t\t\tbreak;\n\n-\t\tREFTABLE_ALLOC_GROW(out, len + 1, cap);\n+\t\tcheck(!REFTABLE_ALLOC_GROW(out, len + 1, cap));\n \t\tout[len++] = log;\n \t}\n \treftable_iterator_destroy(&it);\n--\n2.47.1\n"},{"id":"509722","messageId":"Z3I9s3ur5NDtW4oD@pks.im","threadId":"62694","inReplyTo":"039f15a8-aef7-41a0-a396-c395a8416c3c@web.de","subject":"Re: [PATCH 1/4] reftable: avoid leaks on realloc error","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T06:29:07Z","receivedAt":"2024-12-30T06:29:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 27, 2024 at 09:16:27PM +0100, René Scharfe wrote:\n> Am 27.12.24 um 11:33 schrieb Patrick Steinhardt:\n> > On Wed, Dec 25, 2024 at 07:38:29PM +0100, René Scharfe wrote:\n> >> When realloc(3) fails, it returns NULL and keeps the original allocation\n> >> intact.  REFTABLE_ALLOC_GROW overwrites both the original pointer and\n> >> the allocation count variable in that case, simultaneously leaking the\n> >> original allocation and misrepresenting the number of storable items.\n> >>\n> >> parse_names() and reftable_buf_add() avoid leaking by restoring the\n> >> original pointer value on failure, but all other callers seem to be OK\n> >> with losing the old allocation.  Add a new variant of the macro,\n> >> REFTABLE_ALLOC_GROW_OR_NULL, which plugs the leak and zeros the\n> >> allocation counter.  Use it for those callers.\n> >\n> > Hm, okay. I find it a bit curious to discern those two macros from each\n> > other as all callers need to handle OOM errors anyway, so doing the safe\n> > thing should likely be our default here and all callsites that don't\n> > should be adapted, shouldn't they?\n> \n> I agree, and I my first version only had REFTABLE_ALLOC_GROW.  Keeping\n> stuff unchanged if we cannot grow should be safer, right?  But it would\n> introduce a leak if the caller exits without cleaning up, so each of\n> them needs to be audited.  I was too lazy for that.  And it's work that\n> can be parallelized..\n\nFair enough.\n\n> > In the case of `reftable_buf_add()` I kind of doubt the usefulness of\n> > handling the error just to keep the old pointer intact, as all callsites\n> > will ultimately error out anyway.\n> \n> I can imagine use cases where an object is built piece by piece, one\n> part is too large and then you still want to keep all the rest and just\n> replace the huge thing with a placeholder or entirely ignore it.  Could\n> be a case of YAGNI, though.\n\nProbably.\n\n> > But in the case of `parse_names()` we\n> > do in fact want to handle the case specially so that we can free any\n> > names we have already parsed, so that case makes sense indeed.\n> \n> Yes.  But that leads me on a tangent: Is it really a good idea to load\n> a file into lots of individual string objects instead of loading into\n> a single big buffer and pointing directly into it?  Do those strings\n> need to have individual lifetimes?\n\nGood question indeed. I don't think we ever need individual lifetimes\nhere. On the other hand it's probably okayish, too, given that the\nnumber of table names should be limited due to automatic compaction.\n\n> > So there is merit in having two separate wrappers, but it would be nice\n> > if `REFTABLE_ALLOC_GROW()` would be doing the \"right thing\" for most\n> > cases while the above two callsites would be adapted to use a wrapper\n> > that requires a bit more thought to use correctly. For example something\n> > like `REFTABLE_TRY_ALLOC_GROW()` or similar.\n> \n> So this is about naming?  And with \"right thing\" you mean failing to\n> grow should lead to destruction?\n\nYup, exactly. I just want to give callers a better indicator which of\nthese functions does what, and having sane defaults helps in my opinion.\n\nI guess this also depends on whether or not we want to eventually adapt\nall callsites to handle allocation errors themselves.\n\nPatrick\n"},{"id":"509725","messageId":"Z3JK0sWU-vITVl1R@pks.im","threadId":"62694","inReplyTo":"2f12efca-8b38-446a-a4a6-f80898275acc@web.de","subject":"Re: [PATCH v2 1/4] reftable: avoid leaks on realloc error","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T07:25:06Z","receivedAt":"2024-12-30T07:25:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Dec 28, 2024 at 10:47:05AM +0100, René Scharfe wrote:\n> @@ -141,5 +146,30 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n>  \t\tcheck_int(in, ==, out);\n>  \t}\n> \n> +\tif_test (\"REFTABLE_ALLOC_GROW_OR_NULL works\") {\n> +\t\tint *arr = NULL;\n> +\t\tsize_t alloc = 0, old_alloc;\n> +\n> +\t\tREFTABLE_ALLOC_GROW_OR_NULL(arr, 1, alloc);\n> +\t\tcheck(arr != NULL);\n> +\t\tcheck_uint(alloc, >=, 1);\n> +\t\tarr[0] = 42;\n> +\n> +\t\told_alloc = alloc;\n> +\t\tREFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);\n> +\t\tcheck(arr != NULL);\n> +\t\tcheck_uint(alloc, >, old_alloc);\n> +\t\tarr[alloc - 1] = 42;\n> +\n> +\t\told_alloc = alloc;\n> +\t\treftable_set_alloc(malloc, realloc_stub, free);\n> +\t\tREFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);\n> +\t\tcheck(arr == NULL);\n> +\t\tcheck_uint(alloc, ==, 0);\n> +\t\treftable_set_alloc(malloc, realloc, free);\n> +\n> +\t\treftable_free(arr);\n> +\t}\n> +\n\nThanks for adding this test!\n\nPatrick\n"},{"id":"509726","messageId":"Z3JK1rBxcuMoiUmK@pks.im","threadId":"62694","inReplyTo":"c2cd277b-ba9d-41cb-b9bd-c519b445d179@web.de","subject":"Re: [PATCH v2 3/4] reftable: handle realloc error in parse_names()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T07:25:10Z","receivedAt":"2024-12-30T07:25:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Dec 28, 2024 at 10:48:50AM +0100, René Scharfe wrote:\n> Check the final reallocation for adding the terminating NULL and handle\n> it just like those in the loop.  Simply use REFTABLE_ALLOC_GROW instead\n> of keeping the REFTABLE_REALLOC_ARRAY call and adding code to preserve\n> the original pointer value around it.\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  reftable/basics.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/reftable/basics.c b/reftable/basics.c\n> index cd6b39dbe9..fe2b83ff83 100644\n> --- a/reftable/basics.c\n> +++ b/reftable/basics.c\n> @@ -241,7 +241,8 @@ char **parse_names(char *buf, int size)\n>  \t\tp = next + 1;\n>  \t}\n> \n> -\tREFTABLE_REALLOC_ARRAY(names, names_len + 1);\n> +\tif (REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap))\n> +\t\tgoto err;\n>  \tnames[names_len] = NULL;\n\nOkay. We may be overallocating in some cases now, but it's most likely\nthat we can completely skip this allocation here because `names_cap` is\nbig enough already.\n\nPatrick\n"},{"id":"509727","messageId":"Z3JK4lW4REYqBK9I@pks.im","threadId":"62694","inReplyTo":"f4677194-0a3a-4f07-b003-c0295b51c100@web.de","subject":"Re: [PATCH v2 0/4] reftable: fix realloc error handling","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-30T07:25:22Z","receivedAt":"2024-12-30T07:25:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Dec 28, 2024 at 10:43:41AM +0100, René Scharfe wrote:\n> Changes since v1:\n> - added unit tests\n> - explicitly set pointer to NULL on REFTABLE_ALLOC_GROW_OR_NULL failure\n>   in patch 2; omission found by unit test\n> \n>   reftable: avoid leaks on realloc error\n>   reftable: fix allocation count on realloc error\n>   reftable: handle realloc error in parse_names()\n>   t-reftable-merged: handle realloc errors\n\nI think this version is good enough for now. I'm not particularly happy\nabout the split we have with the reftable reallocators now, but don't\nthink that the series really is to blame for that as it simply fixes a\npreexisting issue. We can iterate on this in the future.\n\nThanks!\n\nPatrick\n"}]}