Web lists-archives.com

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




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@xxxxxxxxxxxxxxxxxxxxx/