patch, 4 partsuse xmalloc in more places

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(-)

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(-)

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(-)

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(-)

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(-)

Hi Peff,

On Thu, Apr 11, 2019 at 09:47:36AM -0400, Jeff King wrote:

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

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.

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

Thanks, Taylor

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

On Thu, Apr 11, 2019 at 12:14:52PM -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).

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

Hi Peff,

On Thu, Apr 11, 2019 at 03:37:35PM -0400, Jeff King wrote:

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.

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...

Thanks, Taylor

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

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.

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