{"thread":{"id":"47041","subject":"[PATCH] diff: fix lstat() error handling in diff_populate_filespec()","startedAt":"2017-10-27T09:33:41Z","lastAt":"2017-10-27T16:58:10Z","messageCount":3,"participants":["Andrey Okoshkin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"331147","messageId":"72e8146b-d4ab-ad7e-6afd-9168806eeca2@samsung.com","threadId":"47041","inReplyTo":"CGME20171027093331epcas2p1a945263c12b8ba608492693da4e3eff2@epcas2p1.samsung.com","subject":"[PATCH] diff: fix lstat() error handling in diff_populate_filespec()","fromName":"Andrey Okoshkin","fromEmail":"a.okoshkin@samsung.com","sentAt":"2017-10-27T09:33:25Z","receivedAt":"2017-10-27T09:33:41Z","isPatch":true,"sender":{"key":"a.okoshkin@samsung.com","avatar":"https://avatars.githubusercontent.com/u/10993050?v=4"},"body":"Add lstat() error handling not only for ENOENT case.\nOtherwise uninitialised 'struct stat st' variable is used later in case of\nlstat() non-ENOENT failure which leads to processing of rubbish values of\nfile mode ('S_ISLNK(st.st_mode)' check) or size ('xsize_t(st.st_size)').\n\nSigned-off-by: Andrey Okoshkin \n---\nHello,\n\nI've injected a fault to git binary with the internal tool for fault tolerance\nevaluation: lstat() returns '-1' and errno is set to 'EACCES' at\ndiff_populate_filespec git/diff.c:2850.\nIn a real life it's very difficult to reproduce such behaviour.\nI'm not sure why only ENOENT error of lstat() is considered as an error but passing\nby other errno values leads to reading of uninitialized 'struct stat st' variable.\nIt means that the populated 'diff_filespec' structure may be incorrectly filled.\n\nAlso diff_populate_filespec() result is not checked at diff_filespec_is_binary() but\nit seems OK there.\n\nBest regards,\nAndrey\n\n diff.c | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 8406a8324..d737a78a1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2848,14 +2848,12 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\tint fd;\n \n \t\tif (lstat(s->path, &st) < 0) {\n-\t\t\tif (errno == ENOENT) {\n-\t\t\terr_empty:\n-\t\t\t\terr = -1;\n-\t\t\tempty:\n-\t\t\t\ts->data = (char *)\"\";\n-\t\t\t\ts->size = 0;\n-\t\t\t\treturn err;\n-\t\t\t}\n+\t\terr_empty:\n+\t\t\terr = -1;\n+\t\tempty:\n+\t\t\ts->data = (char *)\"\";\n+\t\t\ts->size = 0;\n+\t\t\treturn err;\n \t\t}\n \t\ts->size = xsize_t(st.st_size);\n \t\tif (!s->size)\n-- \n2.14.3\n"},{"id":"331172","messageId":"xmqq3764czv9.fsf@gitster.mtv.corp.google.com","threadId":"47041","inReplyTo":"72e8146b-d4ab-ad7e-6afd-9168806eeca2@samsung.com","subject":"Re: [PATCH] diff: fix lstat() error handling in diff_populate_filespec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-27T16:35:54Z","receivedAt":"2017-10-27T16:36:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrey Okoshkin writes:\n\n> I'm not sure why only ENOENT error of lstat() is considered as an\n> error but passing by other errno values leads to reading of\n> uninitialized 'struct stat st' variable. It means that the\n> populated 'diff_filespec' structure may be incorrectly filled.\n\nEntirely correct. There is no fundamental reason to try special\ncasing ENOENT, unless we are clearing the \"this is an error\" bit\nwhen the errno is ENOENT---but this code does not even do so. All\nerrors are errors---we wanted to know the result of lstat() to carry\non, and we couldn't figure out the status. We do not want to die\nimmediately (instead we want to show diffs for other paths), so\nsubstituting the result with an empty string is the least bad thing\nwe can do at this point in the code.\n\n"},{"id":"331173","messageId":"xmqqy3nwbk9x.fsf@gitster.mtv.corp.google.com","threadId":"47041","inReplyTo":"xmqq3764czv9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] diff: fix lstat() error handling in diff_populate_filespec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-27T16:58:02Z","receivedAt":"2017-10-27T16:58:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Andrey Okoshkin writes:\n>\n>> I'm not sure why only ENOENT error of lstat() is considered as an\n>> error but passing by other errno values leads to reading of\n>> uninitialized 'struct stat st' variable. It means that the\n>> populated 'diff_filespec' structure may be incorrectly filled.\n>\n> Entirely correct. There is no fundamental reason to try special\n> casing ENOENT, unless we are clearing the \"this is an error\" bit\n> when the errno is ENOENT---but this code does not even do so. All\n> errors are errors---we wanted to know the result of lstat() to carry\n> on, and we couldn't figure out the status. We do not want to die\n> immediately (instead we want to show diffs for other paths), so\n> substituting the result with an empty string is the least bad thing\n> we can do at this point in the code.\n\nBy the way, if the reason why we are hitting this lstat(2) (not the\nreason why lstat(2) is failing) is not because !s->oid.valid\n(i.e. we are reading the working tree side because that is what is\non one side of the comparison) but because reuse_worktree_file()\ntold us to (i.e. we actually are trying to fill the filespec with\nthe blob contents, but found that we have already the same content\nin a working tree file and found it more efficient to read from\nthere, rather than doing the read_sha1_file() on the s->oid.hash),\nit probably is better to fall back to the other side of the if/else\nwhen we see an error from this lstat(2) and pretend as if we got\nfalse from reuse_worktree_file(). \n\nThat would allow us continue with the correct information we\noriginally wanted to use; after all, reusing is merely an\noptimization.\n"}]}