{"thread":{"id":"53821","subject":"[PATCH] entry: check for fstat() errors after checkout","startedAt":"2020-07-09T02:10:50Z","lastAt":"2020-07-21T20:57:11Z","messageCount":9,"participants":["Matheus Tavares","Derrick Stolee","Junio C Hamano","Matheus Tavares Bernardino"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"401213","messageId":"fbde5e1c9042ea06b448bc759cea9d45e300961f.1594260597.git.matheus.bernardino@usp.br","threadId":"53821","inReplyTo":null,"subject":"[PATCH] entry: check for fstat() errors after checkout","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-07-09T02:10:39Z","receivedAt":"2020-07-09T02:10:50Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"In 11179eb311 (\"entry.c: check if file exists after checkout\",\n2017-10-05) we started checking the result of the lstat() call done\nafter writing a file, to avoid writing garbage to the corresponding\ncache entry. However, the code skips calling lstat() if it's possible\nto use fstat() when it still has the file descriptor open. And when\ncalling fstat() we don't do the same error checking. To fix that, let\nthe callers of fstat_output() know when fstat() fails. In this case,\nwrite_entry() will try to use lstat() and properly report an error if\nthat fails as well.\n\nSigned-off-by: Matheus Tavares \n---\n entry.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 00b4903366..449bd32dee 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -113,8 +113,7 @@ static int fstat_output(int fd, const struct checkout *state, struct stat *st)\n \t/* use fstat() only when path == ce->name */\n \tif (fstat_is_reliable() &&\n \t state->refresh_cache && !state->base_dir_len) {\n-\t\tfstat(fd, st);\n-\t\treturn 1;\n+\t\treturn !fstat(fd, st);\n \t}\n \treturn 0;\n }\n-- \n2.27.0\n\n"},{"id":"401241","messageId":"882592a4-ffb7-924c-b52a-3287b11318b8@gmail.com","threadId":"53821","inReplyTo":"fbde5e1c9042ea06b448bc759cea9d45e300961f.1594260597.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] entry: check for fstat() errors after checkout","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-07-09T11:41:30Z","receivedAt":"2020-07-09T11:41:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/8/2020 10:10 PM, Matheus Tavares wrote:\n> In 11179eb311 (\"entry.c: check if file exists after checkout\",\n> 2017-10-05) we started checking the result of the lstat() call done\n> after writing a file, to avoid writing garbage to the corresponding\n> cache entry. However, the code skips calling lstat() if it's possible\n> to use fstat() when it still has the file descriptor open. And when\n> calling fstat() we don't do the same error checking. To fix that, let\n> the callers of fstat_output() know when fstat() fails. In this case,\n> write_entry() will try to use lstat() and properly report an error if\n> that fails as well.\n\nLooking at this for the first time, I was confused because 11179eb311\ndoesn't touch these lines. But that's the point: it should have.\n\nThanks for finding this! I wonder if there is a way to expose this\nbehavior in a test... it definitely seems like this is only something\nthat happens if there is a failure in the filesystem, so I'm not sure\nsuch a thing is possible.\n\nIt would just be nice to know the ramifications of this change in\nbehavior, keeping in mind that this behavior started way back in\ne4c7292353 (write_entry(): use fstat() instead of lstat() when file is\nopen, 2009-02-09), over 11 years ago!\n\nThanks,\n-Stolee\n\n> Signed-off-by: Matheus Tavares \n> ---\n> entry.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n> \n> diff --git a/entry.c b/entry.c\n> index 00b4903366..449bd32dee 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -113,8 +113,7 @@ static int fstat_output(int fd, const struct checkout *state, struct stat *st)\n> \t/* use fstat() only when path == ce->name */\n> \tif (fstat_is_reliable() &&\n> \t state->refresh_cache && !state->base_dir_len) {\n> -\t\tfstat(fd, st);\n> -\t\treturn 1;\n> +\t\treturn !fstat(fd, st);\n> \t}\n> \treturn 0;\n> }\n> \n\n"},{"id":"401247","messageId":"xmqqr1tkye5g.fsf@gitster.c.googlers.com","threadId":"53821","inReplyTo":"882592a4-ffb7-924c-b52a-3287b11318b8@gmail.com","subject":"Re: [PATCH] entry: check for fstat() errors after checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-09T14:08:27Z","receivedAt":"2020-07-09T14:08:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 7/8/2020 10:10 PM, Matheus Tavares wrote:\n>> In 11179eb311 (\"entry.c: check if file exists after checkout\",\n>> 2017-10-05) we started checking the result of the lstat() call done\n>> after writing a file, to avoid writing garbage to the corresponding\n>> cache entry. However, the code skips calling lstat() if it's possible\n>> to use fstat() when it still has the file descriptor open. And when\n>> calling fstat() we don't do the same error checking. To fix that, let\n>> the callers of fstat_output() know when fstat() fails. In this case,\n>> write_entry() will try to use lstat() and properly report an error if\n>> that fails as well.\n>\n> Looking at this for the first time, I was confused because 11179eb311\n> doesn't touch these lines. But that's the point: it should have.\n>\n> Thanks for finding this! I wonder if there is a way to expose this\n> behavior in a test... it definitely seems like this is only something\n> that happens if there is a failure in the filesystem, so I'm not sure\n> such a thing is possible.\n\nIf another process removed the path from the filesystem after this\nprocess created it and before this codepath used fstat() on it,\nfstat() may succeed while lstat() would definitely fail. There is\nno \"failure in the filesystem\", but it would be harder to arrange.\n\n> It would just be nice to know the ramifications of this change in\n> behavior, keeping in mind that this behavior started way back in\n> e4c7292353 (write_entry(): use fstat() instead of lstat() when file is\n> open, 2009-02-09), over 11 years ago!\n\nYup. I am curious how this was found ;-)\n\nThanks.\n"},{"id":"401253","messageId":"xmqqd054y5t1.fsf@gitster.c.googlers.com","threadId":"53821","inReplyTo":"fbde5e1c9042ea06b448bc759cea9d45e300961f.1594260597.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] entry: check for fstat() errors after checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-09T17:08:42Z","receivedAt":"2020-07-09T17:08:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares writes:\n\n> In 11179eb311 (\"entry.c: check if file exists after checkout\",\n> 2017-10-05) we started checking the result of the lstat() call done\n> after writing a file, to avoid writing garbage to the corresponding\n> cache entry. However, the code skips calling lstat() if it's possible\n> to use fstat() when it still has the file descriptor open. And when\n> calling fstat() we don't do the same error checking. To fix that, let\n> the callers of fstat_output() know when fstat() fails. In this case,\n> write_entry() will try to use lstat() and properly report an error if\n> that fails as well.\n\nThe original is not correct as you point out, as it loses the error\nreturn from fstat(), but I do not think this is right, either.\n\nThe returned value from fstat_output() is suppsed to be \"have we\ndone fstat() so that we do not need to do a lstat()?\" Don't you\ninstead want to extend it to \"0 means we didn't, 1 means we did\nsuccessfully, and -1 means we did and failed\"? At least, the way\n_this_ function is modified by this patch is in line with that.\n\nWhich means that we'd need to update the caller(s) to match, to\navoid risking this change to be just half a change, very similarly\nto how the change in 11179eb311 was just half a change.\n\nPerhaps like this?\n\n entry.c | 23 +++++++++++++++--------\n 1 file changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 53380bb614..f48507ca42 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -108,14 +108,21 @@ static int open_output_fd(char *path, const struct cache_entry *ce, int to_tempf\n \t}\n }\n \n+/*\n+ * We have an open fd to a file that we may use lstat() on later. \n+ * When able, try doing a fstat(fd) instead and tell the caller it\n+ * does not have to do an extra lstat()\n+ *\n+ * Return 1 if we successfully ran fstat() and *st is valid.\n+ * Return 0 if we did not do fstat() and the caller should do lstat().\n+ * Return -1 if we got failure from fstat()---the caller can skip lstat().\n+ */\n static int fstat_output(int fd, const struct checkout *state, struct stat *st)\n {\n \t/* use fstat() only when path == ce->name */\n \tif (fstat_is_reliable() &&\n-\t state->refresh_cache && !state->base_dir_len) {\n-\t\tfstat(fd, st);\n-\t\treturn 1;\n-\t}\n+\t state->refresh_cache && !state->base_dir_len)\n+\t\treturn (fstat(fd, st) < 0) ? -1 : 1;\n \treturn 0;\n }\n \n@@ -369,10 +376,10 @@ static int write_entry(struct cache_entry *ce,\n finish:\n \tif (state->refresh_cache) {\n \t\tassert(state->istate);\n-\t\tif (!fstat_done)\n-\t\t\tif (lstat(ce->name, &st) < 0)\n-\t\t\t\treturn error_errno(\"unable to stat just-written file %s\",\n-\t\t\t\t\t\t ce->name);\n+\t\tif (fstat_done < 0 ||\n+\t\t (!fstat_done && lstat(ce->name, &st) < 0))\n+\t\t\treturn error_errno(\"unable to stat just-written file %s\",\n+\t\t\t\t\t ce->name);\n \t\tfill_stat_cache_info(state->istate, ce, &st);\n \t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n \t\tmark_fsmonitor_invalid(state->istate, ce);\n"},{"id":"401255","messageId":"CAHd-oW7sz5nGUGZDDguz3fdeRLa+8iWRdC+vyCB2jWvFMh3aXg@mail.gmail.com","threadId":"53821","inReplyTo":"xmqqd054y5t1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] entry: check for fstat() errors after checkout","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-07-09T17:39:18Z","receivedAt":"2020-07-09T17:39:33Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Thu, Jul 9, 2020 at 2:08 PM Junio C Hamano wrote:\n>\n> The returned value from fstat_output() is suppsed to be \"have we\n> done fstat() so that we do not need to do a lstat()?\" Don't you\n> instead want to extend it to \"0 means we didn't, 1 means we did\n> successfully, and -1 means we did and failed\"? At least, the way\n> _this_ function is modified by this patch is in line with that.\n\nMakes sense, thanks for spotting this issue.\n\n> Which means that we'd need to update the caller(s) to match, to\n> avoid risking this change to be just half a change, very similarly\n> to how the change in 11179eb311 was just half a change.\n>\n> Perhaps like this?\n>\n> entry.c | 23 +++++++++++++++--------\n> 1 file changed, 15 insertions(+), 8 deletions(-)\n>\n> diff --git a/entry.c b/entry.c\n> index 53380bb614..f48507ca42 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -108,14 +108,21 @@ static int open_output_fd(char *path, const struct cache_entry *ce, int to_tempf\n> }\n> }\n>\n> +/*\n> + * We have an open fd to a file that we may use lstat() on later.\n> + * When able, try doing a fstat(fd) instead and tell the caller it\n> + * does not have to do an extra lstat()\n> + *\n> + * Return 1 if we successfully ran fstat() and *st is valid.\n> + * Return 0 if we did not do fstat() and the caller should do lstat().\n> + * Return -1 if we got failure from fstat()---the caller can skip lstat().\n> + */\n> static int fstat_output(int fd, const struct checkout *state, struct stat *st)\n> {\n> /* use fstat() only when path == ce->name */\n> if (fstat_is_reliable() &&\n> - state->refresh_cache && !state->base_dir_len) {\n> - fstat(fd, st);\n> - return 1;\n> - }\n> + state->refresh_cache && !state->base_dir_len)\n> + return (fstat(fd, st) < 0) ? -1 : 1;\n> return 0;\n> }\n>\n> @@ -369,10 +376,10 @@ static int write_entry(struct cache_entry *ce,\n> finish:\n> if (state->refresh_cache) {\n> assert(state->istate);\n> - if (!fstat_done)\n> - if (lstat(ce->name, &st) < 0)\n> - return error_errno(\"unable to stat just-written file %s\",\n> - ce->name);\n> + if (fstat_done < 0 ||\n> + (!fstat_done && lstat(ce->name, &st) < 0))\n> + return error_errno(\"unable to stat just-written file %s\",\n> + ce->name);\n\nIf fstat() failed or we couldn't fstat() but lstat() failed, we return\nan error. Nice! Thanks for the correction.\n"},{"id":"401257","messageId":"xmqq1rlky2zg.fsf@gitster.c.googlers.com","threadId":"53821","inReplyTo":"CAHd-oW7sz5nGUGZDDguz3fdeRLa+8iWRdC+vyCB2jWvFMh3aXg@mail.gmail.com","subject":"Re: [PATCH] entry: check for fstat() errors after checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-09T18:09:39Z","receivedAt":"2020-07-09T18:09:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino writes:\n\n> On Thu, Jul 9, 2020 at 2:08 PM Junio C Hamano wrote:\n>>\n>> The returned value from fstat_output() is suppsed to be \"have we\n>> done fstat() so that we do not need to do a lstat()?\" Don't you\n>> instead want to extend it to \"0 means we didn't, 1 means we did\n>> successfully, and -1 means we did and failed\"? At least, the way\n>> _this_ function is modified by this patch is in line with that.\n>\n> Makes sense, thanks for spotting this issue.\n>\n>> Which means that we'd need to update the caller(s) to match, to\n>> avoid risking this change to be just half a change, very similarly\n>> to how the change in 11179eb311 was just half a change.\n\nThinking about this again, you _could_ argue that your version is\nbeing more defensive. fstat_is_reliable() might lie and tell us it\nis OK to use fstat() when we should do lstat(), and in such a case,\nwe take a failure from fstat() as a sign to pretend that we didn't\neven call it, and tell the caller to do an lstat(). I am actually\nOK to go in that direction, but then we probably should save away\nerrno before making this fstat() call, and restore it after it when\nwe see an error, if we were to truly pretend that we didn't make a\ncall. Otherwise error_errno() call we will make later in the flow\nwould end up reporting the error from the fstat() we chose to pretend\nthat we didn't call.\n\nAnd having said all that, I think fstat_is_reliable() can be trusted\n(it says false on Windows and says true on all others).\n\n\n"},{"id":"401806","messageId":"CAHd-oW4_A7JRmdTqCMrDU7b6-PqBSpNVD-csm3y9Bype6Vn9rA@mail.gmail.com","threadId":"53821","inReplyTo":"fbde5e1c9042ea06b448bc759cea9d45e300961f.1594260597.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] entry: check for fstat() errors after checkout","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-07-21T15:39:13Z","receivedAt":"2020-07-21T15:39:29Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Junio and Stolee\n\nI was looking further at this code and noticed that the conditions\nunder which we fstat() (or lstat()) an entry are slightly different\nthroughout entry.c:\n\n- In write_entry()'s footer, we call lstat() iff stat->refresh_cache.\n- In write_entry()'s `write_file_entry` label, we call fstat_output()\nwhen !to_tempfile.\n- In streaming_write_entry() we call fstat_output() without checking\nif !to_tempfile.\n- And, finally, in fstat_output() itself, we check\n`state->refresh_cache && !state->base_dir_len`.\n\nI understand we always check state->refresh_cache to avoid getting\nstat information we won't really need later, as we are not updating\nthe index. But why do we check !to_tempfile and !state->base_dir_len?\nDoesn't writing to a tempfile or using a checkout prefix already imply\n!state->refresh_cache?\n\nThanks,\nMatheus\n"},{"id":"401819","messageId":"xmqq1rl48wpt.fsf@gitster.c.googlers.com","threadId":"53821","inReplyTo":"CAHd-oW4_A7JRmdTqCMrDU7b6-PqBSpNVD-csm3y9Bype6Vn9rA@mail.gmail.com","subject":"Re: [PATCH] entry: check for fstat() errors after checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-21T20:00:30Z","receivedAt":"2020-07-21T20:00:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino writes:\n\n> I was looking further at this code and noticed that the conditions\n> under which we fstat() (or lstat()) an entry are slightly different\n> throughout entry.c:\n>\n> - In write_entry()'s footer, we call lstat() iff stat->refresh_cache.\n> - In write_entry()'s `write_file_entry` label, we call fstat_output()\n> when !to_tempfile.\n> - In streaming_write_entry() we call fstat_output() without checking\n> if !to_tempfile.\n> - And, finally, in fstat_output() itself, we check\n> `state->refresh_cache && !state->base_dir_len`.\n>\n> I understand we always check state->refresh_cache to avoid getting\n> stat information we won't really need later, as we are not updating\n> the index. But why do we check !to_tempfile and !state->base_dir_len?\n> Doesn't writing to a tempfile or using a checkout prefix already imply\n> !state->refresh_cache?\n\nYou can easily blame the code back to e4c72923 (write_entry(): use\nfstat() instead of lstat() when file is open, 2009-02-09). Back\nthen, only a single place assigned 0 to state.refresh_cache and that\nis in \"checkout-index\" with either base_dir_len or to_tempfile set.\n\nI do not remember, and I am fairly sure Stolee does not remember\neither. If I have to guess, this was done merely to be extra\ncautious, perhaps? As refresh_cache bit is checked first, check for\n!to_tempfile and !base_dir_len would be dead at best and redundant\nat worst.\n\n\n"},{"id":"401825","messageId":"3c5a8e0d-91d7-00a4-4253-ac60f3a34d05@gmail.com","threadId":"53821","inReplyTo":"xmqq1rl48wpt.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] entry: check for fstat() errors after checkout","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-07-21T20:57:05Z","receivedAt":"2020-07-21T20:57:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/21/2020 4:00 PM, Junio C Hamano wrote:\n> Matheus Tavares Bernardino writes:\n> \n>> I was looking further at this code and noticed that the conditions\n>> under which we fstat() (or lstat()) an entry are slightly different\n>> throughout entry.c:\n>>\n>> - In write_entry()'s footer, we call lstat() iff stat->refresh_cache.\n>> - In write_entry()'s `write_file_entry` label, we call fstat_output()\n>> when !to_tempfile.\n>> - In streaming_write_entry() we call fstat_output() without checking\n>> if !to_tempfile.\n>> - And, finally, in fstat_output() itself, we check\n>> `state->refresh_cache && !state->base_dir_len`.\n>>\n>> I understand we always check state->refresh_cache to avoid getting\n>> stat information we won't really need later, as we are not updating\n>> the index. But why do we check !to_tempfile and !state->base_dir_len?\n>> Doesn't writing to a tempfile or using a checkout prefix already imply\n>> !state->refresh_cache?\n> \n> You can easily blame the code back to e4c72923 (write_entry(): use\n> fstat() instead of lstat() when file is open, 2009-02-09). Back\n> then, only a single place assigned 0 to state.refresh_cache and that\n> is in \"checkout-index\" with either base_dir_len or to_tempfile set.\n> \n> I do not remember, and I am fairly sure Stolee does not remember\n> either. If I have to guess, this was done merely to be extra\n> cautious, perhaps? As refresh_cache bit is checked first, check for\n> !to_tempfile and !base_dir_len would be dead at best and redundant\n> at worst.\n\nYeah, this portion is way outside of my expertise. I'm happy\nto _try_ reading patches, but I'd have difficulty being\nconfident in any change in this area.\n\nThanks,\n-Stolee\n\n"}]}