[PATCH 0/4] use xmalloc in more places

[PATCH 0/4] use xmalloc in more places

[Jeff King]

It was reported on the Git security list that there are a few spots
which use a bare malloc() but don't check the return value, which could
dereference NULL. I don't think any of these are exploitable in an
interesting way, beyond Git just segfaulting more or less immediately.

But we should still be handling failures, and I think it makes sense to
be consistent about how we do it (and the other rules which come with
using xmalloc, like GIT_ALLOC_LIMIT).

This series cleans up most of the bare calls found by:

  git grep -E '[^a-z_](m|c|re)alloc\(' '*.c' :^compat :^contrib :^wrapper.c

The calls I've left are:

  - wrapper.c obviously needs to call the real functions :)

  - compat/ has functions emulating libc and system calls, and which are
    expected to return ENOMEM as appropriate

  - diff-delta will gracefully return NULL when trying to delta
    something too large, and pack-objects will skip past trying to find
    a delta. I've never seen this happen in practice, but then I
    primarily use Linux which is more than happy to overcommit on
    malloc(). I've left it unchanged, though possibly we could have an
    xmalloc_gently() if we want to enforce things like GIT_ALLOC_LIMIT
    but still do the graceful fallback thing.

  - test-hash tries to probe malloc() to see how big a buffer it can
    allocate. I doubt this even does anything useful on systems like
    Linux that overcommit. We also don't seem to ever invoke this with a
    buffer larger than 8k in the first place. So it could maybe go away
    entirely, but I left it here.

  [1/4]: test-prio-queue: use xmalloc
  [2/4]: xdiff: use git-compat-util
  [3/4]: xdiff: use xmalloc/xrealloc
  [4/4]: progress: use xmalloc/xcalloc

 progress.c                 | 18 +++++-------------
 t/helper/test-prio-queue.c |  2 +-
 xdiff/xdiff.h              |  4 ++--
 xdiff/xinclude.h           |  8 +-------
 4 files changed, 9 insertions(+), 23 deletions(-)

[PATCH 1/4] test-prio-queue: use xmalloc

[Jeff King]

test-prio-queue.c doesn't check the return value of malloc, and could
segfault.

It's unlikely for this to matter in practice; it's a small allocation,
and this code isn't even installed alongside the rest of Git. But let's
use xmalloc(), which makes auditing for other accidental uses of bare
malloc() easier.

Reported-by: 王健强 <jianqiang.wang@securitygossip.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-prio-queue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/helper/test-prio-queue.c b/t/helper/test-prio-queue.c
index 5bc9c46ea5..f4028442e3 100644
--- a/t/helper/test-prio-queue.c
+++ b/t/helper/test-prio-queue.c
@@ -40,7 +40,7 @@ int cmd__prio_queue(int argc, const char **argv)
 		} else if (!strcmp(*argv, "stack")) {
 			pq.compare = NULL;
 		} else {
-			int *v = malloc(sizeof(*v));
+			int *v = xmalloc(sizeof(*v));
 			*v = atoi(*argv);
 			prio_queue_put(&pq, v);
 		}
-- 
2.21.0.922.g1a559e573c

[PATCH 2/4] xdiff: use git-compat-util

[Jeff King]

Since the xdiff library was not originally part of Git, it does its own
system includes. Let's instead use git-compat-util, which has two
benefits:

  1. It adjusts for any system-specific quirks in how or what we should
     include (though xdiff's needs are light enough that this hasn't
     been a problem in the past).

  2. It lets us use wrapper functions like xmalloc().

Signed-off-by: Jeff King <peff@peff.net>
---
 xdiff/xinclude.h | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/xdiff/xinclude.h b/xdiff/xinclude.h
index f35c4485df..a4285ac0eb 100644
--- a/xdiff/xinclude.h
+++ b/xdiff/xinclude.h
@@ -23,13 +23,7 @@
 #if !defined(XINCLUDE_H)
 #define XINCLUDE_H
 
-#include <ctype.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <unistd.h>
-#include <string.h>
-#include <limits.h>
-
+#include "git-compat-util.h"
 #include "xmacros.h"
 #include "xdiff.h"
 #include "xtypes.h"
-- 
2.21.0.922.g1a559e573c

[PATCH 3/4] xdiff: use xmalloc/xrealloc

[Jeff King]

