{"thread":{"id":"26673","subject":"[PATCH] unpack-trees.c: check return value of lstat()","startedAt":"2011-03-06T19:13:52Z","lastAt":"2011-03-07T15:59:10Z","messageCount":5,"participants":["Torsten Bögershausen","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"162851","messageId":"201103062013.52793.tboegi@web.de","threadId":"26673","inReplyTo":null,"subject":"[PATCH] unpack-trees.c: check return value of lstat()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2011-03-06T19:13:52Z","receivedAt":"2011-03-06T19:13:52Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"Since commit f66caa \"do not overwrite files in leading path\"\nat one place lstat() was called but the result was not checked.\n\nSigned-off-by: Torsten Bögershausen \n---\n(While I was experimenting with something different, \nthis was found by valgrind)\nPlease review the patch, comments wellcome.\n\n unpack-trees.c | 7 +++----\n 1 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 1ca41b1..42f7aad 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1374,10 +1374,9 @@ static int verify_absent_1(struct cache_entry *ce,\n \t\tchar path[PATH_MAX + 1];\n \t\tmemcpy(path, ce->name, len);\n \t\tpath[len] = 0;\n-\t\tlstat(path, &st);\n-\n-\t\treturn check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,\n-\t\t\t\terror_type, o);\n+\t\tif (!lstat(path, &st))\n+\t\t\treturn check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,\n+\t\t\t error_type, o);\n \t} else if (!lstat(ce->name, &st))\n \t\treturn check_ok_to_remove(ce->name, ce_namelen(ce),\n \t\t\t\tce_to_dtype(ce), ce, &st,\n-- \n1.7.4\n"},{"id":"162852","messageId":"vpqfwr0hwu0.fsf@bauges.imag.fr","threadId":"26673","inReplyTo":"201103062013.52793.tboegi@web.de","subject":"Re: [PATCH] unpack-trees.c: check return value of lstat()","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-03-06T19:29:11Z","receivedAt":"2011-03-06T19:29:11Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Torsten Bögershausen writes:\n\n> +\t\tif (!lstat(path, &st))\n> +\t\t\treturn check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,\n> +\t\t\t error_type, o);\n> \t} else if (!lstat(ce->name, &st))\n\nWhat happens if lstat returns a non-0 value?\n\nAnyway, this seems to have been fixed by a93e53018 (Wed Jan 12 20:28:09\n2011, unpack-trees: handle lstat failure for existing file) already.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"162854","messageId":"4D73E577.1010604@web.de","threadId":"26673","inReplyTo":"vpqfwr0hwu0.fsf@bauges.imag.fr","subject":"Re: [PATCH] unpack-trees.c: check return value of lstat()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2011-03-06T19:50:15Z","receivedAt":"2011-03-06T19:50:15Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 06.03.11 20:29, Matthieu Moy wrote:\n> Torsten Bögershausen writes:\n> \n>> +\t\tif (!lstat(path, &st))\n>> +\t\t\treturn check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,\n>> +\t\t\t error_type, o);\n>> \t} else if (!lstat(ce->name, &st))\n> \n> What happens if lstat returns a non-0 value?\nThe result of \"st\" is 100% garbage, and should not be passed to anybody.\nNot checking the return value (error or not) is not a good thing.\nDevelopers might be tempted to write unreliable code...\n> \n> Anyway, this seems to have been fixed by a93e53018 (Wed Jan 12 20:28:09\n> 2011, unpack-trees: handle lstat failure for existing file) already.\n> \nHm,\nv1.7.4 says:\ncommit e39212ab08e8d37dda5d8fd32b54099fe01dbbdb\nMerge: 716958c 9e08273\n(so far so good)\nMy git looks like this:\n\nstatic int verify_absent_1(struct cache_entry *ce,\n\t\t\t\t enum unpack_trees_error_types error_type,\n\t\t\t\t struct unpack_trees_options *o)\n{\n[snip]\n\telse if (len > 0) {\n\t\tchar path[PATH_MAX + 1];\n\t\tmemcpy(path, ce->name, len);\n\t\tpath[len] = 0;\n\t\tlstat(path, &st);\n ^^^^^^^^^^^^^^^^^^\n\t\treturn check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,\n\t\t\t\terror_type, o);\n\n\n(And I had to correct drizzd@aon.a -> drizzd@aon.at)\n/Torsten\n"},{"id":"162858","messageId":"vpqpqq4ge71.fsf@bauges.imag.fr","threadId":"26673","inReplyTo":"4D73E577.1010604@web.de","subject":"Re: [PATCH] unpack-trees.c: check return value of lstat()","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-03-06T20:57:06Z","receivedAt":"2011-03-06T20:57:06Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Torsten Bögershausen writes:\n\n> On 06.03.11 20:29, Matthieu Moy wrote:\n>> Torsten Bögershausen writes:\n>> \n>>> +\t\tif (!lstat(path, &st))\n>>> +\t\t\treturn check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,\n>>> +\t\t\t error_type, o);\n>>> \t} else if (!lstat(ce->name, &st))\n>> \n>> What happens if lstat returns a non-0 value?\n> The result of \"st\" is 100% garbage, and should not be passed to\n> anybody.\n\nWith your code, you don't do a return, you'll reach the end of the\nfunction without calling return, which is probably the worst thing you\ncould expect.\n\n> v1.7.4 says:\n> commit e39212ab08e8d37dda5d8fd32b54099fe01dbbdb\n> Merge: 716958c 9e08273\n\nYou've cut the date:\n\ncommit e39212ab08e8d37dda5d8fd32b54099fe01dbbdb\nMerge: 716958c 9e08273\nAuthor: Junio C Hamano \nDate: Wed Dec 22 14:40:26 2010 -0800\n\n=> no big surprise that you don't see the bugfix from Jan 12th.\n\n>> Anyway, this seems to have been fixed by a93e53018 (Wed Jan 12 20:28:09\n>> 2011, unpack-trees: handle lstat failure for existing file) already.\n\nDid you look at the content of this commit?\n\n> (And I had to correct drizzd@aon.a -> drizzd@aon.at)\n\nYes, because you've mis-spelled it in the first place and I didn't fix\nit ;-).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"162920","messageId":"4D7500CE.8000106@web.de","threadId":"26673","inReplyTo":"vpqpqq4ge71.fsf@bauges.imag.fr","subject":"Re: [PATCH] unpack-trees.c: check return value of lstat()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2011-03-07T15:59:10Z","receivedAt":"2011-03-07T15:59:10Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 06.03.11 21:57, Matthieu Moy wrote:\n> Torsten Bögershausen writes:\n> \n>> On 06.03.11 20:29, Matthieu Moy wrote:\n>>> Torsten Bögershausen writes:\n>>>\n>>>> +\t\tif (!lstat(path, &st))\n>>>> +\t\t\treturn check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st,\n>>>> +\t\t\t error_type, o);\n>>>> \t} else if (!lstat(ce->name, &st))\n>>>\n>>> What happens if lstat returns a non-0 value?\n>> The result of \"st\" is 100% garbage, and should not be passed to\n>> anybody.\n> \n> With your code, you don't do a return, you'll reach the end of the\n> function without calling return, which is probably the worst thing you\n> could expect.\n> \n>> v1.7.4 says:\n>> commit e39212ab08e8d37dda5d8fd32b54099fe01dbbdb\n>> Merge: 716958c 9e08273\n> \n> You've cut the date:\n> \n> commit e39212ab08e8d37dda5d8fd32b54099fe01dbbdb\n> Merge: 716958c 9e08273\n> Author: Junio C Hamano \n> Date: Wed Dec 22 14:40:26 2010 -0800\n> \n> => no big surprise that you don't see the bugfix from Jan 12th.\n> \n>>> Anyway, this seems to have been fixed by a93e53018 (Wed Jan 12 20:28:09\n>>> 2011, unpack-trees: handle lstat failure for existing file) already.\n> \n> Did you look at the content of this commit?\n> \n>> (And I had to correct drizzd@aon.a -> drizzd@aon.at)\n> \n> Yes, because you've mis-spelled it in the first place and I didn't fix\n> it ;-).\n> \n\nOjojo,\nWhile working with the best tracking tool, I lost tracking myself.\nThat's why I missed Jonathans fix and learned today 9e08273 != a93e53018\nThanks for the patience, (and sorry for the noise)\n/Torsten\n"}]}