# [BUG] ls-files showing deleted files (unchecked lstat return value) 6 messages from 2019-02-17 to 2019-02-20. Participants: Joe Ranieri, Randall S. Becker, SZEDER Gábor, Junio C Hamano. Thread: https://gitlist.dev/t/50528 ## Joe Ranieri, 2019-02-17 13:49 Subject: [BUG] ls-files showing deleted files (unchecked lstat return value) Message-ID: <1ff48830-f3de-11f4-9014-c20eb3890c21@grammatech.com> URL: https://gitlist.dev/e/1ff48830-f3de-11f4-9014-c20eb3890c21%40grammatech.com ``` "git ls-files -m" can show deleted files, despite -d not having been specified. This is due to ls-files.c's show_files function calling lstat but not checking the return value before calling ie_modified with the uninitialized stat structure. This problem was found using the static analysis tool GrammaTech CodeSonar. -- Thanks and Regards, Joe Ranieri Software Engineer GrammaTech, Inc. ``` ## Randall S. Becker, 2019-02-17 15:59 Subject: RE: [BUG] ls-files showing deleted files (unchecked lstat return value) Message-ID: <000001d4c6d9$bceb5810$36c20830$@nexbridge.com> URL: https://gitlist.dev/e/000001d4c6d9%24bceb5810%2436c20830%24%40nexbridge.com In-Reply-To: <1ff48830-f3de-11f4-9014-c20eb3890c21@grammatech.com> ``` On February 17, 2019 8:50, Joe Ranieri wrote: > "git ls-files -m" can show deleted files, despite -d not having been specified. > This is due to ls-files.c's show_files function calling lstat but not checking the > return value before calling ie_modified with the uninitialized stat structure. What version of git are you looking scanning? Commit 8989e1950a (2.21.0-rc1) has the following: err = lstat(fullname.buf, &st); if (show_deleted && err) ... You may be correct that the following check: if (show_modified && ie_modified(repo->index, ce, &st, 0 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? The patch would be (subject to my bad mailer messing it up): diff --git a/builtin/ls-files.c b/builtin/ls-files.c index 29a8762d46..fc21f47954 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -348,7 +348,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir) err = lstat(fullname.buf, &st); if (show_deleted && err) show_ce(repo, dir, ce, fullname.buf, tag_removed); - if (show_modified && ie_modified(repo->index, ce, &st, 0)) + if (show_modified && !err && ie_modified(repo->index, ce, &st, 0)) show_ce(repo, dir, ce, fullname.buf, tag_modified); } } Regards, Randall -- Brief whoami: NonStop developer since approximately 211288444200000000 UNIX developer since approximately 421664400 -- In my real life, I talk too much. ``` ## Joe Ranieri, 2019-02-18 14:09 Subject: Re: [BUG] ls-files showing deleted files (unchecked lstat return value) Message-ID: <6588ca4f-7ecb-748a-ded5-f9bd014700c4@grammatech.com> URL: https://gitlist.dev/e/6588ca4f-7ecb-748a-ded5-f9bd014700c4%40grammatech.com In-Reply-To: <000001d4c6d9$bceb5810$36c20830$@nexbridge.com> ``` On 2/17/19 10:59, Randall S. Becker wrote: > On February 17, 2019 8:50, Joe Ranieri wrote: >> "git ls-files -m" can show deleted files, despite -d not having been specified. >> This is due to ls-files.c's show_files function calling lstat but not checking the >> return value before calling ie_modified with the uninitialized stat structure. > > What version of git are you looking scanning? Commit 8989e1950a (2.21.0-rc1) has the following: The analysis was performed on an old version of git, but I examined the code manually (commit 7589e63) to verify the code hadn't meaningfully changed. > err = lstat(fullname.buf, &st); > if (show_deleted && err) > ... > > You may be correct that the following check: > if (show_modified && ie_modified(repo->index, ce, &st, 0 > > 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? Yes, this is the line I was referring to and the change I would have suggested. I don't have an automated test case, though I was able to replicate it by deleting a file on disk and running "git ls-files -m". -- Thanks and Regards, Joe Ranieri Software Engineer GrammaTech, Inc. ``` ## SZEDER Gábor, 2019-02-18 15:17 Subject: Re: [BUG] ls-files showing deleted files (unchecked lstat return value) Message-ID: <20190218151725.GL1622@szeder.dev> URL: https://gitlist.dev/e/20190218151725.GL1622%40szeder.dev In-Reply-To: <1ff48830-f3de-11f4-9014-c20eb3890c21@grammatech.com> ``` On Sun, Feb 17, 2019 at 08:49:39AM -0500, Joe Ranieri wrote: > "git ls-files -m" can show deleted files, despite -d not having been > specified. To my understanding that's intentional: a deleted file is considered modified, because its content clearly doesn't match the tracked content. > This is due to ls-files.c's show_files function calling lstat but > not checking the return value before calling ie_modified with the > uninitialized stat structure. > > This problem was found using the static analysis tool GrammaTech CodeSonar. ``` ## Randall S. Becker, 2019-02-18 16:14 Subject: RE: [BUG] ls-files showing deleted files (unchecked lstat return value) Message-ID: <000c01d4c7a5$17915a00$46b40e00$@nexbridge.com> URL: https://gitlist.dev/e/000c01d4c7a5%2417915a00%2446b40e00%24%40nexbridge.com In-Reply-To: <20190218151725.GL1622@szeder.dev> ``` On February 18, 2019 10:17, SZEDER Gábor wrote: > To: Joe Ranieri > Cc: git@vger.kernel.org > Subject: Re: [BUG] ls-files showing deleted files (unchecked lstat return > value) > > On Sun, Feb 17, 2019 at 08:49:39AM -0500, Joe Ranieri wrote: > > "git ls-files -m" can show deleted files, despite -d not having been > > specified. > > To my understanding that's intentional: a deleted file is considered modified, > because its content clearly doesn't match the tracked content. That'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. > > This is due to ls-files.c's show_files function calling lstat but not > > checking the return value before calling ie_modified with the > > uninitialized stat structure. ``` ## Junio C Hamano, 2019-02-20 22:51 Subject: Re: [BUG] ls-files showing deleted files (unchecked lstat return value) Message-ID: URL: https://gitlist.dev/e/xmqqh8cyul87.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20190218151725.GL1622@szeder.dev> ``` SZEDER Gábor writes: > On Sun, Feb 17, 2019 at 08:49:39AM -0500, Joe Ranieri wrote: >> "git ls-files -m" can show deleted files, despite -d not having been >> specified. > > To my understanding that's intentional: a deleted file is considered > modified, because its content clearly doesn't match the tracked > content. Hmph, I am not so sure about that. It seems that b0391890 ("Show modified files in git-ls-files", 2005-09-19) fixes one of its draft version's bugs in "http://public-inbox.org/git/43179E59.80106@didntduck.org/" by not letting it use ce_match_stat() directly, but introducing ce_modified() that inspects the data for actual changes. The effort however did not spot the other bug, namely, lstat() returning an error for ENOENT. I think the original intent was for "ls-files -d", "ls-files -m" and "ls-files -d m" all can be used in a meaningful way by keeping these two selectors independent. The buggy implementation did not realize that intent correctly, though. ```