From 03b95333db32ea92b8a4a61f2f055a900f8d7c84 Mon Sep 17 00:00:00 2001 From: Lars Schneider Date: Thu, 5 Oct 2017 12:44:06 +0200 Subject: entry.c: update cache entry only for existing files In 2841e8f ("convert: add "status=delayed" to filter process protocol", 2017-06-30) we taught the filter process protocol to delay responses. That means an external filter might answer in the first write_entry() call on a file that requires filtering "I got your request, but I can't answer right now. Ask again later!". As Git got no answer, we do not write anything to the filesystem. Consequently, the lstat() call in the finish block of the function writes garbage to the cache entry. The garbage is eventually overwritten when the filter answers with the final file content in a subsequent write_entry() call. Fix the brief time window of garbage in the cache entry by adding a special finish block that does nothing for delayed responses. The cache entry is written properly in a subsequent write_entry() call where the filter responds with the final file content. Reported-by: Jeff King Signed-off-by: Lars Schneider Signed-off-by: Junio C Hamano --- entry.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'entry.c') diff --git a/entry.c b/entry.c index 65458f07a4..f879758c73 100644 --- a/entry.c +++ b/entry.c @@ -290,7 +290,7 @@ static int write_entry(struct cache_entry *ce, ce->name, new, size, &buf, dco); if (ret && string_list_has_string(&dco->paths, ce->name)) { free(new); - goto finish; + goto delayed; } } else ret = convert_to_working_tree( @@ -346,6 +346,7 @@ finish: ce->ce_flags |= CE_UPDATE_IN_BASE; state->istate->cache_changed |= CE_ENTRY_CHANGED; } +delayed: return 0; } -- cgit v1.2.3 From 11179eb31178d3d162e1b0af50edbdc2a14da23b Mon Sep 17 00:00:00 2001 From: Lars Schneider Date: Thu, 5 Oct 2017 12:44:07 +0200 Subject: entry.c: check if file exists after checkout If we are checking out a file and somebody else racily deletes our file, then we would write garbage to the cache entry. Fix that by checking the result of the lstat() call on that file. Print an error to the user if the file does not exist. Reported-by: Jeff King Signed-off-by: Lars Schneider Signed-off-by: Junio C Hamano --- entry.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'entry.c') diff --git a/entry.c b/entry.c index f879758c73..ab79f1f69c 100644 --- a/entry.c +++ b/entry.c @@ -341,7 +341,9 @@ finish: if (state->refresh_cache) { assert(state->istate); if (!fstat_done) - lstat(ce->name, &st); + if (lstat(ce->name, &st) < 0) + return error_errno("unable to stat just-written file %s", + ce->name); fill_stat_cache_info(ce, &st); ce->ce_flags |= CE_UPDATE_IN_BASE; state->istate->cache_changed |= CE_ENTRY_CHANGED; -- cgit v1.2.3 From b2401586fc5168974c77cdc6d8548c51e6c852a6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 9 Oct 2017 13:48:24 -0400 Subject: write_entry: fix leak when retrying delayed filter When write_entry() retries a delayed filter request, we don't need to send the blob content to the filter again, and set the pointer to NULL. But doing so means we leak the contents we read earlier from read_blob_entry(). Let's make sure to free it before dropping the pointer. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- entry.c | 1 + 1 file changed, 1 insertion(+) (limited to 'entry.c') diff --git a/entry.c b/entry.c index ab79f1f69c..637c5958b0 100644 --- a/entry.c +++ b/entry.c @@ -283,6 +283,7 @@ static int write_entry(struct cache_entry *ce, if (dco && dco->state != CE_NO_DELAY) { /* Do not send the blob in case of a retry. */ if (dco->state == CE_RETRY) { + free(new); new = NULL; size = 0; } -- cgit v1.2.3 From c602d3a9897a408ce0db543860d472332f79d045 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 9 Oct 2017 13:48:52 -0400 Subject: write_entry: avoid reading blobs in CE_RETRY case When retrying a delayed filter-process request, we don't need to send the blob to the filter a second time. However, we read it unconditionally into a buffer, only to later throw away that buffer. We can make this more efficient by skipping the read in the first place when it isn't necessary. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- entry.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) (limited to 'entry.c') diff --git a/entry.c b/entry.c index 637c5958b0..bec51e37a2 100644 --- a/entry.c +++ b/entry.c @@ -240,6 +240,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout *state, int to_tempfile) { unsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT; + struct delayed_checkout *dco = state->delayed_checkout; int fd, ret, fstat_done = 0; char *new; struct strbuf buf = STRBUF_INIT; @@ -261,10 +262,19 @@ static int write_entry(struct cache_entry *ce, switch (ce_mode_s_ifmt) { case S_IFREG: case S_IFLNK: - new = read_blob_entry(ce, &size); - if (!new) - return error("unable to read sha1 file of %s (%s)", - path, oid_to_hex(&ce->oid)); + /* + * We do not send the blob in case of a retry, so do not + * bother reading it at all. + */ + if (ce_mode_s_ifmt == S_IFREG && dco && dco->state == CE_RETRY) { + new = NULL; + size = 0; + } else { + new = read_blob_entry(ce, &size); + if (!new) + return error("unable to read sha1 file of %s (%s)", + path, oid_to_hex(&ce->oid)); + } if (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) { ret = symlink(new, path); @@ -279,14 +289,7 @@ static int write_entry(struct cache_entry *ce, * Convert from git internal format to working tree format */ if (ce_mode_s_ifmt == S_IFREG) { - struct delayed_checkout *dco = state->delayed_checkout; if (dco && dco->state != CE_NO_DELAY) { - /* Do not send the blob in case of a retry. */ - if (dco->state == CE_RETRY) { - free(new); - new = NULL; - size = 0; - } ret = async_convert_to_working_tree( ce->name, new, size, &buf, dco); if (ret && string_list_has_string(&dco->paths, ce->name)) { -- cgit v1.2.3 From 7cbbf9d6a275eb23b889158edb753d651542d5a9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 9 Oct 2017 13:50:05 -0400 Subject: write_entry: untangle symlink and regular-file cases The write_entry() function switches on the mode of the entry we're going to write out. The cases for S_IFLNK and S_IFREG are lumped together. In earlier versions of the code, this made some sense. They have a shared preamble (which reads the blob content), a short type-specific body, and a shared conclusion (which writes out the file contents; always for S_IFREG and only sometimes for S_IFLNK). But over time this has grown to make less sense. The preamble now has conditional bits for each type, and the S_IFREG body has grown a lot more complicated. It's hard to follow the logic of which code is running for which mode. Let's give each mode its own case arm. We will still share the conclusion code, which means we now jump to it with a goto. Ideally we'd pull that shared code into its own function, but it touches so much internal state in the write_entry() function that the end result is actually harder to follow than the goto. While we're here, we'll touch up a few bits of whitespace to make the beginning and endings of the cases easier to read. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- entry.c | 71 +++++++++++++++++++++++++++++++++++++---------------------------- 1 file changed, 40 insertions(+), 31 deletions(-) (limited to 'entry.c') diff --git a/entry.c b/entry.c index bec51e37a2..206363fd15 100644 --- a/entry.c +++ b/entry.c @@ -260,13 +260,31 @@ static int write_entry(struct cache_entry *ce, } switch (ce_mode_s_ifmt) { - case S_IFREG: case S_IFLNK: + new = read_blob_entry(ce, &size); + if (!new) + return error("unable to read sha1 file of %s (%s)", + path, oid_to_hex(&ce->oid)); + + /* + * We can't make a real symlink; write out a regular file entry + * with the symlink destination as its contents. + */ + if (!has_symlinks || to_tempfile) + goto write_file_entry; + + ret = symlink(new, path); + free(new); + if (ret) + return error_errno("unable to create symlink %s", path); + break; + + case S_IFREG: /* * We do not send the blob in case of a retry, so do not * bother reading it at all. */ - if (ce_mode_s_ifmt == S_IFREG && dco && dco->state == CE_RETRY) { + if (dco && dco->state == CE_RETRY) { new = NULL; size = 0; } else { @@ -276,42 +294,31 @@ static int write_entry(struct cache_entry *ce, path, oid_to_hex(&ce->oid)); } - if (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) { - ret = symlink(new, path); - free(new); - if (ret) - return error_errno("unable to create symlink %s", - path); - break; - } - /* * Convert from git internal format to working tree format */ - if (ce_mode_s_ifmt == S_IFREG) { - if (dco && dco->state != CE_NO_DELAY) { - ret = async_convert_to_working_tree( - ce->name, new, size, &buf, dco); - if (ret && string_list_has_string(&dco->paths, ce->name)) { - free(new); - goto delayed; - } - } else - ret = convert_to_working_tree( - ce->name, new, size, &buf); - - if (ret) { + if (dco && dco->state != CE_NO_DELAY) { + ret = async_convert_to_working_tree(ce->name, new, + size, &buf, dco); + if (ret && string_list_has_string(&dco->paths, ce->name)) { free(new); - new = strbuf_detach(&buf, &newsize); - size = newsize; + goto delayed; } - /* - * No "else" here as errors from convert are OK at this - * point. If the error would have been fatal (e.g. - * filter is required), then we would have died already. - */ + } else + ret = convert_to_working_tree(ce->name, new, size, &buf); + + if (ret) { + free(new); + new = strbuf_detach(&buf, &newsize); + size = newsize; } + /* + * No "else" here as errors from convert are OK at this + * point. If the error would have been fatal (e.g. + * filter is required), then we would have died already. + */ + write_file_entry: fd = open_output_fd(path, ce, to_tempfile); if (fd < 0) { free(new); @@ -326,6 +333,7 @@ static int write_entry(struct cache_entry *ce, if (wrote != size) return error("unable to write file %s", path); break; + case S_IFGITLINK: if (to_tempfile) return error("cannot create temporary submodule %s", path); @@ -337,6 +345,7 @@ static int write_entry(struct cache_entry *ce, NULL, oid_to_hex(&ce->oid), state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0); break; + default: return error("unknown file mode for %s in index", path); } -- cgit v1.2.3