Most of xdiff uses a bare malloc() to allocate memory, and returns an
error when we get NULL. However, there are a few spots which don't check
the return value and may segfault, including at least xdl_merge() and
xpatience.c's find_longest_common_sequence().

Let's use xmalloc() everywhere instead, so that we get a graceful die()
for these cases, without having to do further auditing. This does mean
the existing cases which check errors will now die() instead of
returning an error up the stack. But:

  - that's how the rest of Git behaves already for malloc errors

  - all of the callers of xdi_diff(), etc, die upon seeing an error

So while we might one day want to fully lib-ify the diff code and make
it possible to use as part of a long-running process, we're not close to
that now. And because we're just tweaking the xdl_malloc() macro here,
we're not really moving ourselves any further away from that. We
could, for example, simplify some of the functions which handle malloc()
errors which can no longer occur. But that would probably be taking us
in the wrong direction.

This also makes our malloc handling more consistent with the rest of
Git, including enforcing GIT_ALLOC_LIMIT and trying to reclaim pack
memory when needed.

Reported-by: 王健强 <jianqiang.wang@securitygossip.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 xdiff/xdiff.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index b158369020..032e3a9f41 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -113,9 +113,9 @@ typedef struct s_bdiffparam {
 } bdiffparam_t;
 
 
-#define xdl_malloc(x) malloc(x)
+#define xdl_malloc(x) xmalloc(x)
 #define xdl_free(ptr) free(ptr)
-#define xdl_realloc(ptr,x) realloc(ptr,x)
+#define xdl_realloc(ptr,x) xrealloc(ptr,x)
 
 void *xdl_mmfile_first(mmfile_t *mmf, long *size);
 long xdl_mmfile_size(mmfile_t *mmf);
-- 
2.21.0.922.g1a559e573c

[PATCH 4/4] progress: use xmalloc/xcalloc

[Jeff King]

Since the early days of Git, the progress code allocates its struct with
a bare malloc(), not xmalloc(). If the allocation fails, we just avoid
showing progress at all.

While perhaps a noble goal not to fail the whole operation because of
optional progress, in practice:

  1. Any failure to allocate a few dozen bytes here means critical path
     allocations are likely to fail, too.

  2. These days we use a strbuf for throughput progress (and there's a
     patch under discussion to do the same for non-throughput cases,
     too). And that uses xmalloc() under the hood, which means we'd
     still die on some allocation failures.

Let's switch to xmalloc(). That makes us consistent with the rest of Git
and makes it easier to audit for other (less careful) bare mallocs.

Signed-off-by: Jeff King <peff@peff.net>
---
This one is obviously less urgent than the others in that it doesn't
trigger a segfault. So this is purely cleanup.

 progress.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/progress.c b/progress.c
index 5a99c9fbf0..699ac33c4f 100644
--- a/progress.c
+++ b/progress.c
@@ -139,12 +139,10 @@ void display_throughput(struct progress *progress, uint64_t total)
 	now_ns = getnanotime();
 
 	if (!tp) {
-		progress->throughput = tp = calloc(1, sizeof(*tp));
-		if (tp) {
-			tp->prev_total = tp->curr_total = total;
-			tp->prev_ns = now_ns;
-			strbuf_init(&tp->display, 0);
-		}
+		progress->throughput = tp = xcalloc(1, sizeof(*tp));
+		tp->prev_total = tp->curr_total = total;
+		tp->prev_ns = now_ns;
+		strbuf_init(&tp->display, 0);
 		return;
 	}
 	tp->curr_total = total;
