[PATCH 0/5] avoid peeking into `struct lock_file`

STALE2040d

8 messages, 3 authors, 2021-01-06 · open the first message on its own page

[PATCH 0/5] avoid peeking into `struct lock_file`

From: Martin Ågren <hidden>
Date: 2021-01-05 19:26:02

I made a comment in [1] about how we could avoid peeking into a `struct
lock_file` and instead use a helper function that we happen to have at
our disposal. I then grepped around a bit and found that we're pretty
good at avoiding such peeking at the moment, but that we could do
a bit better.

Here's a series to avoid such `lk.tempfile.foo` in favor of
`get_lock_file_foo(&lk)`.

[1] https://lore.kernel.org/git/CAN0heSrOKr--GenbowHP+iwkijbg5pCeJLq+wz6NXCXTsfcvGg@mail.gmail.com/


Martin Ågren (5):
  builtin/gc: don't peek into `struct lock_file`
  commit-graph: don't peek into `struct lock_file`
  midx: don't peek into `struct lock_file`
  refs/files-backend: don't peek into `struct lock_file`
  read-cache: try not to peek into `struct {lock_,temp}file`

 builtin/gc.c         |  6 +++---
 commit-graph.c       |  6 +++---
 midx.c               |  2 +-
 read-cache.c         | 12 ++++++------
 refs/files-backend.c |  4 ++--
 5 files changed, 15 insertions(+), 15 deletions(-)

-- 
2.30.0

[PATCH 4/5] refs/files-backend: don't peek into `struct lock_file`

From: Martin Ågren <hidden>
Date: 2021-01-05 19:26:18

Similar to the previous commits, avoid peeking into the `struct
lock_file`. Use the lock file API instead. Note how we obtain the path
to the lock file if `fdopen_lock_file()` failed and that this is not a
problem: as documented in lockfile.h, failure to "fdopen" does not roll
back the lock file and we're free to, e.g., query it for its path.

Signed-off-by: Martin Ågren <redacted>
---
 refs/files-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 04e85e7002..4fdc68810b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1824,12 +1824,12 @@ static int create_symref_locked(struct files_ref_store *refs,
 
 	if (!fdopen_lock_file(&lock->lk, "w"))
 		return error("unable to fdopen %s: %s",
-			     lock->lk.tempfile->filename.buf, strerror(errno));
+			     get_lock_file_path(&lock->lk), strerror(errno));
 
 	update_symref_reflog(refs, lock, refname, target, logmsg);
 
 	/* no error check; commit_ref will check ferror */
-	fprintf(lock->lk.tempfile->fp, "ref: %s\n", target);
+	fprintf(get_lock_file_fp(&lock->lk), "ref: %s\n", target);
 	if (commit_ref(lock) < 0)
 		return error("unable to write symref for %s: %s", refname,
 			     strerror(errno));
-- 
2.30.0

[PATCH 3/5] midx: don't peek into `struct lock_file`

From: Martin Ågren <hidden>
Date: 2021-01-05 19:26:18

Similar to the previous commits, avoid peeking into the `struct
lock_file`. Use the lock file API instead.

The two functions we're calling here double-check that the tempfile is
indeed "active", which is arguably overkill considering how we took the
lock on the line immediately above. More importantly, this future-proofs
us against, e.g., other code appearing between these two lines or the
lock file and/or tempfile internals changing.

Signed-off-by: Martin Ågren <redacted>
---
 midx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/midx.c b/midx.c
index 79c282b070..9d41b9c271 100644
--- a/midx.c
+++ b/midx.c
@@ -918,7 +918,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *
 					(pack_name_concat_len % MIDX_CHUNK_ALIGNMENT);
 
 	hold_lock_file_for_update(&lk, midx_name, LOCK_DIE_ON_ERROR);
-	f = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);
+	f = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk));
 	FREE_AND_NULL(midx_name);
 
 	if (packs.m)
-- 
2.30.0

[PATCH 2/5] commit-graph: don't peek into `struct lock_file`

From: Martin Ågren <hidden>
Date: 2021-01-05 19:26:18

Similar to the previous commit, avoid peeking into the `struct
lock_file`. Use the lock file API instead.

Signed-off-by: Martin Ågren <redacted>
---
 commit-graph.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 06f8dc1d89..031641014f 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1694,8 +1694,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 	} else {
 		hold_lock_file_for_update_mode(&lk, ctx->graph_name,
 					       LOCK_DIE_ON_ERROR, 0444);
-		fd = lk.tempfile->fd;
-		f = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);
+		fd = get_lock_file_fd(&lk);
+		f = hashfd(fd, get_lock_file_path(&lk));
 	}
 
 	chunks[0].id = GRAPH_CHUNKID_OIDFANOUT;
@@ -1833,7 +1833,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 		result = rename(ctx->graph_name, final_graph_name);
 
 		for (i = 0; i < ctx->num_commit_graphs_after; i++)
