* [PATCH] diff: fix lstat() error handling in diff_populate_filespec() [not found] <CGME20171027093331epcas2p1a945263c12b8ba608492693da4e3eff2@epcas2p1.samsung.com> @ 2017-10-27 9:33 ` Andrey Okoshkin 2017-10-27 16:35 ` Junio C Hamano 0 siblings, 1 reply; 3+ messages in thread From: Andrey Okoshkin @ 2017-10-27 9:33 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, pclouds, Ivan Arishchenko Add lstat() error handling not only for ENOENT case. Otherwise uninitialised 'struct stat st' variable is used later in case of lstat() non-ENOENT failure which leads to processing of rubbish values of file mode ('S_ISLNK(st.st_mode)' check) or size ('xsize_t(st.st_size)'). Signed-off-by: Andrey Okoshkin <a.okoshkin@samsung.com> --- Hello, I've injected a fault to git binary with the internal tool for fault tolerance evaluation: lstat() returns '-1' and errno is set to 'EACCES' at diff_populate_filespec git/diff.c:2850. In a real life it's very difficult to reproduce such behaviour. I'm not sure why only ENOENT error of lstat() is considered as an error but passing by other errno values leads to reading of uninitialized 'struct stat st' variable. It means that the populated 'diff_filespec' structure may be incorrectly filled. Also diff_populate_filespec() result is not checked at diff_filespec_is_binary() but it seems OK there. Best regards, Andrey diff.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/diff.c b/diff.c index 8406a8324..d737a78a1 100644 --- a/diff.c +++ b/diff.c @@ -2848,14 +2848,12 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags) int fd; if (lstat(s->path, &st) < 0) { - if (errno == ENOENT) { - err_empty: - err = -1; - empty: - s->data = (char *)""; - s->size = 0; - return err; - } + err_empty: + err = -1; + empty: + s->data = (char *)""; + s->size = 0; + return err; } s->size = xsize_t(st.st_size); if (!s->size) -- 2.14.3 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] diff: fix lstat() error handling in diff_populate_filespec() 2017-10-27 9:33 ` [PATCH] diff: fix lstat() error handling in diff_populate_filespec() Andrey Okoshkin @ 2017-10-27 16:35 ` Junio C Hamano 2017-10-27 16:58 ` Junio C Hamano 0 siblings, 1 reply; 3+ messages in thread From: Junio C Hamano @ 2017-10-27 16:35 UTC (permalink / raw) To: Andrey Okoshkin; +Cc: git, Jeff King, pclouds, Ivan Arishchenko Andrey Okoshkin <a.okoshkin@samsung.com> writes: > I'm not sure why only ENOENT error of lstat() is considered as an > error but passing by other errno values leads to reading of > uninitialized 'struct stat st' variable. It means that the > populated 'diff_filespec' structure may be incorrectly filled. Entirely correct. There is no fundamental reason to try special casing ENOENT, unless we are clearing the "this is an error" bit when the errno is ENOENT---but this code does not even do so. All errors are errors---we wanted to know the result of lstat() to carry on, and we couldn't figure out the status. We do not want to die immediately (instead we want to show diffs for other paths), so substituting the result with an empty string is the least bad thing we can do at this point in the code. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] diff: fix lstat() error handling in diff_populate_filespec() 2017-10-27 16:35 ` Junio C Hamano @ 2017-10-27 16:58 ` Junio C Hamano 0 siblings, 0 replies; 3+ messages in thread From: Junio C Hamano @ 2017-10-27 16:58 UTC (permalink / raw) To: Andrey Okoshkin; +Cc: git, Jeff King, pclouds, Ivan Arishchenko Junio C Hamano <gitster@pobox.com> writes: > Andrey Okoshkin <a.okoshkin@samsung.com> writes: > >> I'm not sure why only ENOENT error of lstat() is considered as an >> error but passing by other errno values leads to reading of >> uninitialized 'struct stat st' variable. It means that the >> populated 'diff_filespec' structure may be incorrectly filled. > > Entirely correct. There is no fundamental reason to try special > casing ENOENT, unless we are clearing the "this is an error" bit > when the errno is ENOENT---but this code does not even do so. All > errors are errors---we wanted to know the result of lstat() to carry > on, and we couldn't figure out the status. We do not want to die > immediately (instead we want to show diffs for other paths), so > substituting the result with an empty string is the least bad thing > we can do at this point in the code. By the way, if the reason why we are hitting this lstat(2) (not the reason why lstat(2) is failing) is not because !s->oid.valid (i.e. we are reading the working tree side because that is what is on one side of the comparison) but because reuse_worktree_file() told us to (i.e. we actually are trying to fill the filespec with the blob contents, but found that we have already the same content in a working tree file and found it more efficient to read from there, rather than doing the read_sha1_file() on the s->oid.hash), it probably is better to fall back to the other side of the if/else when we see an error from this lstat(2) and pretend as if we got false from reuse_worktree_file(). That would allow us continue with the correct information we originally wanted to use; after all, reusing is merely an optimization. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-27 16:58 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20171027093331epcas2p1a945263c12b8ba608492693da4e3eff2@epcas2p1.samsung.com> 2017-10-27 9:33 ` [PATCH] diff: fix lstat() error handling in diff_populate_filespec() Andrey Okoshkin 2017-10-27 16:35 ` Junio C Hamano 2017-10-27 16:58 ` 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).