{"thread":{"id":"45547","subject":"[PATCH] difftool: avoid strcpy","startedAt":"2017-03-30T10:36:05Z","lastAt":"2017-03-30T13:20:05Z","messageCount":2,"participants":["Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"315763","messageId":"20170330103550.vpjrurqho4oz3caa@sigill.intra.peff.net","threadId":"45547","inReplyTo":null,"subject":"[PATCH] difftool: avoid strcpy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-30T10:35:50Z","receivedAt":"2017-03-30T10:36:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In order to checkout files, difftool reads \"diff --raw\"\noutput and feeds the names to checkout_entry(). That\nfunction requires us to have a \"struct cache_entry\". And\nbecause that struct uses a FLEX_ARRAY for the name field, we\nhave to actually copy in our new name.\n\nThe current code allocates a single re-usable cache_entry\nthat can hold a name up to PATH_MAX, and then copies\nfilenames into it using strcpy(). But there's no guarantee\nthat incoming names are smaller than PATH_MAX. They've come\nfrom \"diff --raw\" output which might be diffing between two\ntrees (and hence we'd be subject to the PATH_MAX of some\nother system, or even none at all if they were created\ndirectly via \"update-index\").\n\nWe can fix this by using make_cache_entry() to create a\ncorrectly-sized cache_entry for each name. This incurs an\nextra allocation per file, but this is negligible compared\nto actually writing out the file contents.\n\nTo make this simpler, we can push this procedure into a new\nhelper function. Note that we can also get rid of the \"len\"\nvariables for src_path and dst_path (and in fact we must, as\nthe compiler complains that they are unused).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI tested this with:\n\n  git init\n  sha1=$(echo whatever | git hash-object -w --stdin)\n  git update-index --add --cacheinfo \\\n    100644 $sha1 $(perl -e 'print join(\"/\", 1..2048)')\n  git difftool -d HEAD\n\nIt fails anyway, of course, because we can't check out a filename of\nthat length, but not until after it has overflowed the buffer.\n\nI'm not sure if we'd want that in the test suite or not, since the\noutcome is unpredictable.\n\n builtin/difftool.c | 31 +++++++++++++++----------------\n 1 file changed, 15 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 25e54ad3e..b350b3d39 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -297,6 +297,19 @@ static char *get_symlink(const struct object_id *oid, const char *path)\n \treturn data;\n }\n \n+static int checkout_path(unsigned mode, struct object_id *oid,\n+\t\t\t const char *path, const struct checkout *state)\n+{\n+\tstruct cache_entry *ce;\n+\tint ret;\n+\n+\tce = make_cache_entry(mode, oid->hash, path, 0, 0);\n+\tret = checkout_entry(ce, state, NULL);\n+\n+\tfree(ce);\n+\treturn ret;\n+}\n+\n static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\tint argc, const char **argv)\n {\n@@ -306,7 +319,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstruct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;\n \tstruct strbuf wtdir = STRBUF_INIT;\n \tsize_t ldir_len, rdir_len, wtdir_len;\n-\tstruct cache_entry *ce = xcalloc(1, sizeof(ce) + PATH_MAX + 1);\n \tconst char *workdir, *tmp;\n \tint ret = 0, i;\n \tFILE *fp;\n@@ -377,7 +389,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\tstruct object_id loid, roid;\n \t\tchar status;\n \t\tconst char *src_path, *dst_path;\n-\t\tsize_t src_path_len, dst_path_len;\n \n \t\tif (starts_with(info.buf, \"::\"))\n \t\t\tdie(N_(\"combined diff formats('-c' and '--cc') are \"\n@@ -390,17 +401,14 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\tif (strbuf_getline_nul(&lpath, fp))\n \t\t\tbreak;\n \t\tsrc_path = lpath.buf;\n-\t\tsrc_path_len = lpath.len;\n \n \t\ti++;\n \t\tif (status != 'C' && status != 'R') {\n \t\t\tdst_path = src_path;\n-\t\t\tdst_path_len = src_path_len;\n \t\t} else {\n \t\t\tif (strbuf_getline_nul(&rpath, fp))\n \t\t\t\tbreak;\n \t\t\tdst_path = rpath.buf;\n-\t\t\tdst_path_len = rpath.len;\n \t\t}\n \n \t\tif (S_ISGITLINK(lmode) || S_ISGITLINK(rmode)) {\n@@ -430,11 +438,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t}\n \n \t\tif (lmode && status != 'C') {\n-\t\t\tce->ce_mode = lmode;\n-\t\t\toidcpy(&ce->oid, &loid);\n-\t\t\tstrcpy(ce->name, src_path);\n-\t\t\tce->ce_namelen = src_path_len;\n-\t\t\tif (checkout_entry(ce, &lstate, NULL))\n+\t\t\tif (checkout_path(lmode, &loid, src_path, &lstate))\n \t\t\t\treturn error(\"could not write '%s'\", src_path);\n \t\t}\n \n@@ -451,11 +455,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\thashmap_add(&working_tree_dups, entry);\n \n \t\t\tif (!use_wt_file(workdir, dst_path, &roid)) {\n-\t\t\t\tce->ce_mode = rmode;\n-\t\t\t\toidcpy(&ce->oid, &roid);\n-\t\t\t\tstrcpy(ce->name, dst_path);\n-\t\t\t\tce->ce_namelen = dst_path_len;\n-\t\t\t\tif (checkout_entry(ce, &rstate, NULL))\n+\t\t\t\tif (checkout_path(rmode, &roid, dst_path, &rstate))\n \t\t\t\t\treturn error(\"could not write '%s'\",\n \t\t\t\t\t\t     dst_path);\n \t\t\t} else if (!is_null_oid(&roid)) {\n@@ -625,7 +625,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\texit_cleanup(tmpdir, rc);\n \n finish:\n-\tfree(ce);\n \tstrbuf_release(&ldir);\n \tstrbuf_release(&rdir);\n \tstrbuf_release(&wtdir);\n-- \n2.12.2.920.g2106709b0\n"},{"id":"315771","messageId":"alpine.DEB.2.20.1703301517260.4068@virtualbox","threadId":"45547","inReplyTo":"20170330103550.vpjrurqho4oz3caa@sigill.intra.peff.net","subject":"Re: [PATCH] difftool: avoid strcpy","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-30T13:19:40Z","receivedAt":"2017-03-30T13:20:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 30 Mar 2017, Jeff King wrote:\n\n> In order to checkout files, difftool reads \"diff --raw\"\n> output and feeds the names to checkout_entry(). That\n> function requires us to have a \"struct cache_entry\". And\n> because that struct uses a FLEX_ARRAY for the name field, we\n> have to actually copy in our new name.\n> \n> The current code allocates a single re-usable cache_entry\n> that can hold a name up to PATH_MAX, and then copies\n> filenames into it using strcpy(). But there's no guarantee\n> that incoming names are smaller than PATH_MAX. They've come\n> from \"diff --raw\" output which might be diffing between two\n> trees (and hence we'd be subject to the PATH_MAX of some\n> other system, or even none at all if they were created\n> directly via \"update-index\").\n> \n> We can fix this by using make_cache_entry() to create a\n> correctly-sized cache_entry for each name. This incurs an\n> extra allocation per file, but this is negligible compared\n> to actually writing out the file contents.\n> \n> To make this simpler, we can push this procedure into a new\n> helper function. Note that we can also get rid of the \"len\"\n> variables for src_path and dst_path (and in fact we must, as\n> the compiler complains that they are unused).\n\nOh woops. Thanks for noticing and for patching it right away!\n\n> I tested this with:\n> \n>   git init\n>   sha1=$(echo whatever | git hash-object -w --stdin)\n>   git update-index --add --cacheinfo \\\n>     100644 $sha1 $(perl -e 'print join(\"/\", 1..2048)')\n>   git difftool -d HEAD\n> \n> It fails anyway, of course, because we can't check out a filename of\n> that length, but not until after it has overflowed the buffer.\n> \n> I'm not sure if we'd want that in the test suite or not, since the\n> outcome is unpredictable.\n\nI'd leave it out. There is really no reliable way to test this.\n\n>  builtin/difftool.c | 31 +++++++++++++++----------------\n>  1 file changed, 15 insertions(+), 16 deletions(-)\n\nNice! I would have thought that it adds to the total number of lines.\nInstead, it removes many fiddly `*_len = *` assignments and makes the code\nmore robust.\n\nACK!\nDscho\n"}]}