{"thread":{"id":"57736","subject":"[PATCH] reftable: avoid undefined behaviour breaking t0032","startedAt":"2022-04-15T07:03:01Z","lastAt":"2022-04-25T17:30:25Z","messageCount":16,"participants":["Carlo Marcelo Arenas Belón","Junio C Hamano","Ævar Arnfjörð Bjarmason","René Scharfe","Phillip Wood","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"453684","messageId":"20220415070236.25280-1-carenas@gmail.com","threadId":"57736","inReplyTo":null,"subject":"[PATCH] reftable: avoid undefined behaviour breaking t0032","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2022-04-15T07:02:36Z","receivedAt":"2022-04-15T07:03:01Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"At least in glibc based systems, memset with a NULL first parameter\nwill cause a runtime exception.\n\nAvoid doing so by adding a conditional to check for NULL in all three\nidentically looking functions that were affected.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\nBug was introduced with the original code in 1214aa841bc (reftable: add\nblocksource, an abstraction for random access reads, 2021-10-07), so not\nto be considered a regression for this release.\n\n reftable/blocksource.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/reftable/blocksource.c b/reftable/blocksource.c\nindex 0044eecd9aa..984bf07fc17 100644\n--- a/reftable/blocksource.c\n+++ b/reftable/blocksource.c\n@@ -15,7 +15,8 @@ license that can be found in the LICENSE file or at\n \n static void strbuf_return_block(void *b, struct reftable_block *dest)\n {\n-\tmemset(dest->data, 0xff, dest->len);\n+\tif (dest->data)\n+\t\tmemset(dest->data, 0xff, dest->len);\n \treftable_free(dest->data);\n }\n \n@@ -56,7 +57,8 @@ void block_source_from_strbuf(struct reftable_block_source *bs,\n \n static void malloc_return_block(void *b, struct reftable_block *dest)\n {\n-\tmemset(dest->data, 0xff, dest->len);\n+\tif (dest->data)\n+\t\tmemset(dest->data, 0xff, dest->len);\n \treftable_free(dest->data);\n }\n \n@@ -85,7 +87,8 @@ static uint64_t file_size(void *b)\n \n static void file_return_block(void *b, struct reftable_block *dest)\n {\n-\tmemset(dest->data, 0xff, dest->len);\n+\tif (dest->data)\n+\t\tmemset(dest->data, 0xff, dest->len);\n \treftable_free(dest->data);\n }\n \n-- \n2.36.0.rc2.283.gbef64175c85\n\n"},{"id":"453685","messageId":"xmqq8rs6g6tg.fsf@gitster.g","threadId":"57736","inReplyTo":"20220415070236.25280-1-carenas@gmail.com","subject":"Re: [PATCH] reftable: avoid undefined behaviour breaking t0032","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-15T07:10:51Z","receivedAt":"2022-04-15T07:11:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n\n> At least in glibc based systems, memset with a NULL first parameter\n> will cause a runtime exception.\n\nI take it to mean that the code assumes that it is OK to pass NULL\nas long as length is 0 (i.e. filling the range of memory whose size\nis 0 with the specified byte can happen safely no matter what the\nstarting address of that range is, as size==0 by definition should\nmean a no-op).  That would mean we have a rule on how members of\ndest must be set: .data is allowed to be NULL only when .len is 0.\n\nIf so, I wonder if we want to guard with dest->len instead, i.e.\n\n\tif (dest->len)\n\t\tmemset(dest->data, 0xff, dest->len);\n\nWith the form in this patch, i.e.\n\n> -\tmemset(dest->data, 0xff, dest->len);\n> +\tif (dest->data)\n> +\t\tmemset(dest->data, 0xff, dest->len);\n\nwe will fail to catch a bogus caller that violates the rule above\nthat we have on <data, len>.  But if we guard with dest->len, then\na violator of <data, len> rule will be caught by memset().\n\nThanks.\n\n"},{"id":"453686","messageId":"20220415083058.29495-1-carenas@gmail.com","threadId":"57736","inReplyTo":"20220415070236.25280-1-carenas@gmail.com","subject":"[PATCH v2] reftable: avoid undefined behaviour breaking t0032","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2022-04-15T08:30:59Z","receivedAt":"2022-04-15T08:34:54Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"1214aa841bc (reftable: add blocksource, an abstraction for random\naccess reads, 2021-10-07), makes the assumption that it is ok to\nfree a reftable_block pointing to NULL if the size is also set to\n0, but implements that using a memset call that at least in glibc\nbased system will trigger a runtime exception if called with a\nNULL pointer as its first parameter.\n\nAvoid doing so by adding a conditional to check for the size in all\nthree identically looking functions that were affected, and therefore,\nstill allow memset to help catch callers that might incorrectly pass\na NULL pointer with a non zero size, but avoiding the exception for\nthe valid cases.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\nChanges since v1:\n- Improved logic as suggested by Junio\n- Hopefully also improved commit message\n\n reftable/blocksource.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/reftable/blocksource.c b/reftable/blocksource.c\nindex 0044eecd9aa..db1b7dc966f 100644\n--- a/reftable/blocksource.c\n+++ b/reftable/blocksource.c\n@@ -15,7 +15,8 @@ license that can be found in the LICENSE file or at\n \n static void strbuf_return_block(void *b, struct reftable_block *dest)\n {\n-\tmemset(dest->data, 0xff, dest->len);\n+\tif (dest->len)\n+\t\tmemset(dest->data, 0xff, dest->len);\n \treftable_free(dest->data);\n }\n \n@@ -56,7 +57,8 @@ void block_source_from_strbuf(struct reftable_block_source *bs,\n \n static void malloc_return_block(void *b, struct reftable_block *dest)\n {\n-\tmemset(dest->data, 0xff, dest->len);\n+\tif (dest->len)\n+\t\tmemset(dest->data, 0xff, dest->len);\n \treftable_free(dest->data);\n }\n \n@@ -85,7 +87,8 @@ static uint64_t file_size(void *b)\n \n static void file_return_block(void *b, struct reftable_block *dest)\n {\n-\tmemset(dest->data, 0xff, dest->len);\n+\tif (dest->len)\n+\t\tmemset(dest->data, 0xff, dest->len);\n \treftable_free(dest->data);\n }\n \n-- \n2.36.0.rc2.283.gbef64175c85\n\n"},{"id":"453688","messageId":"RFC-cover-0.2-00000000000-20220415T101740Z-avarab@gmail.com","threadId":"57736","inReplyTo":"20220415083058.29495-1-carenas@gmail.com","subject":"[RFC PATCH 0/2] reftable: remove poor man's SANITIZE=address, fix a memset() bug","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-15T10:21:34Z","receivedAt":"2022-04-15T10:21:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Apr 15 2022, Carlo Marcelo Arenas Belón wrote:\n\n> 1214aa841bc (reftable: add blocksource, an abstraction for random\n> access reads, 2021-10-07), makes the assumption that it is ok to\n> free a reftable_block pointing to NULL if the size is also set to\n> 0, but implements that using a memset call that at least in glibc\n> based system will trigger a runtime exception if called with a\n> NULL pointer as its first parameter.\n\nFWIW I've been carrying 1/2 here for a while in my local tree,\ni.e. reftable/* has various abstractions and indirections that aren't\nreally needed. In this case we can just get rid of that & free them,\nso the memset()s you fixed can just be removed.\n\nThe 2/2 is then another memset() issue I spotted when looking at this\nagain, -fanalyzer notes the bug related to it.\n\nÆvar Arnfjörð Bjarmason (2):\n  reftable: remove the \"return_block\" abstraction\n  reftable: don't memset() a NULL from failed malloc()\n\n reftable/block.c                |  4 +---\n reftable/blocksource.c          | 28 +---------------------------\n reftable/publicbasics.c         |  2 ++\n reftable/reftable-blocksource.h |  2 --\n 4 files changed, 4 insertions(+), 32 deletions(-)\n\n-- \n2.36.0.rc2.863.gfc2c14e3b91\n\n"},{"id":"453689","messageId":"RFC-patch-1.2-76ed86bf88c-20220415T101740Z-avarab@gmail.com","threadId":"57736","inReplyTo":"RFC-cover-0.2-00000000000-20220415T101740Z-avarab@gmail.com","subject":"[RFC PATCH 1/2] reftable: remove the \"return_block\" abstraction","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-15T10:21:35Z","receivedAt":"2022-04-15T10:21:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This abstraction added in 1214aa841bc (reftable: add blocksource, an\nabstraction for random access reads, 2021-10-07) has the caller\nprovide a \"blockp->data\", so there's not point in having the vtable\nhave a custom free() function.\n\nIn addition this had what looked like a poor man's SANITIZE=address\ndoing a memset() to 0xff just before the data was free'd.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n reftable/block.c                |  4 +---\n reftable/blocksource.c          | 28 +---------------------------\n reftable/reftable-blocksource.h |  2 --\n 3 files changed, 2 insertions(+), 32 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 34d4d073692..bb17cc32372 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -442,9 +442,7 @@ void block_writer_release(struct block_writer *bw)\n \n void reftable_block_done(struct reftable_block *blockp)\n {\n-\tstruct reftable_block_source source = blockp->source;\n-\tif (blockp && source.ops)\n-\t\tsource.ops->return_block(source.arg, blockp);\n+\tFREE_AND_NULL(blockp->data);\n \tblockp->data = NULL;\n \tblockp->len = 0;\n \tblockp->source.ops = NULL;\ndiff --git a/reftable/blocksource.c b/reftable/blocksource.c\nindex 2605371c28d..d9e47cc316b 100644\n--- a/reftable/blocksource.c\n+++ b/reftable/blocksource.c\n@@ -13,12 +13,6 @@ license that can be found in the LICENSE file or at\n #include \"reftable-blocksource.h\"\n #include \"reftable-error.h\"\n \n-static void strbuf_return_block(void *b, struct reftable_block *dest)\n-{\n-\tmemset(dest->data, 0xff, dest->len);\n-\treftable_free(dest->data);\n-}\n-\n static void strbuf_close(void *b)\n {\n }\n@@ -42,7 +36,6 @@ static uint64_t strbuf_size(void *b)\n static struct reftable_block_source_vtable strbuf_vtable = {\n \t.size = &strbuf_size,\n \t.read_block = &strbuf_read_block,\n-\t.return_block = &strbuf_return_block,\n \t.close = &strbuf_close,\n };\n \n@@ -54,19 +47,7 @@ void block_source_from_strbuf(struct reftable_block_source *bs,\n \tbs->arg = buf;\n }\n \n-static void malloc_return_block(void *b, struct reftable_block *dest)\n-{\n-\tmemset(dest->data, 0xff, dest->len);\n-\treftable_free(dest->data);\n-}\n-\n-static struct reftable_block_source_vtable malloc_vtable = {\n-\t.return_block = &malloc_return_block,\n-};\n-\n-static struct reftable_block_source malloc_block_source_instance = {\n-\t.ops = &malloc_vtable,\n-};\n+static struct reftable_block_source malloc_block_source_instance = { 0 };\n \n struct reftable_block_source malloc_block_source(void)\n {\n@@ -83,12 +64,6 @@ static uint64_t file_size(void *b)\n \treturn ((struct file_block_source *)b)->size;\n }\n \n-static void file_return_block(void *b, struct reftable_block *dest)\n-{\n-\tmemset(dest->data, 0xff, dest->len);\n-\treftable_free(dest->data);\n-}\n-\n static void file_close(void *b)\n {\n \tint fd = ((struct file_block_source *)b)->fd;\n@@ -115,7 +90,6 @@ static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,\n static struct reftable_block_source_vtable file_vtable = {\n \t.size = &file_size,\n \t.read_block = &file_read_block,\n-\t.return_block = &file_return_block,\n \t.close = &file_close,\n };\n \ndiff --git a/reftable/reftable-blocksource.h b/reftable/reftable-blocksource.h\nindex 5aa3990a573..7b7cb280b73 100644\n--- a/reftable/reftable-blocksource.h\n+++ b/reftable/reftable-blocksource.h\n@@ -35,8 +35,6 @@ struct reftable_block_source_vtable {\n \t   beyond the end of the block */\n \tint (*read_block)(void *source, struct reftable_block *dest,\n \t\t\t  uint64_t off, uint32_t size);\n-\t/* mark the block as read; may return the data back to malloc */\n-\tvoid (*return_block)(void *source, struct reftable_block *blockp);\n \n \t/* release all resources associated with the block source */\n \tvoid (*close)(void *source);\n-- \n2.36.0.rc2.863.gfc2c14e3b91\n\n"},{"id":"453690","messageId":"RFC-patch-2.2-364d1194a95-20220415T101740Z-avarab@gmail.com","threadId":"57736","inReplyTo":"RFC-cover-0.2-00000000000-20220415T101740Z-avarab@gmail.com","subject":"[RFC PATCH 2/2] reftable: don't memset() a NULL from failed malloc()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-15T10:21:36Z","receivedAt":"2022-04-15T10:21:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Return NULL instead of possibly feeding a NULL to memset() in\nreftable_calloc(). This issue was noted by GCC 12's -fanalyzer:\n\n\treftable/publicbasics.c: In function ‘reftable_calloc’:\n\treftable/publicbasics.c:43:9: error: use of possibly-NULL ‘p’ where non-null expected [CWE-690] [-Werror=analyzer-possible-null-argument]\n\t   43 |         memset(p, 0, sz);\n\t      |         ^~~~~~~~~~~~~~~~\n\t[...]\n\nThis bug has been with us ever since this code was added in\nef8a6c62687 (reftable: utility functions, 2021-10-07).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n reftable/publicbasics.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/reftable/publicbasics.c b/reftable/publicbasics.c\nindex 0ad7d5c0ff2..a18167f5ab7 100644\n--- a/reftable/publicbasics.c\n+++ b/reftable/publicbasics.c\n@@ -40,6 +40,8 @@ void reftable_free(void *p)\n void *reftable_calloc(size_t sz)\n {\n \tvoid *p = reftable_malloc(sz);\n+\tif (!p)\n+\t\treturn NULL;\n \tmemset(p, 0, sz);\n \treturn p;\n }\n-- \n2.36.0.rc2.863.gfc2c14e3b91\n\n"},{"id":"453701","messageId":"a1b7d41f-561f-6806-1d53-2c15d4a22fdc@web.de","threadId":"57736","inReplyTo":"RFC-patch-1.2-76ed86bf88c-20220415T101740Z-avarab@gmail.com","subject":"Re: [RFC PATCH 1/2] reftable: remove the \"return_block\" abstraction","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-04-15T13:37:25Z","receivedAt":"2022-04-15T13:37:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.04.22 um 12:21 schrieb Ævar Arnfjörð Bjarmason:\n> This abstraction added in 1214aa841bc (reftable: add blocksource, an\n> abstraction for random access reads, 2021-10-07) has the caller\n> provide a \"blockp->data\", so there's not point in having the vtable\n> have a custom free() function.\n>\n> In addition this had what looked like a poor man's SANITIZE=address\n> doing a memset() to 0xff just before the data was free'd.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  reftable/block.c                |  4 +---\n>  reftable/blocksource.c          | 28 +---------------------------\n>  reftable/reftable-blocksource.h |  2 --\n>  3 files changed, 2 insertions(+), 32 deletions(-)\n>\n> diff --git a/reftable/block.c b/reftable/block.c\n> index 34d4d073692..bb17cc32372 100644\n> --- a/reftable/block.c\n> +++ b/reftable/block.c\n> @@ -442,9 +442,7 @@ void block_writer_release(struct block_writer *bw)\n>\n>  void reftable_block_done(struct reftable_block *blockp)\n>  {\n> -\tstruct reftable_block_source source = blockp->source;\n> -\tif (blockp && source.ops)\n> -\t\tsource.ops->return_block(source.arg, blockp);\n\nI don't understand this interface.  Why is source.arg passed to the\nfunction?  Is this perhaps done in preparation for some kind of backend\nthat is planned to be added later which makes use of it?  I suspect we'd\nbetter postpone cleanups until the full functionality is integrated.\n\n> +\tFREE_AND_NULL(blockp->data);\n\nIf you do that...\n\n>  \tblockp->data = NULL;\n\n... then this line becomes redundant.\n\n>  \tblockp->len = 0;\n>  \tblockp->source.ops = NULL;\n> diff --git a/reftable/blocksource.c b/reftable/blocksource.c\n> index 2605371c28d..d9e47cc316b 100644\n> --- a/reftable/blocksource.c\n> +++ b/reftable/blocksource.c\n> @@ -13,12 +13,6 @@ license that can be found in the LICENSE file or at\n>  #include \"reftable-blocksource.h\"\n>  #include \"reftable-error.h\"\n>\n> -static void strbuf_return_block(void *b, struct reftable_block *dest)\n> -{\n> -\tmemset(dest->data, 0xff, dest->len);\n> -\treftable_free(dest->data);\n> -}\n> -\n>  static void strbuf_close(void *b)\n>  {\n>  }\n> @@ -42,7 +36,6 @@ static uint64_t strbuf_size(void *b)\n>  static struct reftable_block_source_vtable strbuf_vtable = {\n>  \t.size = &strbuf_size,\n>  \t.read_block = &strbuf_read_block,\n> -\t.return_block = &strbuf_return_block,\n>  \t.close = &strbuf_close,\n>  };\n>\n> @@ -54,19 +47,7 @@ void block_source_from_strbuf(struct reftable_block_source *bs,\n>  \tbs->arg = buf;\n>  }\n>\n> -static void malloc_return_block(void *b, struct reftable_block *dest)\n> -{\n> -\tmemset(dest->data, 0xff, dest->len);\n> -\treftable_free(dest->data);\n> -}\n> -\n> -static struct reftable_block_source_vtable malloc_vtable = {\n> -\t.return_block = &malloc_return_block,\n> -};\n> -\n> -static struct reftable_block_source malloc_block_source_instance = {\n> -\t.ops = &malloc_vtable,\n> -};\n> +static struct reftable_block_source malloc_block_source_instance = { 0 };\n>\n>  struct reftable_block_source malloc_block_source(void)\n>  {\n> @@ -83,12 +64,6 @@ static uint64_t file_size(void *b)\n>  \treturn ((struct file_block_source *)b)->size;\n>  }\n>\n> -static void file_return_block(void *b, struct reftable_block *dest)\n> -{\n> -\tmemset(dest->data, 0xff, dest->len);\n> -\treftable_free(dest->data);\n> -}\n> -\n>  static void file_close(void *b)\n>  {\n>  \tint fd = ((struct file_block_source *)b)->fd;\n> @@ -115,7 +90,6 @@ static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,\n>  static struct reftable_block_source_vtable file_vtable = {\n>  \t.size = &file_size,\n>  \t.read_block = &file_read_block,\n> -\t.return_block = &file_return_block,\n>  \t.close = &file_close,\n>  };\n>\n> diff --git a/reftable/reftable-blocksource.h b/reftable/reftable-blocksource.h\n> index 5aa3990a573..7b7cb280b73 100644\n> --- a/reftable/reftable-blocksource.h\n> +++ b/reftable/reftable-blocksource.h\n> @@ -35,8 +35,6 @@ struct reftable_block_source_vtable {\n>  \t   beyond the end of the block */\n>  \tint (*read_block)(void *source, struct reftable_block *dest,\n>  \t\t\t  uint64_t off, uint32_t size);\n> -\t/* mark the block as read; may return the data back to malloc */\n> -\tvoid (*return_block)(void *source, struct reftable_block *blockp);\n>\n>  \t/* release all resources associated with the block source */\n>  \tvoid (*close)(void *source);\n\n"},{"id":"453702","messageId":"4321a9dd-bb82-e2fe-5449-395f998378c5@web.de","threadId":"57736","inReplyTo":"RFC-patch-2.2-364d1194a95-20220415T101740Z-avarab@gmail.com","subject":"Re: [RFC PATCH 2/2] reftable: don't memset() a NULL from failed malloc()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-04-15T13:37:21Z","receivedAt":"2022-04-15T13:38:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.04.22 um 12:21 schrieb Ævar Arnfjörð Bjarmason:\n> Return NULL instead of possibly feeding a NULL to memset() in\n> reftable_calloc(). This issue was noted by GCC 12's -fanalyzer:\n>\n> \treftable/publicbasics.c: In function ‘reftable_calloc’:\n> \treftable/publicbasics.c:43:9: error: use of possibly-NULL ‘p’ where non-null expected [CWE-690] [-Werror=analyzer-possible-null-argument]\n> \t   43 |         memset(p, 0, sz);\n> \t      |         ^~~~~~~~~~~~~~~~\n> \t[...]\n>\n> This bug has been with us ever since this code was added in\n> ef8a6c62687 (reftable: utility functions, 2021-10-07).\n\nNot sure it's a bug, or limited to this specific location.  AFAICS it's\nmore a general lack of handling of allocation failures in the reftable\ncode.  Perhaps it was a conscious decision to let the system deal with\nthem via segfaults?\n\nWhen the code is used by Git eventually it should use xmalloc and\nxrealloc instead of calling malloc(3) and realloc(3) directly, to\nhandle allocation failures explicitly, and to support GIT_ALLOC_LIMIT.\nThis will calm down the analyzer and extend the safety to the rest of\nthe reftable code, not just this memset call.\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  reftable/publicbasics.c | 2 ++\n>  1 file changed, 2 insertions(+)\n>\n> diff --git a/reftable/publicbasics.c b/reftable/publicbasics.c\n> index 0ad7d5c0ff2..a18167f5ab7 100644\n> --- a/reftable/publicbasics.c\n> +++ b/reftable/publicbasics.c\n> @@ -40,6 +40,8 @@ void reftable_free(void *p)\n>  void *reftable_calloc(size_t sz)\n>  {\n>  \tvoid *p = reftable_malloc(sz);\n> +\tif (!p)\n> +\t\treturn NULL;\n>  \tmemset(p, 0, sz);\n\nThis function is calloc(3) reimplemented, except it can't make use of\npre-zero'd blocks of memory and doesn't multiply element size and count\nfor the caller while checking for overflow, making it slower and harder\nto use securely. :-/\n\n>  \treturn p;\n>  }\n"},{"id":"453705","messageId":"220415.86fsmebgds.gmgdl@evledraar.gmail.com","threadId":"57736","inReplyTo":"4321a9dd-bb82-e2fe-5449-395f998378c5@web.de","subject":"Re: [RFC PATCH 2/2] reftable: don't memset() a NULL from failed malloc()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-15T13:53:43Z","receivedAt":"2022-04-15T13:55:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 15 2022, René Scharfe wrote:\n\n> Am 15.04.22 um 12:21 schrieb Ævar Arnfjörð Bjarmason:\n>> Return NULL instead of possibly feeding a NULL to memset() in\n>> reftable_calloc(). This issue was noted by GCC 12's -fanalyzer:\n>>\n>> \treftable/publicbasics.c: In function ‘reftable_calloc’:\n>> \treftable/publicbasics.c:43:9: error: use of possibly-NULL ‘p’ where non-null expected [CWE-690] [-Werror=analyzer-possible-null-argument]\n>> \t   43 |         memset(p, 0, sz);\n>> \t      |         ^~~~~~~~~~~~~~~~\n>> \t[...]\n>>\n>> This bug has been with us ever since this code was added in\n>> ef8a6c62687 (reftable: utility functions, 2021-10-07).\n>\n> Not sure it's a bug, or limited to this specific location.  AFAICS it's\n> more a general lack of handling of allocation failures in the reftable\n> code.  Perhaps it was a conscious decision to let the system deal with\n> them via segfaults?\n\nI think it's just buggy, it tries to deal with malloc returning NULL in\nother contexts.\n\n> When the code is used by Git eventually it should use xmalloc and\n> xrealloc instead of calling malloc(3) and realloc(3) directly, to\n> handle allocation failures explicitly, and to support GIT_ALLOC_LIMIT.\n> This will calm down the analyzer and extend the safety to the rest of\n> the reftable code, not just this memset call.\n\nYeah, its whole custom malloc handling should go away.\n\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>>  reftable/publicbasics.c | 2 ++\n>>  1 file changed, 2 insertions(+)\n>>\n>> diff --git a/reftable/publicbasics.c b/reftable/publicbasics.c\n>> index 0ad7d5c0ff2..a18167f5ab7 100644\n>> --- a/reftable/publicbasics.c\n>> +++ b/reftable/publicbasics.c\n>> @@ -40,6 +40,8 @@ void reftable_free(void *p)\n>>  void *reftable_calloc(size_t sz)\n>>  {\n>>  \tvoid *p = reftable_malloc(sz);\n>> +\tif (!p)\n>> +\t\treturn NULL;\n>>  \tmemset(p, 0, sz);\n>\n> This function is calloc(3) reimplemented, except it can't make use of\n> pre-zero'd blocks of memory and doesn't multiply element size and count\n> for the caller while checking for overflow, making it slower and harder\n> to use securely. :-/\n\n*nod*, this is really just re-arranging the deck chairs.\n\nMaybe Han-Wen had something in mind, but I really don't see the point of\nhaving it use malloc wrappers at all, as opposed to just having the\nlinker point it to the right \"malloc\".\n\nSo if it needs to be used outside of git.git it would just need the\ntrivial xmalloc() etc. wrappers.\n"},{"id":"453709","messageId":"23138e2b-436a-0990-cefc-0662674373c3@talktalk.net","threadId":"57736","inReplyTo":"220415.86fsmebgds.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 2/2] reftable: don't memset() a NULL from failed malloc()","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2022-04-15T14:30:11Z","receivedAt":"2022-04-15T14:31:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar and René\n\nOn 15/04/2022 14:53, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Apr 15 2022, René Scharfe wrote:\n> \n>> Am 15.04.22 um 12:21 schrieb Ævar Arnfjörð Bjarmason:\n>>> Return NULL instead of possibly feeding a NULL to memset() in\n>>> reftable_calloc(). This issue was noted by GCC 12's -fanalyzer:\n>>>\n>>> \treftable/publicbasics.c: In function ‘reftable_calloc’:\n>>> \treftable/publicbasics.c:43:9: error: use of possibly-NULL ‘p’ where non-null expected [CWE-690] [-Werror=analyzer-possible-null-argument]\n>>> \t   43 |         memset(p, 0, sz);\n>>> \t      |         ^~~~~~~~~~~~~~~~\n>>> \t[...]\n>>>\n>>> This bug has been with us ever since this code was added in\n>>> ef8a6c62687 (reftable: utility functions, 2021-10-07).\n>>\n>> Not sure it's a bug, or limited to this specific location.  AFAICS it's\n>> more a general lack of handling of allocation failures in the reftable\n>> code.  Perhaps it was a conscious decision to let the system deal with\n>> them via segfaults?\n> \n> I think it's just buggy, it tries to deal with malloc returning NULL in\n> other contexts.\n\nDoes it? I just quickly scanned the output of\n\ngit grep -e 'reftable_[mc]alloc' -e 'reftable_realloc' -A2 origin/master\n\nand I didn't see a single NULL check\n\n>> When the code is used by Git eventually it should use xmalloc and\n>> xrealloc instead of calling malloc(3) and realloc(3) directly, to\n>> handle allocation failures explicitly, and to support GIT_ALLOC_LIMIT.\n>> This will calm down the analyzer and extend the safety to the rest of\n>> the reftable code, not just this memset call.\n> \n> Yeah, its whole custom malloc handling should go away.\n\nIsn't it there so the same code can be used by libgit2 and other things \nthat let the caller specify a custom allocator?\n\nBest Wishes\n\nPhillip\n"},{"id":"453710","messageId":"220415.86bkx2bb0p.gmgdl@evledraar.gmail.com","threadId":"57736","inReplyTo":"23138e2b-436a-0990-cefc-0662674373c3@talktalk.net","subject":"Re: [RFC PATCH 2/2] reftable: don't memset() a NULL from failed malloc()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-15T15:20:53Z","receivedAt":"2022-04-15T15:51:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 15 2022, Phillip Wood wrote:\n\n> Hi Ævar and René\n>\n> On 15/04/2022 14:53, Ævar Arnfjörð Bjarmason wrote:\n>> On Fri, Apr 15 2022, René Scharfe wrote:\n>> \n>>> Am 15.04.22 um 12:21 schrieb Ævar Arnfjörð Bjarmason:\n>>>> Return NULL instead of possibly feeding a NULL to memset() in\n>>>> reftable_calloc(). This issue was noted by GCC 12's -fanalyzer:\n>>>>\n>>>> \treftable/publicbasics.c: In function ‘reftable_calloc’:\n>>>> \treftable/publicbasics.c:43:9: error: use of possibly-NULL ‘p’ where non-null expected [CWE-690] [-Werror=analyzer-possible-null-argument]\n>>>> \t   43 |         memset(p, 0, sz);\n>>>> \t      |         ^~~~~~~~~~~~~~~~\n>>>> \t[...]\n>>>>\n>>>> This bug has been with us ever since this code was added in\n>>>> ef8a6c62687 (reftable: utility functions, 2021-10-07).\n>>>\n>>> Not sure it's a bug, or limited to this specific location.  AFAICS it's\n>>> more a general lack of handling of allocation failures in the reftable\n>>> code.  Perhaps it was a conscious decision to let the system deal with\n>>> them via segfaults?\n>> I think it's just buggy, it tries to deal with malloc returning NULL\n>> in\n>> other contexts.\n>\n> Does it? I just quickly scanned the output of\n>\n> git grep -e 'reftable_[mc]alloc' -e 'reftable_realloc' -A2 origin/master\n>\n> and I didn't see a single NULL check\n\nI think you're right, I retrieved that information from wetware. Looking\nagain I think I was confusing it with the if (x) free(x) fixes in\n34230514b83 (Merge branch 'hn/reftable-coverity-fixes', 2022-02-16).\n\n>>> When the code is used by Git eventually it should use xmalloc and\n>>> xrealloc instead of calling malloc(3) and realloc(3) directly, to\n>>> handle allocation failures explicitly, and to support GIT_ALLOC_LIMIT.\n>>> This will calm down the analyzer and extend the safety to the rest of\n>>> the reftable code, not just this memset call.\n>> Yeah, its whole custom malloc handling should go away.\n>\n> Isn't it there so the same code can be used by libgit2 and other\n> things that let the caller specify a custom allocator?\n\nI don't think so, but I may be wrong. I think it's a case of\nover-generalization where we'd be better off with something simpler,\ni.e. just using our normal allocation functions.\n\nThat still allows for taking this code and plugging it into any custom\nallocator. If you simply want to compile some code such as this to use\nanother allocator you can easily do that via the linker, as\ne.g. git.git's build process allows you to do with nedmalloc.\n\nSo presumably if libgit2 would want to use this they'd just do that via\ntheir build process.\n\nI.e. they'd have \"malloc\" point to a symbol that would resolve to a\nshimmy layer that would dispatch to functions using this:\nhttps://github.com/libgit2/libgit2/blob/main/src/util/alloc.c\n\nThe reason you'd use these sorts of pluggable malloc functions is, I\nthink, a few:\n\n 1. You are a library like libgit2, as opposed to libreftable itself, so\n    you want to provide this sort of \"set the alloc pointers to this\"\n    now. But in this case we either always want malloc/free (git.git) or\n    the \"parent\" can provide these wrappers (libgit2).\n\n 2. You want to support switching dynamically (or not-globally), or have\n    N instances with different malloc, as e.g. the PCREv2 API does:\n    cbe81e653fa (grep/pcre2: move back to thread-only PCREv2 structures,\n    2021-02-18).\n\n    For this code I think that's thoroughly in YAGNI territory.\n\n 3. Your distribution model can't assume the user can adjust this via\n    linking, e.g. C source that's intended to be #included as-is\n    (although there you could use defines...) etc.\n\nBut maybe I'm missing something.\n\nBut a really good reason not to do this and just rely on the link-time\noverriding is:\n\n A. Things look the same, and benefit from more general fixes (although\n    to be fair the x*alloc() wrappers would work either way..)\n\n B. You don't get subtle bugs where you forget some_custom_malloc() and\n    then use a generic free(). Having looked just now reftable/ has at\n    least one such submarine-segfault waiting to happen.\n\n C. If you don't actually need the hyper-customize pluggable model of\n    say PCREv2 where the library is trying to support having two\n    patterns in memory managed by different allocators, or other such\n    advanced use-cases it's just extra complexity.\n\nSome of this was discussed for xdiff/* in this thread:\nhttps://lore.kernel.org/git/220216.86leybszht.gmgdl@evledraar.gmail.com/\n"},{"id":"453712","messageId":"xmqq1qxyfh8k.fsf@gitster.g","threadId":"57736","inReplyTo":"220415.86bkx2bb0p.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 2/2] reftable: don't memset() a NULL from failed malloc()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-15T16:23:23Z","receivedAt":"2022-04-15T16:23:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> Does it? I just quickly scanned the output of\n>>\n>> git grep -e 'reftable_[mc]alloc' -e 'reftable_realloc' -A2 origin/master\n>>\n>> and I didn't see a single NULL check\n>\n> I think you're right, I retrieved that information from wetware. Looking\n> again I think I was confusing it with the if (x) free(x) fixes in\n> 34230514b83 (Merge branch 'hn/reftable-coverity-fixes', 2022-02-16).\n\nTrue.  Initially I hoped that these RFC changes gives us a better\nsolution that comes from stepping back and rethinking the issues\naround the original \"why are we calling memset() with NULL?\", and\nif it were only \"well, in _return_block() functions, we clear the\nblock before calling _free()---that shouldn't be necessary, if the\ncalling custom malloc-free pair cares, they can do the clearing, and\nplain vanilla free() certainly doesn't, so let's stop calling\nmemset()\", it certainly would have fallen into that category, but\neverything these RFC patches do beyond that seems \"oh, it does not\nlook important to me, so let's rip it out to simplify\", without\nknowing enough to say if that is sensible.\n\nBut ...\n\n>> Isn't it there so the same code can be used by libgit2 and other\n>> things that let the caller specify a custom allocator?\n>\n> I don't think so, but I may be wrong. I think it's a case of\n> over-generalization where we'd be better off with something simpler,\n> i.e. just using our normal allocation functions.\n\n... many points that was raised on the reftable code in this thread\nmay deserve response to explain the rationale behind the current\ncode and design, as nobody can improve, without breaking the\nintended way to be used, without knowing what it is.  Thanks for\nmarking the patches with RFC.  I think the \"patch\" is a bit too\ndense a form to point out the issues in the existing code, but the\ndiscussion that follows by quoting the points in the original code\nand suggested changes is a good way to think about the code before\nthese RFC patches.\n\nIdeally, it would have been much better if these points were raised\nduring the review before the code was accepted to the code base, but\nit is better to have one now, rather than never ;-)\n\nTHanks.\n"},{"id":"454377","messageId":"CAFQ2z_O7ytx=AsXuJbYnOdMwT0u03sMP-ZjRRjt4HFn48HT-2g@mail.gmail.com","threadId":"57736","inReplyTo":"RFC-patch-1.2-76ed86bf88c-20220415T101740Z-avarab@gmail.com","subject":"Re: [RFC PATCH 1/2] reftable: remove the \"return_block\" abstraction","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2022-04-25T09:57:38Z","receivedAt":"2022-04-25T09:57:59Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Fri, Apr 15, 2022 at 12:21 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> This abstraction added in 1214aa841bc (reftable: add blocksource, an\n> abstraction for random access reads, 2021-10-07) has the caller\n> provide a \"blockp->data\", so there's not point in having the vtable\n> have a custom free() function.\n>\n> In addition this had what looked like a poor man's SANITIZE=address\n> doing a memset() to 0xff just before the data was free'd.\n\n>  void reftable_block_done(struct reftable_block *blockp)\n>  {\n> -       struct reftable_block_source source = blockp->source;\n> -       if (blockp && source.ops)\n> -               source.ops->return_block(source.arg, blockp);\n> +       FREE_AND_NULL(blockp->data);\n\n\nMy thinking here is that we could mmap the reftable file to do reads.\nIn that case, discarding the block would imply decreasing a refcount\nsomewhere, rather than deallocating memory.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n"},{"id":"454378","messageId":"CAFQ2z_MnFmyW064xPqc1EG1pRNq53pzab9zmt=rxHdGxA2Txcg@mail.gmail.com","threadId":"57736","inReplyTo":"20220415083058.29495-1-carenas@gmail.com","subject":"Re: [PATCH v2] reftable: avoid undefined behaviour breaking t0032","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2022-04-25T10:18:42Z","receivedAt":"2022-04-25T10:18:59Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Fri, Apr 15, 2022 at 10:34 AM Carlo Marcelo Arenas Belón\n<carenas@gmail.com> wrote:\n> 1214aa841bc (reftable: add blocksource, an abstraction for random\n> access reads, 2021-10-07), makes the assumption that it is ok to\n> free a reftable_block pointing to NULL if the size is also set to\n>..\n\neither patch (data or len) LGTM\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n"},{"id":"454379","messageId":"CAFQ2z_OF5+BPxchJcydOhE5tPO2DDeuiJ0-5TzN7kTxD7TpEgA@mail.gmail.com","threadId":"57736","inReplyTo":"xmqq1qxyfh8k.fsf@gitster.g","subject":"Re: [RFC PATCH 2/2] reftable: don't memset() a NULL from failed malloc()","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2022-04-25T10:30:38Z","receivedAt":"2022-04-25T10:31:03Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Fri, Apr 15, 2022 at 6:23 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >> git grep -e 'reftable_[mc]alloc' -e 'reftable_realloc' -A2 origin/master\n> >>\n> >> and I didn't see a single NULL check\n> >\n> > I think you're right, I retrieved that information from wetware. Looking\n> > again I think I was confusing it with the if (x) free(x) fixes in\n> > 34230514b83 (Merge branch 'hn/reftable-coverity-fixes', 2022-02-16).\n>\n> True.  Initially I hoped that these RFC changes gives us a better\n> solution that comes from stepping back and rethinking the issues\n> around the original \"why are we calling memset() with NULL?\", and\n\nmemset with NULL is an oversight.\n\nThe malloc routines are pluggable so the code could be reused for\nlibgit2. However, as use within Git itself is still not imminent, they\ncould just as well be removed as they are just a premature\ngeneralization right now.\n\n> if it were only \"well, in _return_block() functions, we clear the\n> block before calling _free()---that shouldn't be necessary, if the\n> calling custom malloc-free pair cares, they can do the clearing, and\n> plain vanilla free() certainly doesn't, so let's stop calling\n\nThe memset() calls on free() (eg. in  are there to tease out\nuse-after-free bugs in the unittests, but they should probably be\nremoved from file_return_block() as that is production code.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n"},{"id":"454389","messageId":"xmqqilqxvzoa.fsf@gitster.g","threadId":"57736","inReplyTo":"CAFQ2z_O7ytx=AsXuJbYnOdMwT0u03sMP-ZjRRjt4HFn48HT-2g@mail.gmail.com","subject":"Re: [RFC PATCH 1/2] reftable: remove the \"return_block\" abstraction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-25T17:30:13Z","receivedAt":"2022-04-25T17:30:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n> On Fri, Apr 15, 2022 at 12:21 PM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>>\n>> This abstraction added in 1214aa841bc (reftable: add blocksource, an\n>> abstraction for random access reads, 2021-10-07) has the caller\n>> provide a \"blockp->data\", so there's not point in having the vtable\n>> have a custom free() function.\n>>\n>> In addition this had what looked like a poor man's SANITIZE=address\n>> doing a memset() to 0xff just before the data was free'd.\n>\n>>  void reftable_block_done(struct reftable_block *blockp)\n>>  {\n>> -       struct reftable_block_source source = blockp->source;\n>> -       if (blockp && source.ops)\n>> -               source.ops->return_block(source.arg, blockp);\n>> +       FREE_AND_NULL(blockp->data);\n>\n>\n> My thinking here is that we could mmap the reftable file to do reads.\n> In that case, discarding the block would imply decreasing a refcount\n> somewhere, rather than deallocating memory.\n\nSounds like a plan.  As a solution to the memset() thing, ripping\nout this abstraction layer is indeed not just overkill but also\ndoing too much of \"while we are at it\".\n\nLet's take what we've queued on cm/reftable-0-length-memset and\nmerge it down.\n\nThanks.\n"}]}