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/
