* [BUG] ls-files showing deleted files (unchecked lstat return value) @ 2019-02-17 13:49 Joe Ranieri 2019-02-17 15:59 ` Randall S. Becker 2019-02-18 15:17 ` SZEDER Gábor 0 siblings, 2 replies; 6+ messages in thread From: Joe Ranieri @ 2019-02-17 13:49 UTC (permalink / raw) To: git "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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [BUG] ls-files showing deleted files (unchecked lstat return value) 2019-02-17 13:49 [BUG] ls-files showing deleted files (unchecked lstat return value) Joe Ranieri @ 2019-02-17 15:59 ` Randall S. Becker 2019-02-18 14:09 ` Joe Ranieri 2019-02-18 15:17 ` SZEDER Gábor 1 sibling, 1 reply; 6+ messages in thread From: Randall S. Becker @ 2019-02-17 15:59 UTC (permalink / raw) To: 'Joe Ranieri', git 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. ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [BUG] ls-files showing deleted files (unchecked lstat return value) 2019-02-17 15:59 ` Randall S. Becker @ 2019-02-18 14:09 ` Joe Ranieri 0 siblings, 0 replies; 6+ messages in thread From: Joe Ranieri @ 2019-02-18 14:09 UTC (permalink / raw) To: Randall S. Becker, git 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] ls-files showing deleted files (unchecked lstat return value) 2019-02-17 13:49 [BUG] ls-files showing deleted files (unchecked lstat return value) Joe Ranieri 2019-02-17 15:59 ` Randall S. Becker @ 2019-02-18 15:17 ` SZEDER Gábor 2019-02-18 16:14 ` Randall S. Becker 2019-02-20 22:51 ` Junio C Hamano 1 sibling, 2 replies; 6+ messages in thread From: SZEDER Gábor @ 2019-02-18 15:17 UTC (permalink / raw) To: Joe Ranieri; +Cc: git 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [BUG] ls-files showing deleted files (unchecked lstat return value) 2019-02-18 15:17 ` SZEDER Gábor @ 2019-02-18 16:14 ` Randall S. Becker 2019-02-20 22:51 ` Junio C Hamano 1 sibling, 0 replies; 6+ messages in thread From: Randall S. Becker @ 2019-02-18 16:14 UTC (permalink / raw) To: 'SZEDER Gábor', 'Joe Ranieri'; +Cc: git On February 18, 2019 10:17, SZEDER Gábor wrote: > To: Joe Ranieri <jranieri@grammatech.com> > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] ls-files showing deleted files (unchecked lstat return value) 2019-02-18 15:17 ` SZEDER Gábor 2019-02-18 16:14 ` Randall S. Becker @ 2019-02-20 22:51 ` Junio C Hamano 1 sibling, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2019-02-20 22:51 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Joe Ranieri, git SZEDER Gábor <szeder.dev@gmail.com> 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-20 22:51 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-17 13:49 [BUG] ls-files showing deleted files (unchecked lstat return value) Joe Ranieri 2019-02-17 15:59 ` Randall S. Becker 2019-02-18 14:09 ` Joe Ranieri 2019-02-18 15:17 ` SZEDER Gábor 2019-02-18 16:14 ` Randall S. Becker 2019-02-20 22:51 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).