@@ -196,13 +194,7 @@ int display_progress(struct progress *progress, uint64_t n)
 static struct progress *start_progress_delay(const char *title, uint64_t total,
 					     unsigned delay)
 {
-	struct progress *progress = malloc(sizeof(*progress));
-	if (!progress) {
-		/* unlikely, but here's a good fallback */
-		fprintf(stderr, "%s...\n", title);
-		fflush(stderr);
-		return NULL;
-	}
+	struct progress *progress = xmalloc(sizeof(*progress));
 	progress->title = title;
 	progress->total = total;
 	progress->last_value = -1;
-- 
2.21.0.922.g1a559e573c

Re: [PATCH 0/4] use xmalloc in more places

[Taylor Blau]

Hi Peff,

On Thu, Apr 11, 2019 at 09:47:36AM -0400, Jeff King wrote:
> It was reported on the Git security list that there are a few spots
> which use a bare malloc() but don't check the return value, which could
> dereference NULL. I don't think any of these are exploitable in an
> interesting way, beyond Git just segfaulting more or less immediately.

Good; at least none of these seem to be exploitable for nefarious
purposes. Thanks for posting some patches on the public list.

> But we should still be handling failures, and I think it makes sense to
> be consistent about how we do it (and the other rules which come with
> using xmalloc, like GIT_ALLOC_LIMIT).
>
> This series cleans up most of the bare calls found by:
>
>   git grep -E '[^a-z_](m|c|re)alloc\(' '*.c' :^compat :^contrib :^wrapper.c

This (admittedly pretty awesome) 'git grep' invocation reminds me of a
series I was pretty sure you wrote to ban functions like 'strcpy' and
other obviously bad things.

Some quick searching turned up [1], which landed as f225611d1c
(automatically ban strcpy(), 2018-07-26). Do we want something similar
here? Of course, the locations below would have to be exempt, but it
seems worthwhile (and would save a review cycle in the case that someone
added a 'malloc' in a patch sent here).

FWIW, there isn't any mention of 'malloc' anywhere in that original
thread [1], so I _think_ this would be the first time discussing banning
malloc in this fashion.

> The calls I've left are:
>
>   - wrapper.c obviously needs to call the real functions :)

Yep -- this one needs to stay ;-).

>   - compat/ has functions emulating libc and system calls, and which are
>     expected to return ENOMEM as appropriate
>
>   - diff-delta will gracefully return NULL when trying to delta
>     something too large, and pack-objects will skip past trying to find
>     a delta. I've never seen this happen in practice, but then I
>     primarily use Linux which is more than happy to overcommit on
>     malloc(). I've left it unchanged, though possibly we could have an
>     xmalloc_gently() if we want to enforce things like GIT_ALLOC_LIMIT
>     but still do the graceful fallback thing.
>
>   - test-hash tries to probe malloc() to see how big a buffer it can
>     allocate. I doubt this even does anything useful on systems like
>     Linux that overcommit. We also don't seem to ever invoke this with a
>     buffer larger than 8k in the first place. So it could maybe go away
>     entirely, but I left it here.
>
>   [1/4]: test-prio-queue: use xmalloc
>   [2/4]: xdiff: use git-compat-util
>   [3/4]: xdiff: use xmalloc/xrealloc
>   [4/4]: progress: use xmalloc/xcalloc
>
>  progress.c                 | 18 +++++-------------
>  t/helper/test-prio-queue.c |  2 +-
>  xdiff/xdiff.h              |  4 ++--
>  xdiff/xinclude.h           |  8 +-------
>  4 files changed, 9 insertions(+), 23 deletions(-)
>
Thanks,
Taylor

[1]: https://public-inbox.org/git/20180719203901.GA8079@sigill.intra.peff.net/

Re: [PATCH 0/4] use xmalloc in more places

[Jeff King]

On Thu, Apr 11, 2019 at 12:14:52PM -0700, Taylor Blau wrote:

> > This series cleans up most of the bare calls found by:
> >
> >   git grep -E '[^a-z_](m|c|re)alloc\(' '*.c' :^compat :^contrib :^wrapper.c
> 
> This (admittedly pretty awesome) 'git grep' invocation reminds me of a
> series I was pretty sure you wrote to ban functions like 'strcpy' and
> other obviously bad things.
> 
> Some quick searching turned up [1], which landed as f225611d1c
> (automatically ban strcpy(), 2018-07-26). Do we want something similar
> here? Of course, the locations below would have to be exempt, but it
> seems worthwhile (and would save a review cycle in the case that someone
> added a 'malloc' in a patch sent here).

I don't think we can ban malloc, since we have to use it ourselves. :)

With some contortions, we probably could unban it specifically in
wrapper.c (though note there are a few other calls I've left which would
need to be handled somehow).

Another option would be coccinelle patches to convert malloc() to
xmalloc(), etc (with an exception for the wrappers). I'm not entirely
comfortable with automatic conversion here because there's often some
follow-up adjustments (i.e., we can stop handling allocation errors and
maybe delete some code). I think coccinelle can identify callers and
barf, though.

I'm not sure whether coccinelle saves review cycles (since that implies
people actually run it, though maybe that is better now that it's part
of Travis?). It seems to me that it's usually more helpful for people
periodically doing follow-up auditing.

So I dunno. If this was a common mistake I'd be more concerned with
saving review cycles, but all of the cases I found were actually just
leftovers from the very early days of Git.

