{"thread":{"id":"62680","subject":"[PATCH 1/4] reftable/stack: don't perform auto-compaction with less than two tables","startedAt":"2024-12-21T11:50:41Z","lastAt":"2024-12-23T04:08:33Z","messageCount":30,"participants":["Patrick Steinhardt","Kristoffer Haugsbakk","Randall Becker","Junio C Hamano","rsbecker@nexbridge.com"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"509466","messageId":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-0-12db83a3267c@pks.im","threadId":"62680","inReplyTo":null,"subject":"[PATCH 0/4] reftable: fix out-of-memory errors on NonStop","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-21T11:50:06Z","receivedAt":"2024-12-21T11:50:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series fixes out-of-memory errors on NonStop with the\nreftable backend. These errors are caused by zero-sized allocations,\nwhich return `NULL` pointers on NonStop.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (4):\n      reftable/stack: don't perform auto-compaction with less than two tables\n      reftable/merged: fix zero-sized allocation when there are no readers\n      reftable/stack: fix zero-sized allocation when there are no readers\n      reftable/basics: return NULL on zero-sized allocations\n\n reftable/basics.c |  7 +++++++\n reftable/merged.c | 12 +++++++-----\n reftable/stack.c  | 47 ++++++++++++++++++++++++++---------------------\n 3 files changed, 40 insertions(+), 26 deletions(-)\n\n\n---\nbase-commit: ff795a5c5ed2e2d07c688c217a615d89e3f5733b\nchange-id: 20241220-b4-pks-reftable-oom-fix-without-readers-c7d8fda0694d\n\n"},{"id":"509465","messageId":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-1-12db83a3267c@pks.im","threadId":"62680","inReplyTo":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-0-12db83a3267c@pks.im","subject":"[PATCH 1/4] reftable/stack: don't perform auto-compaction with less than two tables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-21T11:50:07Z","receivedAt":"2024-12-21T11:50:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to compact tables we need at least two tables. Bail out early\nfrom `reftable_stack_auto_compact()` in case we have less than two\ntables.\n\nThis is mostly defense in depth: `stack_table_sizes_for_compaction()`\nmay try to allocate a zero-byte object when there aren't any readers,\nand that may lead to a `NULL` pointer on some platforms like NonStop\nwhich causes us to bail out with an out-of-memory error.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 59fd695a12c2033ed589a21ef1c9155eeecc4641..6ca21965d8e1135d986043113d465abd14cd532c 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -1627,6 +1627,9 @@ int reftable_stack_auto_compact(struct reftable_stack *st)\n \tstruct segment seg;\n \tuint64_t *sizes;\n \n+\tif (st->merged->readers_len < 2)\n+\t\treturn 0;\n+\n \tsizes = stack_table_sizes_for_compaction(st);\n \tif (!sizes)\n \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509467","messageId":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-2-12db83a3267c@pks.im","threadId":"62680","inReplyTo":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-0-12db83a3267c@pks.im","subject":"[PATCH 2/4] reftable/merged: fix zero-sized allocation when there are no readers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-21T11:50:08Z","receivedAt":"2024-12-21T11:50:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It was reported [1c that Git started to fail with an out-of-memory error\nwhen initializing repositories with the reftable backend on NonStop\nplatforms. A bisect led to 802c0646ac (reftable/merged: handle\nallocation failures in `merged_table_init_iter()`, 2024-10-02), which\nchanged how we allocate memory when initializing a merged table.\n\nThe root cause of this seems to be that NonStop returns a `NULL` pointer\nwhen doing a zero-sized allocation. This would've already happened\nbefore the above change, but we never noticed because we did not check\nthe result. Now that we do we notice and thus return an out-of-memory\nerror to the caller.\n\nFix the issue by skipping the allocation altogether in case there are no\nreaders.\n\n[1]: <00ad01db5017$aa9ce340$ffd6a9c0$@nexbridge.com>\n\nReported-by: Randall S. Becker <rsbecker@nexbridge.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/merged.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/reftable/merged.c b/reftable/merged.c\nindex bb0836e3443271f9c0d5ba5582c78694d437ddc2..e72b39e178d4dec6ddfca970b5af71b71431397a 100644\n--- a/reftable/merged.c\n+++ b/reftable/merged.c\n@@ -240,14 +240,16 @@ int merged_table_init_iter(struct reftable_merged_table *mt,\n \t\t\t   struct reftable_iterator *it,\n \t\t\t   uint8_t typ)\n {\n-\tstruct merged_subiter *subiters;\n+\tstruct merged_subiter *subiters = NULL;\n \tstruct merged_iter *mi = NULL;\n \tint ret;\n \n-\tREFTABLE_CALLOC_ARRAY(subiters, mt->readers_len);\n-\tif (!subiters) {\n-\t\tret = REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tgoto out;\n+\tif (mt->readers_len) {\n+\t\tREFTABLE_CALLOC_ARRAY(subiters, mt->readers_len);\n+\t\tif (!subiters) {\n+\t\t\tret = REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t\t\tgoto out;\n+\t\t}\n \t}\n \n \tfor (size_t i = 0; i < mt->readers_len; i++) {\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509468","messageId":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-3-12db83a3267c@pks.im","threadId":"62680","inReplyTo":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-0-12db83a3267c@pks.im","subject":"[PATCH 3/4] reftable/stack: fix zero-sized allocation when there are no readers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-21T11:50:09Z","receivedAt":"2024-12-21T11:50:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Similar as the preceding commit, we may try to do a zero-sized\nallocation when reloading a reftable stack that ain't got any tables.\nIt is implementation-defined whether malloc(3p) returns a NULL pointer\nin that case or a zero-sized object. In case it does return a NULL\npointer though it causes us to think we have run into an out-of-memory\nsituation, and thus we return an error.\n\nFix this by only allocating arrays when they have at least one entry.\nRefactor the code so that we don't try to access those arrays in case\nthey are empty.\n\nReported-by: Randall S. Becker <rsbecker@nexbridge.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 44 +++++++++++++++++++++++---------------------\n 1 file changed, 23 insertions(+), 21 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 6ca21965d8e1135d986043113d465abd14cd532c..8328bfc58e9c207983ce88355d6110c40be3fac1 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -270,40 +270,42 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\t\t\t      int reuse_open)\n {\n \tsize_t cur_len = !st->merged ? 0 : st->merged->readers_len;\n-\tstruct reftable_reader **cur;\n+\tstruct reftable_reader **cur = NULL;\n \tstruct reftable_reader **reused = NULL;\n-\tstruct reftable_reader **new_readers;\n+\tstruct reftable_reader **new_readers = NULL;\n \tsize_t reused_len = 0, reused_alloc = 0, names_len;\n \tsize_t new_readers_len = 0;\n \tstruct reftable_merged_table *new_merged = NULL;\n \tstruct reftable_buf table_path = REFTABLE_BUF_INIT;\n \tint err = 0;\n-\tsize_t i;\n \n-\tcur = stack_copy_readers(st, cur_len);\n-\tif (!cur) {\n-\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tgoto done;\n+\tif (cur_len) {\n+\t\tcur = stack_copy_readers(st, cur_len);\n+\t\tif (!cur) {\n+\t\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t\t\tgoto done;\n+\t\t}\n \t}\n \n \tnames_len = names_length(names);\n-\n-\tnew_readers = reftable_calloc(names_len, sizeof(*new_readers));\n-\tif (!new_readers) {\n-\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tgoto done;\n+\tif (names_len) {\n+\t\tnew_readers = reftable_calloc(names_len, sizeof(*new_readers));\n+\t\tif (!new_readers) {\n+\t\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t\t\tgoto done;\n+\t\t}\n \t}\n \n-\twhile (*names) {\n+\tfor (size_t i = 0; i < names_len; i++) {\n \t\tstruct reftable_reader *rd = NULL;\n-\t\tconst char *name = *names++;\n+\t\tconst char *name = names[i];\n \n \t\t/* this is linear; we assume compaction keeps the number of\n \t\t   tables under control so this is not quadratic. */\n-\t\tfor (i = 0; reuse_open && i < cur_len; i++) {\n-\t\t\tif (cur[i] && 0 == strcmp(cur[i]->name, name)) {\n-\t\t\t\trd = cur[i];\n-\t\t\t\tcur[i] = NULL;\n+\t\tfor (size_t j = 0; reuse_open && j < cur_len; j++) {\n+\t\t\tif (cur[j] && 0 == strcmp(cur[j]->name, name)) {\n+\t\t\t\trd = cur[j];\n+\t\t\t\tcur[j] = NULL;\n \n \t\t\t\t/*\n \t\t\t\t * When reloading the stack fails, we end up\n@@ -357,7 +359,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t * file of such an open reader wouldn't have been possible to be\n \t * unlinked by the compacting process.\n \t */\n-\tfor (i = 0; i < cur_len; i++) {\n+\tfor (size_t i = 0; i < cur_len; i++) {\n \t\tif (cur[i]) {\n \t\t\tconst char *name = reader_name(cur[i]);\n \n@@ -388,11 +390,11 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t * happen on the successful case, because on the unsuccessful one we\n \t * decrement their refcount via `new_readers`.\n \t */\n-\tfor (i = 0; i < reused_len; i++)\n+\tfor (size_t i = 0; i < reused_len; i++)\n \t\treftable_reader_decref(reused[i]);\n \n done:\n-\tfor (i = 0; i < new_readers_len; i++)\n+\tfor (size_t i = 0; i < new_readers_len; i++)\n \t\treftable_reader_decref(new_readers[i]);\n \treftable_free(new_readers);\n \treftable_free(reused);\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509469","messageId":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-4-12db83a3267c@pks.im","threadId":"62680","inReplyTo":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-0-12db83a3267c@pks.im","subject":"[PATCH 4/4] reftable/basics: return NULL on zero-sized allocations","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-21T11:50:10Z","receivedAt":"2024-12-21T11:50:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the preceding commits we have fixed a couple of issues when\nallocating zero-sized objects. These issues were masked by\nimplementation-defined behaviour. Quoting malloc(3p):\n\n  If size is 0, either:\n\n    * A null pointer shall be returned and errno may be set to an\n      implementation-defined value, or\n\n    * A pointer to the allocated space shall be returned. The\n      application shall ensure that the pointer is not used to access an\n      object.\n\nSo it is perfectly valid that implementations of this function may or\nmay not return a NULL pointer in such a case.\n\nAdapt both `reftable_malloc()` and `reftable_realloc()` so that they\nreturn NULL pointers on zero-sized allocations. This should remove any\nimplementation-defined behaviour in our allocators and thus allows us to\ndetect such platform-specific issues more easily going forward.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/basics.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/reftable/basics.c b/reftable/basics.c\nindex 7d84a5d62dead1cf1a60698b1bb12fe6ac41c090..70b1091d1495bb5b4c8aae63bd9213dc704aecde 100644\n--- a/reftable/basics.c\n+++ b/reftable/basics.c\n@@ -17,6 +17,8 @@ static void (*reftable_free_ptr)(void *);\n \n void *reftable_malloc(size_t sz)\n {\n+\tif (!sz)\n+\t\treturn NULL;\n \tif (reftable_malloc_ptr)\n \t\treturn (*reftable_malloc_ptr)(sz);\n \treturn malloc(sz);\n@@ -24,6 +26,11 @@ void *reftable_malloc(size_t sz)\n \n void *reftable_realloc(void *p, size_t sz)\n {\n+\tif (!sz) {\n+\t\treftable_free(p);\n+\t\treturn NULL;\n+\t}\n+\n \tif (reftable_realloc_ptr)\n \t\treturn (*reftable_realloc_ptr)(p, sz);\n \treturn realloc(p, sz);\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509471","messageId":"b4ba502a-b689-4736-9712-443a137b5d2c@app.fastmail.com","threadId":"62680","inReplyTo":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-2-12db83a3267c@pks.im","subject":"Re: [PATCH 2/4] reftable/merged: fix zero-sized allocation when there are no readers","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-12-21T12:40:32Z","receivedAt":"2024-12-21T12:41:33Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"Hi\n\nOn Sat, Dec 21, 2024, at 12:50, Patrick Steinhardt wrote:\n> It was reported [1c\n\nA Neo user, I see.\n\ns/c/]/\n\n> [snip]\n> the result. Now that we do we notice and thus return an out-of-memory\n> error to the caller.\n\ns/Now that we do we/Now we do notice/\n\nRepeat of “we”.  And “that” can’t be be used since it has nothing to\nconnect to.\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"509472","messageId":"1b833ccf-080d-4a92-afe2-3f469bc23dde@app.fastmail.com","threadId":"62680","inReplyTo":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-4-12db83a3267c@pks.im","subject":"Re: [PATCH 4/4] reftable/basics: return NULL on zero-sized allocations","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-12-21T12:53:29Z","receivedAt":"2024-12-21T12:53:51Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Sat, Dec 21, 2024, at 12:50, Patrick Steinhardt wrote:\n> In the preceding commits we have fixed a couple of issues when\n> allocating zero-sized objects. These issues were masked by\n> implementation-defined behaviour. Quoting malloc(3p):\n>\n>   If size is 0, either:\n>\n>     * A null pointer shall be returned and errno may be set to an\n>       implementation-defined value, or\n>\n>     * A pointer to the allocated space shall be returned. The\n>       application shall ensure that the pointer is not used to access an\n>       object.\n>\n> So it is perfectly valid that implementations of this function may or\n> may not return a NULL pointer in such a case.\n>\n> Adapt both `reftable_malloc()` and `reftable_realloc()` so that they\n> return NULL pointers on zero-sized allocations. This should remove any\n> implementation-defined behaviour in our allocators and thus allows us to\n> detect such platform-specific issues more easily going forward.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n\nNice commit message.\n"},{"id":"509474","messageId":"Z2bOOn4Oad9b9Ycr@pks.im","threadId":"62680","inReplyTo":"b4ba502a-b689-4736-9712-443a137b5d2c@app.fastmail.com","subject":"Re: [PATCH 2/4] reftable/merged: fix zero-sized allocation when there are no readers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-21T14:18:34Z","receivedAt":"2024-12-21T14:18:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Dec 21, 2024 at 01:40:32PM +0100, Kristoffer Haugsbakk wrote:\n> Hi\n> \n> On Sat, Dec 21, 2024, at 12:50, Patrick Steinhardt wrote:\n> > It was reported [1c\n> \n> A Neo user, I see.\n\nThere are dozens of us!\n\n> s/c/]/\n\nOh, I even saw that typo, but obviously forgot to fix it.\n\n> > [snip]\n> > the result. Now that we do we notice and thus return an out-of-memory\n> > error to the caller.\n> \n> s/Now that we do we/Now we do notice/\n> \n> Repeat of “we”.  And “that” can’t be be used since it has nothing to\n> connect to.\n\nIndeed. Will wait a bit to hear back from Randall before sending another\nversion. Thanks!\n\nPatrick\n"},{"id":"509475","messageId":"BL3PR17MB6020D6CB8712F2C6F36FC8CFF4002@BL3PR17MB6020.namprd17.prod.outlook.com","threadId":"62680","inReplyTo":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-0-12db83a3267c@pks.im","subject":"RE: [PATCH 0/4] reftable: fix out-of-memory errors on NonStop","fromName":"Randall Becker","fromEmail":"randall.becker@nexbridge.ca","sentAt":"2024-12-21T15:05:44Z","receivedAt":"2024-12-21T15:05:57Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On December 21, 2024 6:50 AM, Patrick Steinhardt wrote:\n>this small patch series fixes out-of-memory errors on NonStop with the reftable\n>backend. These errors are caused by zero-sized allocations, which return `NULL`\n>pointers on NonStop.\n>\n>Thanks!\n>\n>Patrick\n>\n>---\n>Patrick Steinhardt (4):\n>      reftable/stack: don't perform auto-compaction with less than two tables\n>      reftable/merged: fix zero-sized allocation when there are no readers\n>      reftable/stack: fix zero-sized allocation when there are no readers\n>      reftable/basics: return NULL on zero-sized allocations\n>\n> reftable/basics.c |  7 +++++++\n> reftable/merged.c | 12 +++++++-----\n> reftable/stack.c  | 47 ++++++++++++++++++++++++++---------------------\n> 3 files changed, 40 insertions(+), 26 deletions(-)\n>\n>\n>---\n>base-commit: ff795a5c5ed2e2d07c688c217a615d89e3f5733b\n>change-id: 20241220-b4-pks-reftable-oom-fix-without-readers-c7d8fda0694d\n\nFrom the malloc man page:\n\"If size is 0, then malloc() returns either NULL, or a unique pointer value that can later be successfully passed to free().\"\n\nThank you for this series. I think the problem may not be limited only to NonStop based on the documented ambiguous behaviour of malloc.\n\n--Randall\n"},{"id":"509477","messageId":"xmqqv7vdc726.fsf@gitster.g","threadId":"62680","inReplyTo":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-0-12db83a3267c@pks.im","subject":"Re: [PATCH 0/4] reftable: fix out-of-memory errors on NonStop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-21T16:52:17Z","receivedAt":"2024-12-21T16:52:20Z","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> this small patch series fixes out-of-memory errors on NonStop with the\n> reftable backend. These errors are caused by zero-sized allocations,\n> which return `NULL` pointers on NonStop.\n\nInteresting.  Git side has long been aware of this issue, but\nbecause being able to get something out of zero-byte allocation was\nsomehow deemed so useful (and I do not either remember the reasons,\nnor if I were asked today, I do not know if I agree), our xmalloc()\nand friends ensure that we never return NULL for 0-sized allocation.\n\nI looked at the patches the places they touch and they are sensible\nchanges to avoid asking 0-sized allocations.  Which is the opposite\napproach from what Git takes *and* which may be more sensible.  The\ncallers should know better what they want to do when they have zero\nsized request.  E.g., if they have 0 records to write to in a format\nthat has num-records followed by an array of entries, they may want\nto make it a no-op, or they may want to explicitly write 0 followed\nby no entries, to record the fact that such a request was made.\n\nOne possible downside of the approach is that all API functions must\nbe vetted for their response to a request that might end up leading\nto a zero-sized allocation and told to special case such a request,\nbut that is something better donw sooner rather than later.\n\nWill queue.  Thanks.\n\n\n\n"},{"id":"509479","messageId":"xmqqmsgpc6bf.fsf@gitster.g","threadId":"62680","inReplyTo":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-1-12db83a3267c@pks.im","subject":"Re: [PATCH 1/4] reftable/stack: don't perform auto-compaction with less than two tables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-21T17:08:20Z","receivedAt":"2024-12-21T17:08:22Z","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> In order to compact tables we need at least two tables. Bail out early\n> from `reftable_stack_auto_compact()` in case we have less than two\n> tables.\n\nWhile that is very true, bailing out on \"< 2\" would change the\nbehaviour.  Where there were only one table, earlier we still went\nahead and exercised compaction code path, but now we no longer do.\nThe end result of having just a single table might logically be the\nsame with this change, but if we were relying on some side effects\nof exercising the compaction code path, do we know that the rest of\nthe code is OK?\n\nThat's the kind of questions I would ask, if this were somebody who\nhasn't been deeply involved in the reftable code and came this deep\nin the pre-release period.  But since we all know you have been the\nmain driver for this effort, we'd take your word for it ;-)\n\nThanks, will queue.\n\n\n\n> This is mostly defense in depth: `stack_table_sizes_for_compaction()`\n> may try to allocate a zero-byte object when there aren't any readers,\n> and that may lead to a `NULL` pointer on some platforms like NonStop\n> which causes us to bail out with an out-of-memory error.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  reftable/stack.c | 3 +++\n>  1 file changed, 3 insertions(+)\n>\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index 59fd695a12c2033ed589a21ef1c9155eeecc4641..6ca21965d8e1135d986043113d465abd14cd532c 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -1627,6 +1627,9 @@ int reftable_stack_auto_compact(struct reftable_stack *st)\n>  \tstruct segment seg;\n>  \tuint64_t *sizes;\n>  \n> +\tif (st->merged->readers_len < 2)\n> +\t\treturn 0;\n> +\n>  \tsizes = stack_table_sizes_for_compaction(st);\n>  \tif (!sizes)\n>  \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n"},{"id":"509480","messageId":"xmqqikrdc63c.fsf@gitster.g","threadId":"62680","inReplyTo":"b4ba502a-b689-4736-9712-443a137b5d2c@app.fastmail.com","subject":"Re: [PATCH 2/4] reftable/merged: fix zero-sized allocation when there are no readers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-21T17:13:11Z","receivedAt":"2024-12-21T17:13:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" <kristofferhaugsbakk@fastmail.com> writes:\n\n> Hi\n>\n> On Sat, Dec 21, 2024, at 12:50, Patrick Steinhardt wrote:\n>> It was reported [1c\n>\n> A Neo user, I see.\n>\n> s/c/]/\n>\n>> [snip]\n>> the result. Now that we do we notice and thus return an out-of-memory\n>> error to the caller.\n>\n> s/Now that we do we/Now we do notice/\n>\n> Repeat of “we”.  And “that” can’t be be used since it has nothing to\n> connect to.\n\nI read \"Now that we do, we notice and ...\" a perfectly sensible\nconstruct, though.\n\nThanks.\n\n"},{"id":"509481","messageId":"xmqq34ihc4zt.fsf@gitster.g","threadId":"62680","inReplyTo":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-3-12db83a3267c@pks.im","subject":"Re: [PATCH 3/4] reftable/stack: fix zero-sized allocation when there are no readers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-21T17:36:54Z","receivedAt":"2024-12-21T17:36:57Z","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> Similar as the preceding commit, we may try to do a zero-sized\n> allocation when reloading a reftable stack that ain't got any tables.\n> It is implementation-defined whether malloc(3p) returns a NULL pointer\n> in that case or a zero-sized object. In case it does return a NULL\n> pointer though it causes us to think we have run into an out-of-memory\n> situation, and thus we return an error.\n>\n> Fix this by only allocating arrays when they have at least one entry.\n> Refactor the code so that we don't try to access those arrays in case\n> they are empty.\n>\n> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  reftable/stack.c | 44 +++++++++++++++++++++++---------------------\n>  1 file changed, 23 insertions(+), 21 deletions(-)\n\nThis somehow did not cleanly apply, so I whiggled it in manually.\n\nI probably wouldn't mixed the \"size_t i\" changes into this fix if I\nwere doing it.  To avoid \"while (*names)\" loop, I would have made it\nto \"for (size_t j = 0; j < names_len; j++)\" and kept the existing\nuse of \"i\" intact, instead.  And reintroducing for() scoped \"i\"\nthree times did not seem to make it easier to read the result.\n\nI am not convinced we need to avoid \"while (*names)\", by the way.\nIf \"names\" were NULL, names_length() would already have segfaulted\nanyway, and basics.c:parse_names(), when not returning NULL (which\nwould have been caught by the sole caller of reload_once() as an\nerror), makes sure it gives its caller a NULL-terminated array.\n\nBut other than that, this seems to make sure that we avoid\nunnecessary work when cur_len or new_readers is zero and avoids\nasking for 0-sized allocation as a side effect of doing so, which is\ngood.\n\nWill queue.  Thanks.\n"},{"id":"509482","messageId":"xmqqy109aq4a.fsf@gitster.g","threadId":"62680","inReplyTo":"BL3PR17MB6020D6CB8712F2C6F36FC8CFF4002@BL3PR17MB6020.namprd17.prod.outlook.com","subject":"Re: [PATCH 0/4] reftable: fix out-of-memory errors on NonStop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-21T17:43:33Z","receivedAt":"2024-12-21T17:43:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Randall Becker <randall.becker@nexbridge.ca> writes:\n\n> Thank you for this series. I think the problem may not be limited\n> only to NonStop based on the documented ambiguous behaviour of\n> malloc.\n\nYes, an implementation is allowed to return NULL when asked to\nallocate 0 bytes.  On the Git side, we sidestep the problem in\nexactly the opposite way---our malloc() wrapper makes another\nrequest to allocate 1-byte block after seeing NULL from a request\nfor 0-byte allocation.  The reftable code takes an approach to\neliminate the need to requiest 0-sized allocation in the first\nplace, which is an arguably more valid solution.\n\nThanks, both, for finding and fixing the issue.\n"},{"id":"509483","messageId":"xmqqseqgc4bd.fsf@gitster.g","threadId":"62680","inReplyTo":"xmqqmsgpc6bf.fsf@gitster.g","subject":"Re: [PATCH 1/4] reftable/stack: don't perform auto-compaction with less than two tables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-21T17:51:34Z","receivedAt":"2024-12-21T17:51:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n>> In order to compact tables we need at least two tables. Bail out early\n>> from `reftable_stack_auto_compact()` in case we have less than two\n>> tables.\n>\n> While that is very true, bailing out on \"< 2\" would change the\n> behaviour.  Where there were only one table, earlier we still went\n> ahead and exercised compaction code path, but now we no longer do.\n> The end result of having just a single table might logically be the\n> same with this change, but if we were relying on some side effects\n> of exercising the compaction code path, do we know that the rest of\n> the code is OK?\n>\n> That's the kind of questions I would ask, if this were somebody who\n> hasn't been deeply involved in the reftable code and came this deep\n> in the pre-release period.  But since we all know you have been the\n> main driver for this effort, we'd take your word for it ;-)\n>\n> Thanks, will queue.\n\nAnd with code inspection, it can trivially seen that this change is\nperfectly fine.\n\nIn the original, when \"== 1\", stack_table_sizes_for_compaction(st)\nyields an array with a single element, suggest_compaction_segment()\ngives back segment with .start and .end both set to 0, for which\nsegment_size() returns 0 hence we do not call stack_compact_range().\nThe original code happens to do the same when \"== 0\" (and 0-sized\nallocation does not give back NULL).\n\nSo bypassing all of these when \"== 1\" is a no-op change, an\noptimization to avoid allocating a single-element array and then\nimmediately freeing it.  Doing so when \"== 0\" is a strict\nimprovement on a platform with malloc that returns NULL for 0-sized\nallocation.  So bypassing when \"< 2\" is totally safe and justifyable\nchange.\n\nThanks.\n"},{"id":"509484","messageId":"xmqqmsgoc42a.fsf@gitster.g","threadId":"62680","inReplyTo":"xmqq34ihc4zt.fsf@gitster.g","subject":"Re: [PATCH 3/4] reftable/stack: fix zero-sized allocation when there are no readers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-21T17:57:01Z","receivedAt":"2024-12-21T17:57:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>>  reftable/stack.c | 44 +++++++++++++++++++++++---------------------\n>>  1 file changed, 23 insertions(+), 21 deletions(-)\n>\n> This somehow did not cleanly apply, so I whiggled it in manually.\n\nThat is because I usually ignore author-supplied base, and this time\nI used the tip of ps/reftable-alloc-failures topic, which brings all\nthe text these four patches touch, in my initial attempt.\n\nApplying these on the author-supplied base (ff795a5c5e) yields the\nsame tree as the result of merging my manual application of these\nfour patches to ps/reftable-alloc-failures into the same base.\n"},{"id":"509485","messageId":"01b201db53d3$0b5953d0$220bfb70$@nexbridge.com","threadId":"62680","inReplyTo":"xmqqmsgoc42a.fsf@gitster.g","subject":"RE: [PATCH 3/4] reftable/stack: fix zero-sized allocation when there are no readers","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-12-21T18:06:17Z","receivedAt":"2024-12-21T18:06:28Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On December 21, 2024 12:57 PM, Junio C Hamano wrote:\n>Junio C Hamano <gitster@pobox.com> writes:\n>\n>>>  reftable/stack.c | 44 +++++++++++++++++++++++---------------------\n>>>  1 file changed, 23 insertions(+), 21 deletions(-)\n>>\n>> This somehow did not cleanly apply, so I whiggled it in manually.\n>\n>That is because I usually ignore author-supplied base, and this time I used\nthe tip of\n>ps/reftable-alloc-failures topic, which brings all the text these four\npatches touch, in\n>my initial attempt.\n>\n>Applying these on the author-supplied base (ff795a5c5e) yields the same\ntree as\n>the result of merging my manual application of these four patches to\nps/reftable-\n>alloc-failures into the same base.\n\nReady to test this. Please let me know when and I will report results.\n\n"},{"id":"509486","messageId":"xmqqikrcc2i6.fsf@gitster.g","threadId":"62680","inReplyTo":"01b201db53d3$0b5953d0$220bfb70$@nexbridge.com","subject":"Re: [PATCH 3/4] reftable/stack: fix zero-sized allocation when there are no readers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-21T18:30:41Z","receivedAt":"2024-12-21T18:30:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"<rsbecker@nexbridge.com> writes:\n\n>>Applying these on the author-supplied base (ff795a5c5e) yields the same\n> tree as\n>>the result of merging my manual application of these four patches to\n> ps/reftable-\n>>alloc-failures into the same base.\n>\n> Ready to test this. Please let me know when and I will report results.\n\nIf you want to start sooner\n\n    $ git checkout -b test ff795a5c5ed2e2d07c688c217a615d89e3f5733b\n    $ git am ... these four patches ...\n\nshould give you the fix without anything else mixed in.  I'll push\nout the usual four branches after integration testing, but it will\nbe queued in 'seen' (just above the point that corresponds to\n'next') first, before merging it to 'next' (and then down to\n'master' before -rc1).\n\nThanks.\n"},{"id":"509490","messageId":"Z2e8FUdLPhJuzrEo@pks.im","threadId":"62680","inReplyTo":"xmqqseqgc4bd.fsf@gitster.g","subject":"Re: [PATCH 1/4] reftable/stack: don't perform auto-compaction with less than two tables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-22T07:13:25Z","receivedAt":"2024-12-22T07:13:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Dec 21, 2024 at 09:51:34AM -0800, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Patrick Steinhardt <ps@pks.im> writes:\n> >\n> >> In order to compact tables we need at least two tables. Bail out early\n> >> from `reftable_stack_auto_compact()` in case we have less than two\n> >> tables.\n> >\n> > While that is very true, bailing out on \"< 2\" would change the\n> > behaviour.  Where there were only one table, earlier we still went\n> > ahead and exercised compaction code path, but now we no longer do.\n> > The end result of having just a single table might logically be the\n> > same with this change, but if we were relying on some side effects\n> > of exercising the compaction code path, do we know that the rest of\n> > the code is OK?\n> >\n> > That's the kind of questions I would ask, if this were somebody who\n> > hasn't been deeply involved in the reftable code and came this deep\n> > in the pre-release period.  But since we all know you have been the\n> > main driver for this effort, we'd take your word for it ;-)\n> >\n> > Thanks, will queue.\n> \n> And with code inspection, it can trivially seen that this change is\n> perfectly fine.\n> \n> In the original, when \"== 1\", stack_table_sizes_for_compaction(st)\n> yields an array with a single element, suggest_compaction_segment()\n> gives back segment with .start and .end both set to 0, for which\n> segment_size() returns 0 hence we do not call stack_compact_range().\n> The original code happens to do the same when \"== 0\" (and 0-sized\n> allocation does not give back NULL).\n> \n> So bypassing all of these when \"== 1\" is a no-op change, an\n> optimization to avoid allocating a single-element array and then\n> immediately freeing it.  Doing so when \"== 0\" is a strict\n> improvement on a platform with malloc that returns NULL for 0-sized\n> allocation.  So bypassing when \"< 2\" is totally safe and justifyable\n> change.\n\nIndeed. I'll include an explanation in v2 of this patch series.\n\nPatrick\n"},{"id":"509491","messageId":"Z2e8I6eGcTXL7uec@pks.im","threadId":"62680","inReplyTo":"xmqq34ihc4zt.fsf@gitster.g","subject":"Re: [PATCH 3/4] reftable/stack: fix zero-sized allocation when there are no readers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-22T07:13:39Z","receivedAt":"2024-12-22T07:13:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Dec 21, 2024 at 09:36:54AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > Similar as the preceding commit, we may try to do a zero-sized\n> > allocation when reloading a reftable stack that ain't got any tables.\n> > It is implementation-defined whether malloc(3p) returns a NULL pointer\n> > in that case or a zero-sized object. In case it does return a NULL\n> > pointer though it causes us to think we have run into an out-of-memory\n> > situation, and thus we return an error.\n> >\n> > Fix this by only allocating arrays when they have at least one entry.\n> > Refactor the code so that we don't try to access those arrays in case\n> > they are empty.\n> >\n> > Reported-by: Randall S. Becker <rsbecker@nexbridge.com>\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  reftable/stack.c | 44 +++++++++++++++++++++++---------------------\n> >  1 file changed, 23 insertions(+), 21 deletions(-)\n> \n> This somehow did not cleanly apply, so I whiggled it in manually.\n> \n> I probably wouldn't mixed the \"size_t i\" changes into this fix if I\n> were doing it.  To avoid \"while (*names)\" loop, I would have made it\n> to \"for (size_t j = 0; j < names_len; j++)\" and kept the existing\n> use of \"i\" intact, instead.  And reintroducing for() scoped \"i\"\n> three times did not seem to make it easier to read the result.\n> \n> I am not convinced we need to avoid \"while (*names)\", by the way.\n> If \"names\" were NULL, names_length() would already have segfaulted\n> anyway, and basics.c:parse_names(), when not returning NULL (which\n> would have been caught by the sole caller of reload_once() as an\n> error), makes sure it gives its caller a NULL-terminated array.\n\nTrue. I was initially erring on the side of being overly cautious here\nbecause I didn't yet have the idea to adapt `reftable_malloc()` and\n`reftable_realloc()` so that they have the same behaviour as NonStop.\nLet me redo this change and reduce it to the minimum required one.\n\nPatrick\n"},{"id":"509493","messageId":"20241222-b4-pks-reftable-oom-fix-without-readers-v2-0-19550090d15a@pks.im","threadId":"62680","inReplyTo":"20241221-b4-pks-reftable-oom-fix-without-readers-v1-0-12db83a3267c@pks.im","subject":"[PATCH v2 0/4] reftable: fix out-of-memory errors on NonStop","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-22T07:24:27Z","receivedAt":"2024-12-22T07:24:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series fixes out-of-memory errors on NonStop with the\nreftable backend. These errors are caused by zero-sized allocations,\nwhich return `NULL` pointers on NonStop.\n\nChanges in v2:\n\n  - Some small touchups to commit messages.\n  - Explain why it is safe to stop auto-compacting with less than two\n    tables.\n  - Adapt `reftable_stack_reload_once()` so that we only do the minimum\n    changes required to fix issue.\n  - Link to v1: https://lore.kernel.org/r/20241221-b4-pks-reftable-oom-fix-without-readers-v1-0-12db83a3267c@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (4):\n      reftable/stack: don't perform auto-compaction with less than two tables\n      reftable/merged: fix zero-sized allocation when there are no readers\n      reftable/stack: fix zero-sized allocation when there are no readers\n      reftable/basics: return NULL on zero-sized allocations\n\n reftable/basics.c |  7 +++++++\n reftable/merged.c | 12 +++++++-----\n reftable/stack.c  | 27 +++++++++++++++++----------\n 3 files changed, 31 insertions(+), 15 deletions(-)\n\nRange-diff versus v1:\n\n1:  9fcd452995 ! 1:  a7c5b7c52e reftable/stack: don't perform auto-compaction with less than two tables\n    @@ Commit message\n         from `reftable_stack_auto_compact()` in case we have less than two\n         tables.\n     \n    -    This is mostly defense in depth: `stack_table_sizes_for_compaction()`\n    -    may try to allocate a zero-byte object when there aren't any readers,\n    -    and that may lead to a `NULL` pointer on some platforms like NonStop\n    -    which causes us to bail out with an out-of-memory error.\n    +    In the original, `stack_table_sizes_for_compaction()` yields an array\n    +    that has the same length as the number of tables. This array is then\n    +    passed on to `suggest_compaction_segment()`, which returns an empty\n    +    segment in case we have less than two tables. The segment is then passed\n    +    to `segment_size()`, which will return `0` because both start and end of\n    +    the segment are `0`. And because we only call `stack_compact_range()` in\n    +    case we have a positive segment size we don't perform auto-compaction at\n    +    all. Consequently, this change does not result in a user-visible change\n    +    in behaviour when called with a single table.\n    +\n    +    But when called with no tables this protects us against a potential\n    +    out-of-memory error: `stack_table_sizes_for_compaction()` would try to\n    +    allocate a zero-byte object when there aren't any tables, and that may\n    +    lead to a `NULL` pointer on some platforms like NonStop which causes us\n    +    to bail out with an out-of-memory error.\n     \n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n     \n2:  0703e520de ! 2:  1ca81345ce reftable/merged: fix zero-sized allocation when there are no readers\n    @@ Metadata\n      ## Commit message ##\n         reftable/merged: fix zero-sized allocation when there are no readers\n     \n    -    It was reported [1c that Git started to fail with an out-of-memory error\n    +    It was reported [1] that Git started to fail with an out-of-memory error\n         when initializing repositories with the reftable backend on NonStop\n         platforms. A bisect led to 802c0646ac (reftable/merged: handle\n         allocation failures in `merged_table_init_iter()`, 2024-10-02), which\n    @@ Commit message\n         The root cause of this seems to be that NonStop returns a `NULL` pointer\n         when doing a zero-sized allocation. This would've already happened\n         before the above change, but we never noticed because we did not check\n    -    the result. Now that we do we notice and thus return an out-of-memory\n    -    error to the caller.\n    +    the result. Now we do notice and thus return an out-of-memory error to\n    +    the caller.\n     \n         Fix the issue by skipping the allocation altogether in case there are no\n         readers.\n3:  67baf67817 ! 3:  9be3c6e6c1 reftable/stack: fix zero-sized allocation when there are no readers\n    @@ Commit message\n         situation, and thus we return an error.\n     \n         Fix this by only allocating arrays when they have at least one entry.\n    -    Refactor the code so that we don't try to access those arrays in case\n    -    they are empty.\n     \n         Reported-by: Randall S. Becker <rsbecker@nexbridge.com>\n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n    @@ reftable/stack.c: static int reftable_stack_reload_once(struct reftable_stack *s\n      \tsize_t reused_len = 0, reused_alloc = 0, names_len;\n      \tsize_t new_readers_len = 0;\n      \tstruct reftable_merged_table *new_merged = NULL;\n    - \tstruct reftable_buf table_path = REFTABLE_BUF_INIT;\n    +@@ reftable/stack.c: static int reftable_stack_reload_once(struct reftable_stack *st,\n      \tint err = 0;\n    --\tsize_t i;\n    + \tsize_t i;\n      \n     -\tcur = stack_copy_readers(st, cur_len);\n     -\tif (!cur) {\n    @@ reftable/stack.c: static int reftable_stack_reload_once(struct reftable_stack *s\n      \t}\n      \n      \tnames_len = names_length(names);\n    --\n    + \n     -\tnew_readers = reftable_calloc(names_len, sizeof(*new_readers));\n     -\tif (!new_readers) {\n     -\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n    @@ reftable/stack.c: static int reftable_stack_reload_once(struct reftable_stack *s\n     +\t\t}\n      \t}\n      \n    --\twhile (*names) {\n    -+\tfor (size_t i = 0; i < names_len; i++) {\n    - \t\tstruct reftable_reader *rd = NULL;\n    --\t\tconst char *name = *names++;\n    -+\t\tconst char *name = names[i];\n    - \n    - \t\t/* this is linear; we assume compaction keeps the number of\n    - \t\t   tables under control so this is not quadratic. */\n    --\t\tfor (i = 0; reuse_open && i < cur_len; i++) {\n    --\t\t\tif (cur[i] && 0 == strcmp(cur[i]->name, name)) {\n    --\t\t\t\trd = cur[i];\n    --\t\t\t\tcur[i] = NULL;\n    -+\t\tfor (size_t j = 0; reuse_open && j < cur_len; j++) {\n    -+\t\t\tif (cur[j] && 0 == strcmp(cur[j]->name, name)) {\n    -+\t\t\t\trd = cur[j];\n    -+\t\t\t\tcur[j] = NULL;\n    - \n    - \t\t\t\t/*\n    - \t\t\t\t * When reloading the stack fails, we end up\n    -@@ reftable/stack.c: static int reftable_stack_reload_once(struct reftable_stack *st,\n    - \t * file of such an open reader wouldn't have been possible to be\n    - \t * unlinked by the compacting process.\n    - \t */\n    --\tfor (i = 0; i < cur_len; i++) {\n    -+\tfor (size_t i = 0; i < cur_len; i++) {\n    - \t\tif (cur[i]) {\n    - \t\t\tconst char *name = reader_name(cur[i]);\n    - \n    -@@ reftable/stack.c: static int reftable_stack_reload_once(struct reftable_stack *st,\n    - \t * happen on the successful case, because on the unsuccessful one we\n    - \t * decrement their refcount via `new_readers`.\n    - \t */\n    --\tfor (i = 0; i < reused_len; i++)\n    -+\tfor (size_t i = 0; i < reused_len; i++)\n    - \t\treftable_reader_decref(reused[i]);\n    - \n    - done:\n    --\tfor (i = 0; i < new_readers_len; i++)\n    -+\tfor (size_t i = 0; i < new_readers_len; i++)\n    - \t\treftable_reader_decref(new_readers[i]);\n    - \treftable_free(new_readers);\n    - \treftable_free(reused);\n    + \twhile (*names) {\n4:  16ae8201b1 = 4:  8028e2cf68 reftable/basics: return NULL on zero-sized allocations\n\n---\nbase-commit: ff795a5c5ed2e2d07c688c217a615d89e3f5733b\nchange-id: 20241220-b4-pks-reftable-oom-fix-without-readers-c7d8fda0694d\n\n"},{"id":"509494","messageId":"20241222-b4-pks-reftable-oom-fix-without-readers-v2-1-19550090d15a@pks.im","threadId":"62680","inReplyTo":"20241222-b4-pks-reftable-oom-fix-without-readers-v2-0-19550090d15a@pks.im","subject":"[PATCH v2 1/4] reftable/stack: don't perform auto-compaction with less than two tables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-22T07:24:28Z","receivedAt":"2024-12-22T07:24:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to compact tables we need at least two tables. Bail out early\nfrom `reftable_stack_auto_compact()` in case we have less than two\ntables.\n\nIn the original, `stack_table_sizes_for_compaction()` yields an array\nthat has the same length as the number of tables. This array is then\npassed on to `suggest_compaction_segment()`, which returns an empty\nsegment in case we have less than two tables. The segment is then passed\nto `segment_size()`, which will return `0` because both start and end of\nthe segment are `0`. And because we only call `stack_compact_range()` in\ncase we have a positive segment size we don't perform auto-compaction at\nall. Consequently, this change does not result in a user-visible change\nin behaviour when called with a single table.\n\nBut when called with no tables this protects us against a potential\nout-of-memory error: `stack_table_sizes_for_compaction()` would try to\nallocate a zero-byte object when there aren't any tables, and that may\nlead to a `NULL` pointer on some platforms like NonStop which causes us\nto bail out with an out-of-memory error.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 59fd695a12c2033ed589a21ef1c9155eeecc4641..6ca21965d8e1135d986043113d465abd14cd532c 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -1627,6 +1627,9 @@ int reftable_stack_auto_compact(struct reftable_stack *st)\n \tstruct segment seg;\n \tuint64_t *sizes;\n \n+\tif (st->merged->readers_len < 2)\n+\t\treturn 0;\n+\n \tsizes = stack_table_sizes_for_compaction(st);\n \tif (!sizes)\n \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509495","messageId":"20241222-b4-pks-reftable-oom-fix-without-readers-v2-2-19550090d15a@pks.im","threadId":"62680","inReplyTo":"20241222-b4-pks-reftable-oom-fix-without-readers-v2-0-19550090d15a@pks.im","subject":"[PATCH v2 2/4] reftable/merged: fix zero-sized allocation when there are no readers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-22T07:24:29Z","receivedAt":"2024-12-22T07:24:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It was reported [1] that Git started to fail with an out-of-memory error\nwhen initializing repositories with the reftable backend on NonStop\nplatforms. A bisect led to 802c0646ac (reftable/merged: handle\nallocation failures in `merged_table_init_iter()`, 2024-10-02), which\nchanged how we allocate memory when initializing a merged table.\n\nThe root cause of this seems to be that NonStop returns a `NULL` pointer\nwhen doing a zero-sized allocation. This would've already happened\nbefore the above change, but we never noticed because we did not check\nthe result. Now we do notice and thus return an out-of-memory error to\nthe caller.\n\nFix the issue by skipping the allocation altogether in case there are no\nreaders.\n\n[1]: <00ad01db5017$aa9ce340$ffd6a9c0$@nexbridge.com>\n\nReported-by: Randall S. Becker <rsbecker@nexbridge.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/merged.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/reftable/merged.c b/reftable/merged.c\nindex bb0836e3443271f9c0d5ba5582c78694d437ddc2..e72b39e178d4dec6ddfca970b5af71b71431397a 100644\n--- a/reftable/merged.c\n+++ b/reftable/merged.c\n@@ -240,14 +240,16 @@ int merged_table_init_iter(struct reftable_merged_table *mt,\n \t\t\t   struct reftable_iterator *it,\n \t\t\t   uint8_t typ)\n {\n-\tstruct merged_subiter *subiters;\n+\tstruct merged_subiter *subiters = NULL;\n \tstruct merged_iter *mi = NULL;\n \tint ret;\n \n-\tREFTABLE_CALLOC_ARRAY(subiters, mt->readers_len);\n-\tif (!subiters) {\n-\t\tret = REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tgoto out;\n+\tif (mt->readers_len) {\n+\t\tREFTABLE_CALLOC_ARRAY(subiters, mt->readers_len);\n+\t\tif (!subiters) {\n+\t\t\tret = REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t\t\tgoto out;\n+\t\t}\n \t}\n \n \tfor (size_t i = 0; i < mt->readers_len; i++) {\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509496","messageId":"20241222-b4-pks-reftable-oom-fix-without-readers-v2-3-19550090d15a@pks.im","threadId":"62680","inReplyTo":"20241222-b4-pks-reftable-oom-fix-without-readers-v2-0-19550090d15a@pks.im","subject":"[PATCH v2 3/4] reftable/stack: fix zero-sized allocation when there are no readers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-22T07:24:30Z","receivedAt":"2024-12-22T07:24:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Similar as the preceding commit, we may try to do a zero-sized\nallocation when reloading a reftable stack that ain't got any tables.\nIt is implementation-defined whether malloc(3p) returns a NULL pointer\nin that case or a zero-sized object. In case it does return a NULL\npointer though it causes us to think we have run into an out-of-memory\nsituation, and thus we return an error.\n\nFix this by only allocating arrays when they have at least one entry.\n\nReported-by: Randall S. Becker <rsbecker@nexbridge.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 24 ++++++++++++++----------\n 1 file changed, 14 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 6ca21965d8e1135d986043113d465abd14cd532c..634f0c54251b3581ca73250aca9f653f4645a569 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -270,9 +270,9 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \t\t\t\t      int reuse_open)\n {\n \tsize_t cur_len = !st->merged ? 0 : st->merged->readers_len;\n-\tstruct reftable_reader **cur;\n+\tstruct reftable_reader **cur = NULL;\n \tstruct reftable_reader **reused = NULL;\n-\tstruct reftable_reader **new_readers;\n+\tstruct reftable_reader **new_readers = NULL;\n \tsize_t reused_len = 0, reused_alloc = 0, names_len;\n \tsize_t new_readers_len = 0;\n \tstruct reftable_merged_table *new_merged = NULL;\n@@ -280,18 +280,22 @@ static int reftable_stack_reload_once(struct reftable_stack *st,\n \tint err = 0;\n \tsize_t i;\n \n-\tcur = stack_copy_readers(st, cur_len);\n-\tif (!cur) {\n-\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tgoto done;\n+\tif (cur_len) {\n+\t\tcur = stack_copy_readers(st, cur_len);\n+\t\tif (!cur) {\n+\t\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t\t\tgoto done;\n+\t\t}\n \t}\n \n \tnames_len = names_length(names);\n \n-\tnew_readers = reftable_calloc(names_len, sizeof(*new_readers));\n-\tif (!new_readers) {\n-\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n-\t\tgoto done;\n+\tif (names_len) {\n+\t\tnew_readers = reftable_calloc(names_len, sizeof(*new_readers));\n+\t\tif (!new_readers) {\n+\t\t\terr = REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t\t\tgoto done;\n+\t\t}\n \t}\n \n \twhile (*names) {\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509497","messageId":"20241222-b4-pks-reftable-oom-fix-without-readers-v2-4-19550090d15a@pks.im","threadId":"62680","inReplyTo":"20241222-b4-pks-reftable-oom-fix-without-readers-v2-0-19550090d15a@pks.im","subject":"[PATCH v2 4/4] reftable/basics: return NULL on zero-sized allocations","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-22T07:24:31Z","receivedAt":"2024-12-22T07:24:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the preceding commits we have fixed a couple of issues when\nallocating zero-sized objects. These issues were masked by\nimplementation-defined behaviour. Quoting malloc(3p):\n\n  If size is 0, either:\n\n    * A null pointer shall be returned and errno may be set to an\n      implementation-defined value, or\n\n    * A pointer to the allocated space shall be returned. The\n      application shall ensure that the pointer is not used to access an\n      object.\n\nSo it is perfectly valid that implementations of this function may or\nmay not return a NULL pointer in such a case.\n\nAdapt both `reftable_malloc()` and `reftable_realloc()` so that they\nreturn NULL pointers on zero-sized allocations. This should remove any\nimplementation-defined behaviour in our allocators and thus allows us to\ndetect such platform-specific issues more easily going forward.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/basics.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/reftable/basics.c b/reftable/basics.c\nindex 7d84a5d62dead1cf1a60698b1bb12fe6ac41c090..70b1091d1495bb5b4c8aae63bd9213dc704aecde 100644\n--- a/reftable/basics.c\n+++ b/reftable/basics.c\n@@ -17,6 +17,8 @@ static void (*reftable_free_ptr)(void *);\n \n void *reftable_malloc(size_t sz)\n {\n+\tif (!sz)\n+\t\treturn NULL;\n \tif (reftable_malloc_ptr)\n \t\treturn (*reftable_malloc_ptr)(sz);\n \treturn malloc(sz);\n@@ -24,6 +26,11 @@ void *reftable_malloc(size_t sz)\n \n void *reftable_realloc(void *p, size_t sz)\n {\n+\tif (!sz) {\n+\t\treftable_free(p);\n+\t\treturn NULL;\n+\t}\n+\n \tif (reftable_realloc_ptr)\n \t\treturn (*reftable_realloc_ptr)(p, sz);\n \treturn realloc(p, sz);\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509501","messageId":"xmqq34ifbrx1.fsf@gitster.g","threadId":"62680","inReplyTo":"20241222-b4-pks-reftable-oom-fix-without-readers-v2-0-19550090d15a@pks.im","subject":"Re: [PATCH v2 0/4] reftable: fix out-of-memory errors on NonStop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-22T16:31:38Z","receivedAt":"2024-12-22T16:31:41Z","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> Changes in v2:\n>\n>   - Some small touchups to commit messages.\n>   - Explain why it is safe to stop auto-compacting with less than two\n>     tables.\n>   - Adapt `reftable_stack_reload_once()` so that we only do the minimum\n>     changes required to fix issue.\n>   - Link to v1: https://lore.kernel.org/r/20241221-b4-pks-reftable-oom-fix-without-readers-v1-0-12db83a3267c@pks.im\n>\n> Thanks!\n\nThanks.  All four patches looked good.\n\nWill merge to 'next' and then to 'master'.\n\n"},{"id":"509502","messageId":"028401db5499$ad9cb760$08d62620$@nexbridge.com","threadId":"62680","inReplyTo":"xmqqikrcc2i6.fsf@gitster.g","subject":"RE: [PATCH 3/4] reftable/stack: fix zero-sized allocation when there are no readers","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-12-22T17:48:09Z","receivedAt":"2024-12-22T17:48:26Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On December 21, 2024 1:31 PM, Junio C Hamano wrote:\n>To: rsbecker@nexbridge.com\n>Cc: 'Patrick Steinhardt' <ps@pks.im>; git@vger.kernel.org; 'Randall S.\nBecker'\n><randall.becker@nexbridge.ca>\n>Subject: Re: [PATCH 3/4] reftable/stack: fix zero-sized allocation when\nthere are no\n>readers\n>\n><rsbecker@nexbridge.com> writes:\n>\n>>>Applying these on the author-supplied base (ff795a5c5e) yields the\n>>>same\n>> tree as\n>>>the result of merging my manual application of these four patches to\n>> ps/reftable-\n>>>alloc-failures into the same base.\n>>\n>> Ready to test this. Please let me know when and I will report results.\n>\n>If you want to start sooner\n>\n>    $ git checkout -b test ff795a5c5ed2e2d07c688c217a615d89e3f5733b\n>    $ git am ... these four patches ...\n>\n>should give you the fix without anything else mixed in.  I'll push out the\nusual four\n>branches after integration testing, but it will be queued in 'seen' (just\nabove the\n>point that corresponds to\n>'next') first, before merging it to 'next' (and then down to 'master'\nbefore -rc1).\n\nFYI: 'seen' looks better now. I am having issues in t0211-trace2-perf.sh\nwith\nundefined SSL symbols (SSL_get0_group_name), but am able to use git init\nagain.\n\n"},{"id":"509504","messageId":"028801db549d$bde37240$39aa56c0$@nexbridge.com","threadId":"62680","inReplyTo":"xmqqikrcc2i6.fsf@gitster.g","subject":"RE: [PATCH 3/4] reftable/stack: fix zero-sized allocation when there are no readers","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-12-22T18:17:15Z","receivedAt":"2024-12-22T18:17:25Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On December 22, 2024 12:48 PM, I wrote:\n>On December 21, 2024 1:31 PM, Junio C Hamano wrote:\n>>To: rsbecker@nexbridge.com\n>>Cc: 'Patrick Steinhardt' <ps@pks.im>; git@vger.kernel.org; 'Randall S.\nBecker'\n>><randall.becker@nexbridge.ca>\n>>Subject: Re: [PATCH 3/4] reftable/stack: fix zero-sized allocation when\n>>there are no readers\n>>\n>><rsbecker@nexbridge.com> writes:\n>>\n>>>>Applying these on the author-supplied base (ff795a5c5e) yields the\n>>>>same\n>>> tree as\n>>>>the result of merging my manual application of these four patches to\n>>> ps/reftable-\n>>>>alloc-failures into the same base.\n>>>\n>>> Ready to test this. Please let me know when and I will report results.\n>>\n>>If you want to start sooner\n>>\n>>    $ git checkout -b test ff795a5c5ed2e2d07c688c217a615d89e3f5733b\n>>    $ git am ... these four patches ...\n>>\n>>should give you the fix without anything else mixed in.  I'll push out\n>>the usual four branches after integration testing, but it will be\n>>queued in 'seen' (just above the point that corresponds to\n>>'next') first, before merging it to 'next' (and then down to 'master'\nbefore -rc1).\n>\n>FYI: 'seen' looks better now. I am having issues in t0211-trace2-perf.sh\nwith\n>undefined SSL symbols (SSL_get0_group_name), but am able to use git init\nagain.\n\n'seen' looks good. Operator error on trace2-perf.sh - used the wrong version\nof OpenSSL.\n\nThanks,\nRandall\n\n"},{"id":"509505","messageId":"Z2hb2CenfZ7bJ7Zj@pks.im","threadId":"62680","inReplyTo":"028801db549d$bde37240$39aa56c0$@nexbridge.com","subject":"Re: [PATCH 3/4] reftable/stack: fix zero-sized allocation when there are no readers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-22T18:35:04Z","receivedAt":"2024-12-22T18:36:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Dec 22, 2024 at 01:17:15PM -0500, rsbecker@nexbridge.com wrote:\n> On December 22, 2024 12:48 PM, I wrote:\n> >On December 21, 2024 1:31 PM, Junio C Hamano wrote:\n> >>To: rsbecker@nexbridge.com\n> >>Cc: 'Patrick Steinhardt' <ps@pks.im>; git@vger.kernel.org; 'Randall S.\n> Becker'\n> >><randall.becker@nexbridge.ca>\n> >>Subject: Re: [PATCH 3/4] reftable/stack: fix zero-sized allocation when\n> >>there are no readers\n> >>\n> >><rsbecker@nexbridge.com> writes:\n> >>\n> >>>>Applying these on the author-supplied base (ff795a5c5e) yields the\n> >>>>same\n> >>> tree as\n> >>>>the result of merging my manual application of these four patches to\n> >>> ps/reftable-\n> >>>>alloc-failures into the same base.\n> >>>\n> >>> Ready to test this. Please let me know when and I will report results.\n> >>\n> >>If you want to start sooner\n> >>\n> >>    $ git checkout -b test ff795a5c5ed2e2d07c688c217a615d89e3f5733b\n> >>    $ git am ... these four patches ...\n> >>\n> >>should give you the fix without anything else mixed in.  I'll push out\n> >>the usual four branches after integration testing, but it will be\n> >>queued in 'seen' (just above the point that corresponds to\n> >>'next') first, before merging it to 'next' (and then down to 'master'\n> before -rc1).\n> >\n> >FYI: 'seen' looks better now. I am having issues in t0211-trace2-perf.sh\n> with\n> >undefined SSL symbols (SSL_get0_group_name), but am able to use git init\n> again.\n> \n> 'seen' looks good. Operator error on trace2-perf.sh - used the wrong version\n> of OpenSSL.\n\nThanks for confirming!\n\nPatrick\n"},{"id":"509509","messageId":"xmqq7c7r2g8x.fsf@gitster.g","threadId":"62680","inReplyTo":"Z2hb2CenfZ7bJ7Zj@pks.im","subject":"Re: [PATCH 3/4] reftable/stack: fix zero-sized allocation when there are no readers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-23T04:08:30Z","receivedAt":"2024-12-23T04:08:33Z","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>> >FYI: 'seen' looks better now. I am having issues in t0211-trace2-perf.sh\n>> with\n>> >undefined SSL symbols (SSL_get0_group_name), but am able to use git init\n>> again.\n>> \n>> 'seen' looks good. Operator error on trace2-perf.sh - used the wrong version\n>> of OpenSSL.\n>\n> Thanks for confirming!\n\nThanks, all.\n"}]}