Hi Peff,
On Thu, 30 Mar 2017, Jeff King wrote:
> In order to checkout files, difftool reads "diff --raw"
> output and feeds the names to checkout_entry(). That
> function requires us to have a "struct cache_entry". And
> because that struct uses a FLEX_ARRAY for the name field, we
> have to actually copy in our new name.
>
> The current code allocates a single re-usable cache_entry
> that can hold a name up to PATH_MAX, and then copies
> filenames into it using strcpy(). But there's no guarantee
> that incoming names are smaller than PATH_MAX. They've come
> from "diff --raw" output which might be diffing between two
> trees (and hence we'd be subject to the PATH_MAX of some
> other system, or even none at all if they were created
> directly via "update-index").
>
> We can fix this by using make_cache_entry() to create a
> correctly-sized cache_entry for each name. This incurs an
> extra allocation per file, but this is negligible compared
> to actually writing out the file contents.
>
> To make this simpler, we can push this procedure into a new
> helper function. Note that we can also get rid of the "len"
> variables for src_path and dst_path (and in fact we must, as
> the compiler complains that they are unused).
Oh woops. Thanks for noticing and for patching it right away!
> I tested this with:
>
> git init
> sha1=$(echo whatever | git hash-object -w --stdin)
> git update-index --add --cacheinfo \
> 100644 $sha1 $(perl -e 'print join("/", 1..2048)')
> git difftool -d HEAD
>
> It fails anyway, of course, because we can't check out a filename of
> that length, but not until after it has overflowed the buffer.
>
> I'm not sure if we'd want that in the test suite or not, since the
> outcome is unpredictable.
I'd leave it out. There is really no reliable way to test this.
> builtin/difftool.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
Nice! I would have thought that it adds to the total number of lines.
Instead, it removes many fiddly `*_len = *` assignments and makes the code
more robust.
ACK!
Dscho