Re: [PATCH 0/4] use xmalloc in more places
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Apr 11, 2019, 19:14 UTC
- Message-ID
- <20190411191452.GA21290@D-10-18-172-132.dhcp4.washington.edu>
- In-Reply-To
- <20190411134736.GA28543@sigill.intra.peff.net>
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.
Show 7 quoted lines
> 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.cThis (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 ;-).
Show 28 quoted lines
> - 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/