Re: [PATCH 2/4] avoid computing zero offsets from NULL pointer

From: Jeff King <peff@peff.net>
Date: Jan 27, 2020, 21:19 UTC
Message-ID: <20200127211933.GA3794@coredump.intra.peff.net>
In-Reply-To: <xmqq7e1cbr9w.fsf@gitster-ct.c.googlers.com>

On Mon, Jan 27, 2020 at 12:03:55PM -0800, Junio C Hamano wrote:

Show 24 quoted lines

> > diff --git a/xdiff-interface.c b/xdiff-interface.c
> > index 8509f9ea22..2f1fe48512 100644
> > --- a/xdiff-interface.c
> > +++ b/xdiff-interface.c
> > @@ -84,8 +84,8 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b)
> >  {
> >  	const int blk = 1024;
> >  	long trimmed = 0, recovered = 0;
> > -	char *ap = a->ptr + a->size;
> > -	char *bp = b->ptr + b->size;
> > +	char *ap = a->ptr ? a->ptr + a->size : a->ptr;
> > +	char *bp = b->ptr ? b->ptr + b->size : b->ptr;
> >  	long smaller = (a->size < b->size) ? a->size : b->size;
> >  
> >  	while (blk + trimmed <= smaller && !memcmp(ap - blk, bp - blk, blk)) {
> 
> Isn't it a bug for a->ptr or b->ptr to be NULL here?  Even if we
> manage to assign ap = a->ptr = NULL without complaints, how would
> that memcmp work?
> 
> Is it that the corresponding .size would always be 0 if .ptr is NULL
> that protects us?
> 
> A bit puzzled.

Yes, that's what's happening; all of the cases in this first patch are dealing with "NULL + 0". Which isn't to say somebody couldn't pass in an mmfile_t with NULL and a non-zero size, but obviously that would be a bug. Before my patch that would be a segfault, but afterwards we'd quietly treat it as if the size were zero.

If we want to be more defensive, we could do something like this:

  /* dual inline/macro magic to avoid evaluating ptr twice but knowing
   * enough about the type of *ptr to get the size. */
  #define SAFE_END_PTR(ptr, len) safe_end_ptr(ptr, len, sizeof(*ptr))
  static inline void *safe_end_ptr(void *ptr, size_t nr, size_t elem_size)
  {
	if (!ptr) {
		if (nr)
			BUG("non-zero size coupled with NULL pointer");
		return NULL;
	}
	return (char *)ptr + nr * elem_size;
  }

  ...
  char *ap = SAFE_END_PTR(a->ptr, a->size);

I'm not sure if it's worth it, though.

Yet another alternative is to consider it a bug to use an mmfile_t with a NULL pointer, figure out where that's being set up, and fix it.

As an aside, I also wondered whether we could run into problems with "memcmp(NULL, ..., 0)", which is also undefined behavior. But we don't here because the first half of the while() condition wouldn't trigger.

-Peff