{"thread":{"id":"50528","subject":"[BUG] ls-files showing deleted files (unchecked lstat return value)","startedAt":"2019-02-17T13:59:36Z","lastAt":"2019-02-20T22:51:56Z","messageCount":6,"participants":["Joe Ranieri","Randall S. Becker","SZEDER Gábor","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"369512","messageId":"1ff48830-f3de-11f4-9014-c20eb3890c21@grammatech.com","threadId":"50528","inReplyTo":null,"subject":"[BUG] ls-files showing deleted files (unchecked lstat return value)","fromName":"Joe Ranieri","fromEmail":"jranieri@grammatech.com","sentAt":"2019-02-17T13:49:39Z","receivedAt":"2019-02-17T13:59:36Z","isPatch":false,"sender":{"key":"jranieri@grammatech.com","avatar":null},"body":"\"git ls-files -m\" can show deleted files, despite -d not having been \nspecified. This is due to ls-files.c's show_files function calling lstat \nbut not checking the return value before calling ie_modified with the \nuninitialized stat structure.\n\nThis problem was found using the static analysis tool GrammaTech CodeSonar.\n\n-- \nThanks and Regards,\nJoe Ranieri\nSoftware Engineer\nGrammaTech, Inc.\n"},{"id":"369513","messageId":"000001d4c6d9$bceb5810$36c20830$@nexbridge.com","threadId":"50528","inReplyTo":"1ff48830-f3de-11f4-9014-c20eb3890c21@grammatech.com","subject":"RE: [BUG] ls-files showing deleted files (unchecked lstat return value)","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-02-17T15:59:12Z","receivedAt":"2019-02-17T15:59:23Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On February 17, 2019 8:50, Joe Ranieri wrote:\n> \"git ls-files -m\" can show deleted files, despite -d not having been specified.\n> This is due to ls-files.c's show_files function calling lstat but not checking the\n> return value before calling ie_modified with the uninitialized stat structure.\n\nWhat version of git are you looking scanning? Commit 8989e1950a (2.21.0-rc1) has the following:\n\nerr = lstat(fullname.buf, &st);\nif (show_deleted && err)\n...\n\nYou may be correct that the following check:\n if (show_modified && ie_modified(repo->index, ce, &st, 0\n\nmay need to include !err. Is that your conclusion? Is there a test case you have to demonstrate this that I can include in the test suite?\n\nThe patch would be (subject to my bad mailer messing it up):\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 29a8762d46..fc21f47954 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -348,7 +348,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir)\n err = lstat(fullname.buf, &st);\n if (show_deleted && err)\n show_ce(repo, dir, ce, fullname.buf, tag_removed);\n- if (show_modified && ie_modified(repo->index, ce, &st, 0))\n+ if (show_modified && !err && ie_modified(repo->index, ce, &st, 0))\n show_ce(repo, dir, ce, fullname.buf, tag_modified);\n }\n }\n\nRegards,\nRandall\n\n-- Brief whoami:\n NonStop developer since approximately 211288444200000000\n UNIX developer since approximately 421664400\n-- In my real life, I talk too much.\n\n\n\n"},{"id":"369527","messageId":"6588ca4f-7ecb-748a-ded5-f9bd014700c4@grammatech.com","threadId":"50528","inReplyTo":"000001d4c6d9$bceb5810$36c20830$@nexbridge.com","subject":"Re: [BUG] ls-files showing deleted files (unchecked lstat return value)","fromName":"Joe Ranieri","fromEmail":"jranieri@grammatech.com","sentAt":"2019-02-18T14:09:56Z","receivedAt":"2019-02-18T14:09:59Z","isPatch":false,"sender":{"key":"jranieri@grammatech.com","avatar":null},"body":"On 2/17/19 10:59, Randall S. Becker wrote:\n> On February 17, 2019 8:50, Joe Ranieri wrote:\n>> \"git ls-files -m\" can show deleted files, despite -d not having been specified.\n>> This is due to ls-files.c's show_files function calling lstat but not checking the\n>> return value before calling ie_modified with the uninitialized stat structure.\n> \n> What version of git are you looking scanning? Commit 8989e1950a (2.21.0-rc1) has the following:\n\nThe analysis was performed on an old version of git, but I examined the \ncode manually (commit 7589e63) to verify the code hadn't meaningfully \nchanged.\n\n> err = lstat(fullname.buf, &st);\n> if (show_deleted && err)\n> ...\n> \n> You may be correct that the following check:\n> if (show_modified && ie_modified(repo->index, ce, &st, 0\n> \n> may need to include !err. Is that your conclusion? Is there a test case you have to demonstrate this that I can include in the test suite?\n\nYes, this is the line I was referring to and the change I would have \nsuggested. I don't have an automated test case, though I was able to \nreplicate it by deleting a file on disk and running \"git ls-files -m\".\n\n-- \nThanks and Regards,\nJoe Ranieri\nSoftware Engineer\nGrammaTech, Inc.\n"},{"id":"369531","messageId":"20190218151725.GL1622@szeder.dev","threadId":"50528","inReplyTo":"1ff48830-f3de-11f4-9014-c20eb3890c21@grammatech.com","subject":"Re: [BUG] ls-files showing deleted files (unchecked lstat return value)","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-02-18T15:17:25Z","receivedAt":"2019-02-18T15:17:32Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, Feb 17, 2019 at 08:49:39AM -0500, Joe Ranieri wrote:\n> \"git ls-files -m\" can show deleted files, despite -d not having been\n> specified. \n\nTo my understanding that's intentional: a deleted file is considered\nmodified, because its content clearly doesn't match the tracked\ncontent.\n\n\n> This is due to ls-files.c's show_files function calling lstat but\n> not checking the return value before calling ie_modified with the\n> uninitialized stat structure.\n> \n> This problem was found using the static analysis tool GrammaTech CodeSonar.\n"},{"id":"369536","messageId":"000c01d4c7a5$17915a00$46b40e00$@nexbridge.com","threadId":"50528","inReplyTo":"20190218151725.GL1622@szeder.dev","subject":"RE: [BUG] ls-files showing deleted files (unchecked lstat return value)","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2019-02-18T16:14:52Z","receivedAt":"2019-02-18T16:15:04Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On February 18, 2019 10:17, SZEDER Gábor wrote:\n> To: Joe Ranieri \n> Cc: git@vger.kernel.org\n> Subject: Re: [BUG] ls-files showing deleted files (unchecked lstat return\n> value)\n> \n> On Sun, Feb 17, 2019 at 08:49:39AM -0500, Joe Ranieri wrote:\n> > \"git ls-files -m\" can show deleted files, despite -d not having been\n> > specified.\n> \n> To my understanding that's intentional: a deleted file is considered modified,\n> because its content clearly doesn't match the tracked content.\n\nThat's a good point and why I asked for a specific test condition that illustrated the bug as being unanticipated. I'm not sure what Joe supplied elsewhere in the thread does that.\n\n> > This is due to ls-files.c's show_files function calling lstat but not\n> > checking the return value before calling ie_modified with the\n> > uninitialized stat structure.\n\n\n"},{"id":"369745","messageId":"xmqqh8cyul87.fsf@gitster-ct.c.googlers.com","threadId":"50528","inReplyTo":"20190218151725.GL1622@szeder.dev","subject":"Re: [BUG] ls-files showing deleted files (unchecked lstat return value)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-20T22:51:52Z","receivedAt":"2019-02-20T22:51:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Sun, Feb 17, 2019 at 08:49:39AM -0500, Joe Ranieri wrote:\n>> \"git ls-files -m\" can show deleted files, despite -d not having been\n>> specified. \n>\n> To my understanding that's intentional: a deleted file is considered\n> modified, because its content clearly doesn't match the tracked\n> content.\n\nHmph, I am not so sure about that.\n\nIt seems that b0391890 (\"Show modified files in git-ls-files\",\n2005-09-19) fixes one of its draft version's bugs in\n\"http://public-inbox.org/git/43179E59.80106@didntduck.org/\" by not\nletting it use ce_match_stat() directly, but introducing\nce_modified() that inspects the data for actual changes. The effort\nhowever did not spot the other bug, namely, lstat() returning an\nerror for ENOENT.\n\nI think the original intent was for \"ls-files -d\", \"ls-files -m\" and\n\"ls-files -d m\" all can be used in a meaningful way by keeping these\ntwo selectors independent. The buggy implementation did not realize\nthat intent correctly, though.\n"}]}