-			fprintf(lk.tempfile->fp, "%s\n", ctx->commit_graph_hash_after[i]);
+			fprintf(get_lock_file_fp(&lk), "%s\n", ctx->commit_graph_hash_after[i]);
 
 		if (result) {
 			error(_("failed to rename temporary commit-graph file"));
-- 
2.30.0

[PATCH 1/5] builtin/gc: don't peek into `struct lock_file`

From: Martin Ågren <hidden>
Date: 2021-01-05 19:26:18

A `struct lock_file` is pretty much just a wrapper around a tempfile.
But it's easy enough to avoid relying on this. Use the wrappers that the
lock file API provides rather than peeking at the temp file or even into
*its* internals.

Signed-off-by: Martin Ågren <redacted>
---
 builtin/gc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index 4c24f41852..64f2b52d6e 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -92,7 +92,7 @@ static void process_log_file(void)
 		 */
 		int saved_errno = errno;
 		fprintf(stderr, _("Failed to fstat %s: %s"),
-			get_tempfile_path(log_lock.tempfile),
+			get_lock_file_path(&log_lock),
 			strerror(saved_errno));
 		fflush(stderr);
 		commit_lock_file(&log_lock);
@@ -1518,7 +1518,7 @@ static int update_background_schedule(int run_maintenance)
 	strvec_split(&crontab_list.args, crontab_name);
 	strvec_push(&crontab_list.args, "-l");
 	crontab_list.in = -1;
-	crontab_list.out = dup(lk.tempfile->fd);
+	crontab_list.out = dup(get_lock_file_fd(&lk));
 	crontab_list.git_cmd = 0;
 
 	if (start_command(&crontab_list)) {
@@ -1533,7 +1533,7 @@ static int update_background_schedule(int run_maintenance)
 	 * Read from the .lock file, filtering out the old
 	 * schedule while appending the new schedule.
 	 */
-	cron_list = fdopen(lk.tempfile->fd, "r");
+	cron_list = fdopen(get_lock_file_fd(&lk), "r");
 	rewind(cron_list);
 
 	strvec_split(&crontab_edit.args, crontab_name);
-- 
2.30.0

[PATCH 5/5] read-cache: try not to peek into `struct {lock_,temp}file`

From: Martin Ågren <hidden>
Date: 2021-01-05 19:26:43

Similar to the previous commits, try to avoid peeking into the `struct
lock_file`. We also have some `struct tempfile`s -- let's avoid looking
into those as well.

Note that `do_write_index()` takes a tempfile and that when we call it,
we either have a tempfile which we can easily hand down, or we have a
lock file, from which we need to somehow obtain the internal tempfile.
So we need to leave that one instance of peeking-into. Nevertheless,
this commit leaves us not relying on exactly how the path of the
tempfile / lock file is stored internally.

Signed-off-by: Martin Ågren <redacted>
---
 read-cache.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index ecf6f68994..29144cf879 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -3014,10 +3014,10 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	if (ce_flush(&c, newfd, istate->oid.hash))
 		return -1;
 	if (close_tempfile_gently(tempfile)) {
-		error(_("could not close '%s'"), tempfile->filename.buf);
+		error(_("could not close '%s'"), get_tempfile_path(tempfile));
 		return -1;
 	}
-	if (stat(tempfile->filename.buf, &st))
+	if (stat(get_tempfile_path(tempfile), &st))
 		return -1;
 	istate->timestamp.sec = (unsigned int)st.st_mtime;
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
@@ -3058,10 +3058,10 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
 	 * that is associated with the given "istate".
 	 */
 	trace2_region_enter_printf("index", "do_write_index", the_repository,
-				   "%s", lock->tempfile->filename.buf);
+				   "%s", get_lock_file_path(lock));
 	ret = do_write_index(istate, lock->tempfile, 0);
 	trace2_region_leave_printf("index", "do_write_index", the_repository,
-				   "%s", lock->tempfile->filename.buf);
+				   "%s", get_lock_file_path(lock));
 
 	if (ret)
 		return ret;
@@ -3158,10 +3158,10 @@ static int write_shared_index(struct index_state *istate,
 	move_cache_to_base_index(istate);
 
 	trace2_region_enter_printf("index", "shared/do_write_index",
-				   the_repository, "%s", (*temp)->filename.buf);
+				   the_repository, "%s", get_tempfile_path(*temp));
 	ret = do_write_index(si->base, *temp, 1);
 	trace2_region_leave_printf("index", "shared/do_write_index",
-				   the_repository, "%s", (*temp)->filename.buf);
+				   the_repository, "%s", get_tempfile_path(*temp));
 
 	if (ret)
 		return ret;
-- 
2.30.0

Re: [PATCH 0/5] avoid peeking into `struct lock_file`

From: Derrick Stolee <hidden>
Date: 2021-01-06 02:16:59

On 1/5/2021 2:23 PM, Martin Ågren wrote:
I made a comment in [1] about how we could avoid peeking into a `struct
lock_file` and instead use a helper function that we happen to have at
our disposal. I then grepped around a bit and found that we're pretty
good at avoiding such peeking at the moment, but that we could do
a bit better.

Here's a series to avoid such `lk.tempfile.foo` in favor of
`get_lock_file_foo(&lk)`.

[1] https://lore.kernel.org/git/CAN0heSrOKr--GenbowHP+iwkijbg5pCeJLq+wz6NXCXTsfcvGg@mail.gmail.com/
Thanks for being diligent and keeping the code clean.

This series is good-to-go.

Reviewed-by: Derrick Stolee <redacted>

Re: [PATCH 0/5] avoid peeking into `struct lock_file`

From: Junio C Hamano <hidden>
Date: 2021-01-06 11:56:22

Derrick Stolee [off-list ref] writes:
On 1/5/2021 2:23 PM, Martin Ågren wrote:
quoted
I made a comment in [1] about how we could avoid peeking into a `struct
lock_file` and instead use a helper function that we happen to have at
our disposal. I then grepped around a bit and found that we're pretty
good at avoiding such peeking at the moment, but that we could do
a bit better.

Here's a series to avoid such `lk.tempfile.foo` in favor of
`get_lock_file_foo(&lk)`.

[1] https://lore.kernel.org/git/CAN0heSrOKr--GenbowHP+iwkijbg5pCeJLq+wz6NXCXTsfcvGg@mail.gmail.com/
Thanks for being diligent and keeping the code clean.

This series is good-to-go.

Reviewed-by: Derrick Stolee <redacted>
Thanks, both.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help