-Peff

Re: [PATCH 0/4] use xmalloc in more places

[Taylor Blau]

Hi Peff,

On Thu, Apr 11, 2019 at 03:37:35PM -0400, Jeff King wrote:
> On Thu, Apr 11, 2019 at 12:14:52PM -0700, Taylor Blau wrote:
>
> > > This series cleans up most of the bare calls found by:
> > >
> > >   git grep -E '[^a-z_](m|c|re)alloc\(' '*.c' :^compat :^contrib :^wrapper.c
> >
> > This (admittedly pretty awesome) 'git grep' invocation reminds me of a
> > series I was pretty sure you wrote to ban functions like 'strcpy' and
> > other obviously bad things.
> >
> > Some quick searching turned up [1], which landed as f225611d1c
> > (automatically ban strcpy(), 2018-07-26). Do we want something similar
> > here? Of course, the locations below would have to be exempt, but it
> > seems worthwhile (and would save a review cycle in the case that someone
> > added a 'malloc' in a patch sent here).
>
> I don't think we can ban malloc, since we have to use it ourselves. :)
>
> With some contortions, we probably could unban it specifically in
> wrapper.c (though note there are a few other calls I've left which would
> need to be handled somehow).

Right. I think that I should have made this point clearer in my initial
reply. I was thinking that we could #undef the banned macro in
wrapper.c, or some similar hula-hooping.

> Another option would be coccinelle patches to convert malloc() to
> xmalloc(), etc (with an exception for the wrappers). I'm not entirely
> comfortable with automatic conversion here because there's often some
> follow-up adjustments (i.e., we can stop handling allocation errors and
> maybe delete some code). I think coccinelle can identify callers and
> barf, though.
>
> I'm not sure whether coccinelle saves review cycles (since that implies
> people actually run it, though maybe that is better now that it's part
> of Travis?). It seems to me that it's usually more helpful for people
> periodically doing follow-up auditing.
>
> So I dunno. If this was a common mistake I'd be more concerned with
> saving review cycles, but all of the cases I found were actually just
> leftovers from the very early days of Git.

Yeah... maybe that's the bigger question that I hadn't asked. I made the
suggestion thinking that it would help newcomers avoid writing
'malloc()' and sending it if they didn't know we use our 'xmalloc()'
instead.

But I'm not sure if the argument holds up. I think that in general
exactly the sorts of new-comers that I'm thinking of would have more
than one review cycle anyway, so it might not be worth the effort,
anyway...

> -Peff

Thanks,
Taylor

Re: [PATCH 0/4] use xmalloc in more places

[Jeff King]

On Thu, Apr 11, 2019 at 12:43:08PM -0700, Taylor Blau wrote:

> > I don't think we can ban malloc, since we have to use it ourselves. :)
> >
> > With some contortions, we probably could unban it specifically in
> > wrapper.c (though note there are a few other calls I've left which would
> > need to be handled somehow).
> 
> Right. I think that I should have made this point clearer in my initial
> reply. I was thinking that we could #undef the banned macro in
> wrapper.c, or some similar hula-hooping.

That _probably_ works, but I think technically falls afoul of platforms
on which there's a malloc macro in the first place. We need to not just
#undef it then, but restore the original macro, which is impossible. So
you're better off just not fudging it in the first place. Which would
probably be something like:

  #define SUPPRESS_BAN_MALLOC
  #include "git-compat-util.h"

or something, with the appropriate magic in banned.h.

This might be academic, but you wouldn't know until somebody's platform
subtly breaks. ;) We did run into it already with strcpy(), I think,
hence the defensive #undefs in banned.h.

> Yeah... maybe that's the bigger question that I hadn't asked. I made the
> suggestion thinking that it would help newcomers avoid writing
> 'malloc()' and sending it if they didn't know we use our 'xmalloc()'
> instead.
> 
> But I'm not sure if the argument holds up. I think that in general
> exactly the sorts of new-comers that I'm thinking of would have more
> than one review cycle anyway, so it might not be worth the effort,
> anyway...

I think it's still a reasonable thought, even if I'm not sure the
balance of cost/reward is quite there so far (but might change if it's
an error we see people start to make). Compared to coccinelle, the
banned-function approach is a little nicer for helping new submitters
because it catches the problem during a normal compile (and we know
nobody would ever submit a patch without having at least compiled it,
right? ;) ).

-Peff