{"thread":{"id":"61284","subject":"[PATCH] reftable: use xmalloc() and xrealloc()","startedAt":"2024-04-06T20:38:07Z","lastAt":"2024-04-09T03:24:08Z","messageCount":7,"participants":["René Scharfe","Patrick Steinhardt","Junio C Hamano","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"492408","messageId":"963961ee-0f1d-42b8-8dda-5838e7a2ed94@web.de","threadId":"61284","inReplyTo":null,"subject":"[PATCH] reftable: use xmalloc() and xrealloc()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-06T20:37:55Z","receivedAt":"2024-04-06T20:38:07Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"malloc(3) and realloc(3) can fail and return NULL. None of the reftable\ncode checks for that possibility and would happily dereference NULL\npointers. Use xmalloc() and xrealloc() instead like in the rest of Git\nto report allocation errors and exit cleanly, and to also honor the\nenvironment variable GIT_ALLOC_LIMIT.\n\nSigned-off-by: René Scharfe \n---\n reftable/publicbasics.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/publicbasics.c b/reftable/publicbasics.c\nindex 44b84a125e..f33a65df34 100644\n--- a/reftable/publicbasics.c\n+++ b/reftable/publicbasics.c\n@@ -19,14 +19,14 @@ void *reftable_malloc(size_t sz)\n {\n \tif (reftable_malloc_ptr)\n \t\treturn (*reftable_malloc_ptr)(sz);\n-\treturn malloc(sz);\n+\treturn xmalloc(sz);\n }\n\n void *reftable_realloc(void *p, size_t sz)\n {\n \tif (reftable_realloc_ptr)\n \t\treturn (*reftable_realloc_ptr)(p, sz);\n-\treturn realloc(p, sz);\n+\treturn xrealloc(p, sz);\n }\n\n void reftable_free(void *p)\n--\n2.44.0\n"},{"id":"492485","messageId":"ZhOETox9FTIOAShN@tanuki","threadId":"61284","inReplyTo":"963961ee-0f1d-42b8-8dda-5838e7a2ed94@web.de","subject":"Re: [PATCH] reftable: use xmalloc() and xrealloc()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T05:44:46Z","receivedAt":"2024-04-08T05:44:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Apr 06, 2024 at 10:37:55PM +0200, René Scharfe wrote:\n> malloc(3) and realloc(3) can fail and return NULL. None of the reftable\n> code checks for that possibility and would happily dereference NULL\n> pointers. Use xmalloc() and xrealloc() instead like in the rest of Git\n> to report allocation errors and exit cleanly, and to also honor the\n> environment variable GIT_ALLOC_LIMIT.\n> \n> Signed-off-by: René Scharfe \n> ---\n> reftable/publicbasics.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/reftable/publicbasics.c b/reftable/publicbasics.c\n> index 44b84a125e..f33a65df34 100644\n> --- a/reftable/publicbasics.c\n> +++ b/reftable/publicbasics.c\n> @@ -19,14 +19,14 @@ void *reftable_malloc(size_t sz)\n> {\n> \tif (reftable_malloc_ptr)\n> \t\treturn (*reftable_malloc_ptr)(sz);\n> -\treturn malloc(sz);\n> +\treturn xmalloc(sz);\n> }\n> \n> void *reftable_realloc(void *p, size_t sz)\n> {\n> \tif (reftable_realloc_ptr)\n> \t\treturn (*reftable_realloc_ptr)(p, sz);\n> -\treturn realloc(p, sz);\n> +\treturn xrealloc(p, sz);\n> }\n> \n> void reftable_free(void *p)\n\nThese are part of the library interfaces that should ideally not be tied\nto the Git code base at all so that they can theoretically be reused by\nanother project like libgit2. So I think that instead of rewriting the\ngeneric fallbacks we should call `reftable_set_alloc()` somewhen early\nin Git's startup code.\n\nIt does raise the question what to do about the generic fallbacks. We\ncould start calling `abort()` when we observe allocation failures. It's\nnot exactly nice behaviour in a library though, where the caller may in\nfact want to handle this case. But it may at least be better than\nfailing on a `NULL` pointer exception somewhere down the road. So it\nmight be the best alternative for now. We could then conver the reftable\nlibrary over time to handle allocation failures and, once that's done,\nwe can eventually drop such a call to `abort()`.\n\nCc'ing Han-Wen's new mail address as he no longer works at Google.\n\nPatrick\n"},{"id":"492544","messageId":"xmqq34rvg8tg.fsf@gitster.g","threadId":"61284","inReplyTo":"ZhOETox9FTIOAShN@tanuki","subject":"Re: [PATCH] reftable: use xmalloc() and xrealloc()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-08T15:42:19Z","receivedAt":"2024-04-08T15:42:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> These are part of the library interfaces that should ideally not be tied\n> to the Git code base at all so that they can theoretically be reused by\n> another project like libgit2. So I think that instead of rewriting the\n> generic fallbacks we should call `reftable_set_alloc()` somewhen early\n> in Git's startup code.\n\nIt sounds like a sensible approach to me on the surface.\n\nThe reftable_subsystem_init() function, which would be the interface\ninto \"reftable library\" from Git side, can call such customization\nfunctions supplied by the library.\n\n> It does raise the question what to do about the generic fallbacks.\n\nGeneric fallbacks would be a plain vanilla malloc(), realloc(), and\nfriends, right?\n\n> We could start calling `abort()` when we observe allocation\n> failures. It's not exactly nice behaviour in a library though,\n> where the caller may in fact want to handle this case.\n\nBut they would not be able to \"handle\" it, unless their substitute\nalloc() function magically finds more memory and never runs out. If\nyou want to allow them to \"handle\" the situation, the library itself\nneeds be prepared to see NULL returned from the allocator, and fail\nthe operation it was doing, and return an error. If the caller asks\nreftable_write_foo(), which may need to allocate some memory to\nfinish its work, it would see NULL and say \"sorry, I cannot\ncontinue\", and return an error to its caller, I would imagine.\n\nI think there are two levels of \"handling\" allocation and its\nfailure. Substituting allocation functions would be a way to solve\nonly one of them (i.e. somehow allow the library client to specify a\nway to supply you an unbounded amount of memory). As long as the\nlibrary is not willing to check allocation failures and propagate\nthe error to the caller, you would have to \"abort\" the operation no\nmatter what before returning the control back to your client, and at\nthat point you would start wanting to make it customizable how to\n\"abort\". Their custom \"abort\" function might do longjmp() to try to\n\"recover\", or simply call die() in our case where Git is the library\nclient, I guess. So reftable_set_alloc() and reftable_set_abort() may\nneed to be there. If you make it mandatory to call them, you can punt\nand make it the responsibility of the library clients to worry about\nerror handling, I guess?\n\nThanks.\n\n\n\n"},{"id":"492561","messageId":"ZhQcYe595pHlQf5j@tanuki","threadId":"61284","inReplyTo":"xmqq34rvg8tg.fsf@gitster.g","subject":"Re: [PATCH] reftable: use xmalloc() and xrealloc()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T16:33:37Z","receivedAt":"2024-04-08T16:33:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 08, 2024 at 08:42:19AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > These are part of the library interfaces that should ideally not be tied\n> > to the Git code base at all so that they can theoretically be reused by\n> > another project like libgit2. So I think that instead of rewriting the\n> > generic fallbacks we should call `reftable_set_alloc()` somewhen early\n> > in Git's startup code.\n> \n> It sounds like a sensible approach to me on the surface.\n> \n> The reftable_subsystem_init() function, which would be the interface\n> into \"reftable library\" from Git side, can call such customization\n> functions supplied by the library.\n> \n> > It does raise the question what to do about the generic fallbacks.\n> \n> Generic fallbacks would be a plain vanilla malloc(), realloc(), and\n> friends, right?\n\nYeah.\n\n> > We could start calling `abort()` when we observe allocation\n> > failures. It's not exactly nice behaviour in a library though,\n> > where the caller may in fact want to handle this case.\n> \n> But they would not be able to \"handle\" it, unless their substitute\n> alloc() function magically finds more memory and never runs out. If\n> you want to allow them to \"handle\" the situation, the library itself\n> needs be prepared to see NULL returned from the allocator, and fail\n> the operation it was doing, and return an error. If the caller asks\n> reftable_write_foo(), which may need to allocate some memory to\n> finish its work, it would see NULL and say \"sorry, I cannot\n> continue\", and return an error to its caller, I would imagine.\n> \n> I think there are two levels of \"handling\" allocation and its\n> failure. Substituting allocation functions would be a way to solve\n> only one of them (i.e. somehow allow the library client to specify a\n> way to supply you an unbounded amount of memory). As long as the\n> library is not willing to check allocation failures and propagate\n> the error to the caller, you would have to \"abort\" the operation no\n> matter what before returning the control back to your client, and at\n> that point you would start wanting to make it customizable how to\n> \"abort\".\n\nI actually think that the reftable library _should_ be willing to check\nfor allocation failures and return proper error codes to the caller.\nThat would be quite an undertaking, but there is no need to do it all in\na single go. We can refactor the code over time to start handling such\nfailures.\n\n> Their custom \"abort\" function might do longjmp() to try to \"recover\",\n> or simply call die() in our case where Git is the library client, I\n> guess. So reftable_set_alloc() and reftable_set_abort() may need to\n> be there. If you make it mandatory to call them, you can punt and\n> make it the responsibility of the library clients to worry about error\n> handling, I guess?\n\nThat would be a possibility indeed. A custom \"failure\" function may try\nto e.g. release caches such that the allocation can be retried. And if\neverything fails then in theory, the caller could do a longjmp(3P).\n\nIn practice this could cause all kinds of problems though. Imagine for\nexample that we have acquired a lockfile and then subsequently an\nallocation fails. If the application were to longjmp(3P) then all the\ncleanup code would not be invoked at all, thus leaving behind a stale\nlockfile.\n\nOverall I think that handling allocation failures is the more flexible\napproach in the long run, even though it requires more work.\n\nPatrick\n"},{"id":"492575","messageId":"CAOw_e7Z9dGeVU399D6o37L3am0abnYUrZnNQEFKhyUv=A2=j8g@mail.gmail.com","threadId":"61284","inReplyTo":"ZhOETox9FTIOAShN@tanuki","subject":"Re: [PATCH] reftable: use xmalloc() and xrealloc()","fromName":"Han-Wen Nienhuys","fromEmail":"hanwenn@gmail.com","sentAt":"2024-04-08T17:50:47Z","receivedAt":"2024-04-08T17:50:59Z","isPatch":true,"sender":{"key":"hanwenn@gmail.com","avatar":"https://gravatar.com/avatar/058832acb8d613baeb6ce9d21b009d9772424a309b9a521330423daef909a27a?d=mp&s=160"},"body":"Op ma 8 apr 2024 07:44 schreef Patrick Steinhardt :\n>\n> It does raise the question what to do about the generic fallbacks. We\n> could start calling `abort()` when we observe allocation failures. It's\n> not exactly nice behaviour in a library though, where the caller may in\n> fact want to handle this case. But it may at least be better than\n> failing on a `NULL` pointer exception somewhere down the road. So it\n> might be the best alternative for now. We could then conver the reftable\n> library over time to handle allocation failures and, once that's done,\n> we can eventually drop such a call to `abort()`.\n\n\nI must admit that I didn't think this part through very much; I\nbelieve someone told me that libgit2 has pluggable memory allocation\nroutines, so I tried to make the malloc pluggable here too. Handling\nOOM better for the malloc calls themselves doesn't seem too difficult,\n\n hanwen@fedora:~/vc/git/reftable$ grep [cme]alloc *c | wc\n 57 276 3469\n\nHowever, it is probably pointless as long as strbuf_* functions do not\nsignal OOM gracefully. There was some talk of libifying strbuf. Did\nthat work include returning OOM error codes in case malloc returns\nnull? A quick look at strbuf.h suggests not.\n\nI would just call xmalloc as default, rather than calling\nreftable_set_alloc, because it might be tricky to ensure it is called\nearly enough.\n"},{"id":"492579","messageId":"xmqqzfu3a8x3.fsf@gitster.g","threadId":"61284","inReplyTo":"CAOw_e7Z9dGeVU399D6o37L3am0abnYUrZnNQEFKhyUv=A2=j8g@mail.gmail.com","subject":"Re: [PATCH] reftable: use xmalloc() and xrealloc()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-08T20:36:40Z","receivedAt":"2024-04-08T20:36:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n> However, it is probably pointless as long as strbuf_* functions do not\n> signal OOM gracefully. There was some talk of libifying strbuf. Did\n> that work include returning OOM error codes in case malloc returns\n> null? A quick look at strbuf.h suggests not.\n\nI would expect not.\n\nThe \"libified\" strbuf (aka \"strbuf API in the Git std lib\") will\nhave to be different from what we internally use from .\n will export gitstdlib_strbuf_addstr(), which is\n\"properly\" libified and signals an allocation failure to its caller.\n\nWhen that happens, I would expect that strbuf_addstr() would be a\nthin wrapper around gitstdlib_strbuf_addstr(), and still just dies\nwith \"we ran out of memory\", i.e.\n\n\t/* strbuf.h */\n\t#include \n\t#include \n\n\tvoid strbuf_addstr(struct strbuf *sb, const char *s)\n\t{\n\t\tint err = gitstdlib_strbuf_addstr(sb, s);\n if (!err)\n\t\t\treturn; /* happy */\n\t\tswitch (err) {\n\t\tcase GITLIB_OOM: /* there may be others */\n\t\t\tdie(\"Out of memory\");\n\t\t\t...\n\t\t}\n\t}\n\nwhich would keep the damage to Git codebase to the minimum when we\nbecome the first client of the \"Git std lib\".\n"},{"id":"492613","messageId":"ZhS000b12DNoQ2pw@tanuki","threadId":"61284","inReplyTo":"CAOw_e7Z9dGeVU399D6o37L3am0abnYUrZnNQEFKhyUv=A2=j8g@mail.gmail.com","subject":"Re: [PATCH] reftable: use xmalloc() and xrealloc()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-09T03:24:03Z","receivedAt":"2024-04-09T03:24:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 08, 2024 at 07:50:47PM +0200, Han-Wen Nienhuys wrote:\n> Op ma 8 apr 2024 07:44 schreef Patrick Steinhardt :\n> >\n> > It does raise the question what to do about the generic fallbacks. We\n> > could start calling `abort()` when we observe allocation failures. It's\n> > not exactly nice behaviour in a library though, where the caller may in\n> > fact want to handle this case. But it may at least be better than\n> > failing on a `NULL` pointer exception somewhere down the road. So it\n> > might be the best alternative for now. We could then conver the reftable\n> > library over time to handle allocation failures and, once that's done,\n> > we can eventually drop such a call to `abort()`.\n> \n> \n> I must admit that I didn't think this part through very much; I\n> believe someone told me that libgit2 has pluggable memory allocation\n> routines, so I tried to make the malloc pluggable here too.\n\nThat was me probably back when I was writing the libgit2 backend.\n\n> Handling OOM better for the malloc calls themselves doesn't seem too\n> difficult,\n> \n> hanwen@fedora:~/vc/git/reftable$ grep [cme]alloc *c | wc\n> 57 276 3469\n> \n> However, it is probably pointless as long as strbuf_* functions do not\n> signal OOM gracefully. There was some talk of libifying strbuf. Did\n> that work include returning OOM error codes in case malloc returns\n> null? A quick look at strbuf.h suggests not.\n\nYeah, `strbuf` also crossed my mind. And there are some other systems\nthat the reftable library calls into, like the tempfiles framework, that\nwould continue to use `xmalloc()` and related functions.\n\n> I would just call xmalloc as default, rather than calling\n> reftable_set_alloc, because it might be tricky to ensure it is called\n> early enough.\n\nI don't think it should be particularly tricky to call\n`reftable_set_alloc()` early enough. The reftable code won't do any\nallocations before we set up the refdb. So calling this in our `main()`\nfunction in \"common-main.c\" should be sufficient.\n\nPatrick\n"}]}