git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] difftool: avoid strcpy

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Mar 30, 2017, 13:19 UTC
Message-ID
<alpine.DEB.2.20.1703301517260.4068@virtualbox>
In-Reply-To
<20170330103550.vpjrurqho4oz3caa@sigill.intra.peff.net>
Hi Peff,
On Thu, 30 Mar 2017, Jeff King wrote:
Show 24 quoted lines
> 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!
Show 13 quoted lines
> 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

Previous: Jeff King
Message 2 of 2 in “difftool: avoid strcpy”
  1. difftool: avoid strcpyJeff King, Mar 30, 2017
  2. Johannes SchindelinMar 30, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.