Re: [PATCH] difftool: avoid strcpy

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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