threads / bug / 50528
Subject: [BUG] ls-files showing deleted files (unchecked lstat return value)
"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. BeckerRE: [BUG] ls-files showing deleted files (unchecked lstat return value)
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.
Re: [BUG] ls-files showing deleted files (unchecked lstat return value)
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áborRe: [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.
> 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. BeckerRE: [BUG] ls-files showing deleted files (unchecked lstat return value)
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.
Junio C HamanoRe: [BUG] ls-files showing deleted files (unchecked lstat return value)
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.