{"thread":{"id":"42797","subject":"[PATCH] notes-merge: use O_EXCL to avoid overwriting existing files","startedAt":"2016-07-07T20:08:50Z","lastAt":"2016-07-08T09:43:52Z","messageCount":4,"participants":["René Scharfe","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"290977","messageId":"577EB6BE.6090504@web.de","threadId":"42797","inReplyTo":null,"subject":"[PATCH] notes-merge: use O_EXCL to avoid overwriting existing files","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-07-07T20:08:30Z","receivedAt":"2016-07-07T20:08:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use the open(2) flag O_EXCL to ensure the file doesn't already exist\ninstead of (racily) calling stat(2) through file_exists().  While at it\nswitch to xopen() to reduce code duplication and get more consistent\nerror messages.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n notes-merge.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/notes-merge.c b/notes-merge.c\nindex b7814c9..2b29fc4 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -298,12 +298,8 @@ static void write_buf_to_worktree(const unsigned char *obj,\n \tchar *path = git_pathdup(NOTES_MERGE_WORKTREE \"/%s\", sha1_to_hex(obj));\n \tif (safe_create_leading_directories_const(path))\n \t\tdie_errno(\"unable to create directory for '%s'\", path);\n-\tif (file_exists(path))\n-\t\tdie(\"found existing file at '%s'\", path);\n \n-\tfd = open(path, O_WRONLY | O_TRUNC | O_CREAT, 0666);\n-\tif (fd < 0)\n-\t\tdie_errno(\"failed to open '%s'\", path);\n+\tfd = xopen(path, O_WRONLY | O_EXCL | O_CREAT, 0666);\n \n \twhile (size > 0) {\n \t\tlong ret = write_in_full(fd, buf, size);\n-- \n2.9.0\n\n"},{"id":"290982","messageId":"20160707203822.GB11804@sigill.intra.peff.net","threadId":"42797","inReplyTo":"577EB6BE.6090504@web.de","subject":"Re: [PATCH] notes-merge: use O_EXCL to avoid overwriting existing files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-07T20:38:22Z","receivedAt":"2016-07-07T20:38:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 07, 2016 at 10:08:30PM +0200, René Scharfe wrote:\n\n> Use the open(2) flag O_EXCL to ensure the file doesn't already exist\n> instead of (racily) calling stat(2) through file_exists().  While at it\n> switch to xopen() to reduce code duplication and get more consistent\n> error messages.\n\nThis is definitely an improvement, as it behaves the same except for the\nTOCTOU race. But not being very familiar with the notes-merge code, I\nhave to wonder if this is a system of a larger design issue.\n\nWhy do we care that the file exists? Should we instead be using the\nlockfile code to get exclusive access to it? That would also switch us\nto doing the write-to-tempfile-and-rename dance, but that seems like it\nwould be a good thing. If we hit a write() error in the code now, we\nleave a partially-written file in the notes worktree.\n\nI dunno. From my cursory reading of the code, it seems like we'd never\nreally expect this file_exists() to trigger in the first place, so\nperhaps it's not worth thinking too hard about it.\n\n-Peff\n"},{"id":"290987","messageId":"xmqq1t35p32j.fsf@gitster.mtv.corp.google.com","threadId":"42797","inReplyTo":"20160707203822.GB11804@sigill.intra.peff.net","subject":"Re: [PATCH] notes-merge: use O_EXCL to avoid overwriting existing files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-07T21:10:12Z","receivedAt":"2016-07-07T21:10:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Why do we care that the file exists? Should we instead be using the\n> lockfile code to get exclusive access to it? That would also switch us\n> to doing the write-to-tempfile-and-rename dance, but that seems like it\n> would be a good thing. If we hit a write() error in the code now, we\n> leave a partially-written file in the notes worktree.\n\nYeah, I had the same thought when I saw the change.\n\n> I dunno. From my cursory reading of the code, it seems like we'd never\n> really expect this file_exists() to trigger in the first place, so\n> perhaps it's not worth thinking too hard about it.\n\nPerhaps.\n"},{"id":"291026","messageId":"20160708093733.GA20528@sigill.intra.peff.net","threadId":"42797","inReplyTo":"577EB6BE.6090504@web.de","subject":"Re: [PATCH] notes-merge: use O_EXCL to avoid overwriting existing files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T09:37:33Z","receivedAt":"2016-07-08T09:43:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 07, 2016 at 10:08:30PM +0200, René Scharfe wrote:\n\n> diff --git a/notes-merge.c b/notes-merge.c\n> index b7814c9..2b29fc4 100644\n> --- a/notes-merge.c\n> +++ b/notes-merge.c\n> @@ -298,12 +298,8 @@ static void write_buf_to_worktree(const unsigned char *obj,\n>  \tchar *path = git_pathdup(NOTES_MERGE_WORKTREE \"/%s\", sha1_to_hex(obj));\n>  \tif (safe_create_leading_directories_const(path))\n>  \t\tdie_errno(\"unable to create directory for '%s'\", path);\n> -\tif (file_exists(path))\n> -\t\tdie(\"found existing file at '%s'\", path);\n>  \n> -\tfd = open(path, O_WRONLY | O_TRUNC | O_CREAT, 0666);\n> -\tif (fd < 0)\n> -\t\tdie_errno(\"failed to open '%s'\", path);\n> +\tfd = xopen(path, O_WRONLY | O_EXCL | O_CREAT, 0666);\n\nWhile working on write_file_buf() elsewhere, I wondered if this was a\ngood candidate for conversion. But it's not because of the O_EXCL.\n\nI was tempted by something like:\n\n  write_file_buf(path, O_EXCL, buf, len);\n\nbut I think that just makes the interface more cluttered for the other\ncallers. If you are doing something clever with O_EXCL, you probably\nshould just do it by hand.\n\nHowever, we can make the loop less painful, as below (on top of your\npatch).\n\n-- >8 --\nSubject: [PATCH] notes-merge: use write_or_die()\n\nThis output loop is overkill. For one thing, we do not need\nto loop on write_in_full(); it's entire purpose is to avoid\nlooping, and it even includes the \"disk full?\" check itself.\n\nSecondly, the check for EPIPE is pointless. We know this is\nan on-disk file that we just opened, so short of somebody\nsneaking a FIFO into our notes-merge working tree, it will\nnot be a pipe. And even if it is, we would generally die of\nSIGPIPE before hitting this code. And even if we somehow\nignored SIGPIPE, dying (rather than silently ignoring the\nerror) seems like the only sensible action anyway.\n\nSo what we're left with is calling write_in_full() and dying\nif it fails. And that's exactly what write_or_die() does.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n notes-merge.c | 16 +---------------\n 1 file changed, 1 insertion(+), 15 deletions(-)\n\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 2b29fc4..a659a62 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -300,21 +300,7 @@ static void write_buf_to_worktree(const unsigned char *obj,\n \t\tdie_errno(\"unable to create directory for '%s'\", path);\n \n \tfd = xopen(path, O_WRONLY | O_EXCL | O_CREAT, 0666);\n-\n-\twhile (size > 0) {\n-\t\tlong ret = write_in_full(fd, buf, size);\n-\t\tif (ret < 0) {\n-\t\t\t/* Ignore epipe */\n-\t\t\tif (errno == EPIPE)\n-\t\t\t\tbreak;\n-\t\t\tdie_errno(\"notes-merge\");\n-\t\t} else if (!ret) {\n-\t\t\tdie(\"notes-merge: disk full?\");\n-\t\t}\n-\t\tsize -= ret;\n-\t\tbuf += ret;\n-\t}\n-\n+\twrite_or_die(fd, buf, size);\n \tclose(fd);\n \tfree(path);\n }\n-- \n2.9.0.393.g704e522\n\n"}]}