Re: Memory corruption when rebasing with git version 1.8.1.5 on arch

From: Jeff King <peff@peff.net>
Date: Mar 22, 2013, 21:24 UTC
Message-ID: <20130322212421.GA5079@sigill.intra.peff.net>
In-Reply-To: <7vip4j71dw.fsf@alter.siamese.dyndns.org>

On Fri, Mar 22, 2013 at 11:08:59AM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> This may be sufficient.  In the olden days, we relied on that all
> whitespace fixing rules made the result shorter and took advantage
> of it in update-pre-post-images to rewrite the images in place.  The
> oddball tab-in-indent (aka Python), however, can grow the result by
> expanding tabs (deemed "incorrect") in the input into runs of spaces
> (deemed "kosher").
> 
> Fortunately, we already support its more generalized form "match
> while ignoring whitespace differences" that can lengthen the result;
> as long as we correctly count the number of bytes needed to hold
> rewritten postimage, the existing logic in update_pre_post_images
> should be able to do the rest for us.

Yeah, your patch looks right to me. I do wonder if the optimization here:

Show 9 quoted lines

> @@ -2399,8 +2401,10 @@ static int match_fragment(struct image *img,
>  	 * hunk match.  Update the context lines in the postimage.
>  	 */
>  	fixed_buf = strbuf_detach(&fixed, &fixed_len);
> +	if (postlen < postimage->len)
> +		postlen = 0;
>  	update_pre_post_images(preimage, postimage,
> -			       fixed_buf, fixed_len, 0);
> +			       fixed_buf, fixed_len, postlen);

should simply go into update_pre_post_images (i.e., let it decide on whether to do it inline or with a new allocation, rather than making postlen==0 special). That would let the ignore-whitespace code path use the optimization, too (when it's possible).

By the way, I notice that when update_pre_post_images does allocate, the old value of postimage->buf is lost. It looks like that is not leaked, because it was pointing to a strbuf (newlines in apply_one_fragment) that we are going to release anyway afterwards. But that means nobody is freeing postimage->buf, which means that our newly malloc'd version is getting leaked.

-Peff