Re: [PATCH 2/6] diff: clear emitted_symbols flag after use

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 24, 2019, 20:18 UTC
Message-ID: <xmqqy379hkri.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190124123240.GB11354@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 11 quoted lines

> When we run "git log --cc --stat -p --color-moved" starting at D, we get
> this sequence of events:
>
>   1. The diff for D is using -p, so diff_flush() calls into
>      diff_flush_patch_all_file_pairs(). There we see that o->color_moved
>      is in effect, so we point o->emitted_symbols to a static local
>      struct, causing diff_flush_patch() to queue the symbols instead of
>      actually writing them out.
>
>      We then do our move detection, emit the symbols, and clear the
>      struct. But we leave o->emitted_symbols pointing to our struct.

Wow, that was nasty.

I did not like the complexity of that "emitted symbols" conversion we had to do recently and never trusted the code. There still is something funny in diff_flush_patch_all_file_pairs() even after this patch, though.

 - We first check o->color_moved and unconditionally point
   o->emitted_symbols to &esm.

 - In an if() block we enter when o->emitted_symbols is set, there
   is a check to see if o->color_moved is set.  This makes sense
   only if we are trying to be prepared to handle a case where we
   are not the one that assigned a non-NULL to o->emitted_symbols
   due to o->color_moved.  So it certainly is possible that
   o->emitted_symbols is set before we enter this function.

 - But then, it means that o->emitted_symbols we may have had
   non-NULL when the function is called may be overwritten if
   o->color_moved is set.

The above observation does not necessarily indicate any bug; it just shows that the code structure is messier than necessary.

Show 5 quoted lines

> To fix it, we can simply restore o->emitted_symbols to NULL after
> flushing it, so that it does not affect anything outside of
> diff_flush_patch_all_file_pairs(). This intuitively makes sense, since
> nobody outside of that function is going to bother flushing it, so we
> would not want them to write to it either.

Perhaps. I see word-diff codepath gives an allocated buffer to o->emitted_symbols, so assigning NULL without freeing would mean a leak, but I guess this helper function is not designed to be called