{"thread":{"id":"50919","subject":"[PATCH 0/4] use xmalloc in more places","startedAt":"2019-04-11T13:47:39Z","lastAt":"2019-04-11T20:04:55Z","messageCount":9,"participants":["Jeff King","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"373652","messageId":"20190411134736.GA28543@sigill.intra.peff.net","threadId":"50919","inReplyTo":null,"subject":"[PATCH 0/4] use xmalloc in more places","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-11T13:47:36Z","receivedAt":"2019-04-11T13:47:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It was reported on the Git security list that there are a few spots\nwhich use a bare malloc() but don't check the return value, which could\ndereference NULL. I don't think any of these are exploitable in an\ninteresting way, beyond Git just segfaulting more or less immediately.\n\nBut we should still be handling failures, and I think it makes sense to\nbe consistent about how we do it (and the other rules which come with\nusing xmalloc, like GIT_ALLOC_LIMIT).\n\nThis series cleans up most of the bare calls found by:\n\n git grep -E '[^a-z_](m|c|re)alloc\\(' '*.c' :^compat :^contrib :^wrapper.c\n\nThe calls I've left are:\n\n - wrapper.c obviously needs to call the real functions :)\n\n - compat/ has functions emulating libc and system calls, and which are\n expected to return ENOMEM as appropriate\n\n - diff-delta will gracefully return NULL when trying to delta\n something too large, and pack-objects will skip past trying to find\n a delta. I've never seen this happen in practice, but then I\n primarily use Linux which is more than happy to overcommit on\n malloc(). I've left it unchanged, though possibly we could have an\n xmalloc_gently() if we want to enforce things like GIT_ALLOC_LIMIT\n but still do the graceful fallback thing.\n\n - test-hash tries to probe malloc() to see how big a buffer it can\n allocate. I doubt this even does anything useful on systems like\n Linux that overcommit. We also don't seem to ever invoke this with a\n buffer larger than 8k in the first place. So it could maybe go away\n entirely, but I left it here.\n\n [1/4]: test-prio-queue: use xmalloc\n [2/4]: xdiff: use git-compat-util\n [3/4]: xdiff: use xmalloc/xrealloc\n [4/4]: progress: use xmalloc/xcalloc\n\n progress.c | 18 +++++-------------\n t/helper/test-prio-queue.c | 2 +-\n xdiff/xdiff.h | 4 ++--\n xdiff/xinclude.h | 8 +-------\n 4 files changed, 9 insertions(+), 23 deletions(-)\n\n"},{"id":"373653","messageId":"20190411134814.GA9182@sigill.intra.peff.net","threadId":"50919","inReplyTo":"20190411134736.GA28543@sigill.intra.peff.net","subject":"[PATCH 1/4] test-prio-queue: use xmalloc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-11T13:48:14Z","receivedAt":"2019-04-11T13:48:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"test-prio-queue.c doesn't check the return value of malloc, and could\nsegfault.\n\nIt's unlikely for this to matter in practice; it's a small allocation,\nand this code isn't even installed alongside the rest of Git. But let's\nuse xmalloc(), which makes auditing for other accidental uses of bare\nmalloc() easier.\n\nReported-by: 王健强 \nSigned-off-by: Jeff King \n---\n t/helper/test-prio-queue.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-prio-queue.c b/t/helper/test-prio-queue.c\nindex 5bc9c46ea5..f4028442e3 100644\n--- a/t/helper/test-prio-queue.c\n+++ b/t/helper/test-prio-queue.c\n@@ -40,7 +40,7 @@ int cmd__prio_queue(int argc, const char **argv)\n \t\t} else if (!strcmp(*argv, \"stack\")) {\n \t\t\tpq.compare = NULL;\n \t\t} else {\n-\t\t\tint *v = malloc(sizeof(*v));\n+\t\t\tint *v = xmalloc(sizeof(*v));\n \t\t\t*v = atoi(*argv);\n \t\t\tprio_queue_put(&pq, v);\n \t\t}\n-- \n2.21.0.922.g1a559e573c\n\n"},{"id":"373654","messageId":"20190411134832.GB9182@sigill.intra.peff.net","threadId":"50919","inReplyTo":"20190411134736.GA28543@sigill.intra.peff.net","subject":"[PATCH 2/4] xdiff: use git-compat-util","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-11T13:48:33Z","receivedAt":"2019-04-11T13:48:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since the xdiff library was not originally part of Git, it does its own\nsystem includes. Let's instead use git-compat-util, which has two\nbenefits:\n\n 1. It adjusts for any system-specific quirks in how or what we should\n include (though xdiff's needs are light enough that this hasn't\n been a problem in the past).\n\n 2. It lets us use wrapper functions like xmalloc().\n\nSigned-off-by: Jeff King \n---\n xdiff/xinclude.h | 8 +-------\n 1 file changed, 1 insertion(+), 7 deletions(-)\n\ndiff --git a/xdiff/xinclude.h b/xdiff/xinclude.h\nindex f35c4485df..a4285ac0eb 100644\n--- a/xdiff/xinclude.h\n+++ b/xdiff/xinclude.h\n@@ -23,13 +23,7 @@\n #if !defined(XINCLUDE_H)\n #define XINCLUDE_H\n \n-#include \n-#include \n-#include \n-#include \n-#include \n-#include \n-\n+#include \"git-compat-util.h\"\n #include \"xmacros.h\"\n #include \"xdiff.h\"\n #include \"xtypes.h\"\n-- \n2.21.0.922.g1a559e573c\n\n"},{"id":"373655","messageId":"20190411134925.GC9182@sigill.intra.peff.net","threadId":"50919","inReplyTo":"20190411134736.GA28543@sigill.intra.peff.net","subject":"[PATCH 3/4] xdiff: use xmalloc/xrealloc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-11T13:49:25Z","receivedAt":"2019-04-11T13:49:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Most of xdiff uses a bare malloc() to allocate memory, and returns an\nerror when we get NULL. However, there are a few spots which don't check\nthe return value and may segfault, including at least xdl_merge() and\nxpatience.c's find_longest_common_sequence().\n\nLet's use xmalloc() everywhere instead, so that we get a graceful die()\nfor these cases, without having to do further auditing. This does mean\nthe existing cases which check errors will now die() instead of\nreturning an error up the stack. But:\n\n - that's how the rest of Git behaves already for malloc errors\n\n - all of the callers of xdi_diff(), etc, die upon seeing an error\n\nSo while we might one day want to fully lib-ify the diff code and make\nit possible to use as part of a long-running process, we're not close to\nthat now. And because we're just tweaking the xdl_malloc() macro here,\nwe're not really moving ourselves any further away from that. We\ncould, for example, simplify some of the functions which handle malloc()\nerrors which can no longer occur. But that would probably be taking us\nin the wrong direction.\n\nThis also makes our malloc handling more consistent with the rest of\nGit, including enforcing GIT_ALLOC_LIMIT and trying to reclaim pack\nmemory when needed.\n\nReported-by: 王健强 \nSigned-off-by: Jeff King \n---\n xdiff/xdiff.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex b158369020..032e3a9f41 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -113,9 +113,9 @@ typedef struct s_bdiffparam {\n } bdiffparam_t;\n \n \n-#define xdl_malloc(x) malloc(x)\n+#define xdl_malloc(x) xmalloc(x)\n #define xdl_free(ptr) free(ptr)\n-#define xdl_realloc(ptr,x) realloc(ptr,x)\n+#define xdl_realloc(ptr,x) xrealloc(ptr,x)\n \n void *xdl_mmfile_first(mmfile_t *mmf, long *size);\n long xdl_mmfile_size(mmfile_t *mmf);\n-- \n2.21.0.922.g1a559e573c\n\n"},{"id":"373656","messageId":"20190411134957.GD9182@sigill.intra.peff.net","threadId":"50919","inReplyTo":"20190411134736.GA28543@sigill.intra.peff.net","subject":"[PATCH 4/4] progress: use xmalloc/xcalloc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-11T13:49:57Z","receivedAt":"2019-04-11T13:50:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since the early days of Git, the progress code allocates its struct with\na bare malloc(), not xmalloc(). If the allocation fails, we just avoid\nshowing progress at all.\n\nWhile perhaps a noble goal not to fail the whole operation because of\noptional progress, in practice:\n\n 1. Any failure to allocate a few dozen bytes here means critical path\n allocations are likely to fail, too.\n\n 2. These days we use a strbuf for throughput progress (and there's a\n patch under discussion to do the same for non-throughput cases,\n too). And that uses xmalloc() under the hood, which means we'd\n still die on some allocation failures.\n\nLet's switch to xmalloc(). That makes us consistent with the rest of Git\nand makes it easier to audit for other (less careful) bare mallocs.\n\nSigned-off-by: Jeff King \n---\nThis one is obviously less urgent than the others in that it doesn't\ntrigger a segfault. So this is purely cleanup.\n\n progress.c | 18 +++++-------------\n 1 file changed, 5 insertions(+), 13 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 5a99c9fbf0..699ac33c4f 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -139,12 +139,10 @@ void display_throughput(struct progress *progress, uint64_t total)\n \tnow_ns = getnanotime();\n \n \tif (!tp) {\n-\t\tprogress->throughput = tp = calloc(1, sizeof(*tp));\n-\t\tif (tp) {\n-\t\t\ttp->prev_total = tp->curr_total = total;\n-\t\t\ttp->prev_ns = now_ns;\n-\t\t\tstrbuf_init(&tp->display, 0);\n-\t\t}\n+\t\tprogress->throughput = tp = xcalloc(1, sizeof(*tp));\n+\t\ttp->prev_total = tp->curr_total = total;\n+\t\ttp->prev_ns = now_ns;\n+\t\tstrbuf_init(&tp->display, 0);\n \t\treturn;\n \t}\n \ttp->curr_total = total;\n@@ -196,13 +194,7 @@ int display_progress(struct progress *progress, uint64_t n)\n static struct progress *start_progress_delay(const char *title, uint64_t total,\n \t\t\t\t\t unsigned delay)\n {\n-\tstruct progress *progress = malloc(sizeof(*progress));\n-\tif (!progress) {\n-\t\t/* unlikely, but here's a good fallback */\n-\t\tfprintf(stderr, \"%s...\\n\", title);\n-\t\tfflush(stderr);\n-\t\treturn NULL;\n-\t}\n+\tstruct progress *progress = xmalloc(sizeof(*progress));\n \tprogress->title = title;\n \tprogress->total = total;\n \tprogress->last_value = -1;\n-- \n2.21.0.922.g1a559e573c\n"},{"id":"373677","messageId":"20190411191452.GA21290@D-10-18-172-132.dhcp4.washington.edu","threadId":"50919","inReplyTo":"20190411134736.GA28543@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] use xmalloc in more places","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-11T19:14:52Z","receivedAt":"2019-04-11T19:14:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Peff,\n\nOn Thu, Apr 11, 2019 at 09:47:36AM -0400, Jeff King wrote:\n> It was reported on the Git security list that there are a few spots\n> which use a bare malloc() but don't check the return value, which could\n> dereference NULL. I don't think any of these are exploitable in an\n> interesting way, beyond Git just segfaulting more or less immediately.\n\nGood; at least none of these seem to be exploitable for nefarious\npurposes. Thanks for posting some patches on the public list.\n\n> But we should still be handling failures, and I think it makes sense to\n> be consistent about how we do it (and the other rules which come with\n> using xmalloc, like GIT_ALLOC_LIMIT).\n>\n> This series cleans up most of the bare calls found by:\n>\n> git grep -E '[^a-z_](m|c|re)alloc\\(' '*.c' :^compat :^contrib :^wrapper.c\n\nThis (admittedly pretty awesome) 'git grep' invocation reminds me of a\nseries I was pretty sure you wrote to ban functions like 'strcpy' and\nother obviously bad things.\n\nSome quick searching turned up [1], which landed as f225611d1c\n(automatically ban strcpy(), 2018-07-26). Do we want something similar\nhere? Of course, the locations below would have to be exempt, but it\nseems worthwhile (and would save a review cycle in the case that someone\nadded a 'malloc' in a patch sent here).\n\nFWIW, there isn't any mention of 'malloc' anywhere in that original\nthread [1], so I _think_ this would be the first time discussing banning\nmalloc in this fashion.\n\n> The calls I've left are:\n>\n> - wrapper.c obviously needs to call the real functions :)\n\nYep -- this one needs to stay ;-).\n\n> - compat/ has functions emulating libc and system calls, and which are\n> expected to return ENOMEM as appropriate\n>\n> - diff-delta will gracefully return NULL when trying to delta\n> something too large, and pack-objects will skip past trying to find\n> a delta. I've never seen this happen in practice, but then I\n> primarily use Linux which is more than happy to overcommit on\n> malloc(). I've left it unchanged, though possibly we could have an\n> xmalloc_gently() if we want to enforce things like GIT_ALLOC_LIMIT\n> but still do the graceful fallback thing.\n>\n> - test-hash tries to probe malloc() to see how big a buffer it can\n> allocate. I doubt this even does anything useful on systems like\n> Linux that overcommit. We also don't seem to ever invoke this with a\n> buffer larger than 8k in the first place. So it could maybe go away\n> entirely, but I left it here.\n>\n> [1/4]: test-prio-queue: use xmalloc\n> [2/4]: xdiff: use git-compat-util\n> [3/4]: xdiff: use xmalloc/xrealloc\n> [4/4]: progress: use xmalloc/xcalloc\n>\n> progress.c | 18 +++++-------------\n> t/helper/test-prio-queue.c | 2 +-\n> xdiff/xdiff.h | 4 ++--\n> xdiff/xinclude.h | 8 +-------\n> 4 files changed, 9 insertions(+), 23 deletions(-)\n>\nThanks,\nTaylor\n\n[1]: https://public-inbox.org/git/20180719203901.GA8079@sigill.intra.peff.net/\n"},{"id":"373678","messageId":"20190411193735.GB32528@sigill.intra.peff.net","threadId":"50919","inReplyTo":"20190411191452.GA21290@D-10-18-172-132.dhcp4.washington.edu","subject":"Re: [PATCH 0/4] use xmalloc in more places","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-11T19:37:35Z","receivedAt":"2019-04-11T19:37:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 11, 2019 at 12:14:52PM -0700, Taylor Blau wrote:\n\n> > This series cleans up most of the bare calls found by:\n> >\n> > git grep -E '[^a-z_](m|c|re)alloc\\(' '*.c' :^compat :^contrib :^wrapper.c\n> \n> This (admittedly pretty awesome) 'git grep' invocation reminds me of a\n> series I was pretty sure you wrote to ban functions like 'strcpy' and\n> other obviously bad things.\n> \n> Some quick searching turned up [1], which landed as f225611d1c\n> (automatically ban strcpy(), 2018-07-26). Do we want something similar\n> here? Of course, the locations below would have to be exempt, but it\n> seems worthwhile (and would save a review cycle in the case that someone\n> added a 'malloc' in a patch sent here).\n\nI don't think we can ban malloc, since we have to use it ourselves. :)\n\nWith some contortions, we probably could unban it specifically in\nwrapper.c (though note there are a few other calls I've left which would\nneed to be handled somehow).\n\nAnother option would be coccinelle patches to convert malloc() to\nxmalloc(), etc (with an exception for the wrappers). I'm not entirely\ncomfortable with automatic conversion here because there's often some\nfollow-up adjustments (i.e., we can stop handling allocation errors and\nmaybe delete some code). I think coccinelle can identify callers and\nbarf, though.\n\nI'm not sure whether coccinelle saves review cycles (since that implies\npeople actually run it, though maybe that is better now that it's part\nof Travis?). It seems to me that it's usually more helpful for people\nperiodically doing follow-up auditing.\n\nSo I dunno. If this was a common mistake I'd be more concerned with\nsaving review cycles, but all of the cases I found were actually just\nleftovers from the very early days of Git.\n\n-Peff\n"},{"id":"373679","messageId":"20190411194308.GA26327@D-10-18-172-132.dhcp4.washington.edu","threadId":"50919","inReplyTo":"20190411193735.GB32528@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] use xmalloc in more places","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-04-11T19:43:08Z","receivedAt":"2019-04-11T19:43:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Peff,\n\nOn Thu, Apr 11, 2019 at 03:37:35PM -0400, Jeff King wrote:\n> On Thu, Apr 11, 2019 at 12:14:52PM -0700, Taylor Blau wrote:\n>\n> > > This series cleans up most of the bare calls found by:\n> > >\n> > > git grep -E '[^a-z_](m|c|re)alloc\\(' '*.c' :^compat :^contrib :^wrapper.c\n> >\n> > This (admittedly pretty awesome) 'git grep' invocation reminds me of a\n> > series I was pretty sure you wrote to ban functions like 'strcpy' and\n> > other obviously bad things.\n> >\n> > Some quick searching turned up [1], which landed as f225611d1c\n> > (automatically ban strcpy(), 2018-07-26). Do we want something similar\n> > here? Of course, the locations below would have to be exempt, but it\n> > seems worthwhile (and would save a review cycle in the case that someone\n> > added a 'malloc' in a patch sent here).\n>\n> I don't think we can ban malloc, since we have to use it ourselves. :)\n>\n> With some contortions, we probably could unban it specifically in\n> wrapper.c (though note there are a few other calls I've left which would\n> need to be handled somehow).\n\nRight. I think that I should have made this point clearer in my initial\nreply. I was thinking that we could #undef the banned macro in\nwrapper.c, or some similar hula-hooping.\n\n> Another option would be coccinelle patches to convert malloc() to\n> xmalloc(), etc (with an exception for the wrappers). I'm not entirely\n> comfortable with automatic conversion here because there's often some\n> follow-up adjustments (i.e., we can stop handling allocation errors and\n> maybe delete some code). I think coccinelle can identify callers and\n> barf, though.\n>\n> I'm not sure whether coccinelle saves review cycles (since that implies\n> people actually run it, though maybe that is better now that it's part\n> of Travis?). It seems to me that it's usually more helpful for people\n> periodically doing follow-up auditing.\n>\n> So I dunno. If this was a common mistake I'd be more concerned with\n> saving review cycles, but all of the cases I found were actually just\n> leftovers from the very early days of Git.\n\nYeah... maybe that's the bigger question that I hadn't asked. I made the\nsuggestion thinking that it would help newcomers avoid writing\n'malloc()' and sending it if they didn't know we use our 'xmalloc()'\ninstead.\n\nBut I'm not sure if the argument holds up. I think that in general\nexactly the sorts of new-comers that I'm thinking of would have more\nthan one review cycle anyway, so it might not be worth the effort,\nanyway...\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"373681","messageId":"20190411200452.GA19315@sigill.intra.peff.net","threadId":"50919","inReplyTo":"20190411194308.GA26327@D-10-18-172-132.dhcp4.washington.edu","subject":"Re: [PATCH 0/4] use xmalloc in more places","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-11T20:04:52Z","receivedAt":"2019-04-11T20:04:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 11, 2019 at 12:43:08PM -0700, Taylor Blau wrote:\n\n> > I don't think we can ban malloc, since we have to use it ourselves. :)\n> >\n> > With some contortions, we probably could unban it specifically in\n> > wrapper.c (though note there are a few other calls I've left which would\n> > need to be handled somehow).\n> \n> Right. I think that I should have made this point clearer in my initial\n> reply. I was thinking that we could #undef the banned macro in\n> wrapper.c, or some similar hula-hooping.\n\nThat _probably_ works, but I think technically falls afoul of platforms\non which there's a malloc macro in the first place. We need to not just\n#undef it then, but restore the original macro, which is impossible. So\nyou're better off just not fudging it in the first place. Which would\nprobably be something like:\n\n #define SUPPRESS_BAN_MALLOC\n #include \"git-compat-util.h\"\n\nor something, with the appropriate magic in banned.h.\n\nThis might be academic, but you wouldn't know until somebody's platform\nsubtly breaks. ;) We did run into it already with strcpy(), I think,\nhence the defensive #undefs in banned.h.\n\n> Yeah... maybe that's the bigger question that I hadn't asked. I made the\n> suggestion thinking that it would help newcomers avoid writing\n> 'malloc()' and sending it if they didn't know we use our 'xmalloc()'\n> instead.\n> \n> But I'm not sure if the argument holds up. I think that in general\n> exactly the sorts of new-comers that I'm thinking of would have more\n> than one review cycle anyway, so it might not be worth the effort,\n> anyway...\n\nI think it's still a reasonable thought, even if I'm not sure the\nbalance of cost/reward is quite there so far (but might change if it's\nan error we see people start to make). Compared to coccinelle, the\nbanned-function approach is a little nicer for helping new submitters\nbecause it catches the problem during a normal compile (and we know\nnobody would ever submit a patch without having at least compiled it,\nright? ;) ).\n\n-Peff\n"}]}