[PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()

[PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()

[Jeff King]

When updating sparse patterns, we open a lock_file to write out the new
data. The lock_file struct holds the file descriptor, but we call
fdopen() to get a stdio handle to do the actual write.

After we finish writing, we fflush() so that all of the data is on disk,
and then call commit_lock_file() which closes the descriptor. But we
never fclose() the stdio handle, leaking it.

The obvious solution seems like it would be to just call fclose(). But
when? If we do it before commit_lock_file(), then the lock_file code is
left thinking it owns the now-closed file descriptor, and will do an
extra close() on the descriptor. But if we do it before, we have the
opposite problem: the lock_file code will close the descriptor, and
fclose() will do the extra close().

We can handle this correctly by using fdopen_lock_file(). That leaves
ownership of the stdio handle with the lock_file, which knows not to
double-close it.

We do have to adjust the code a bit:

  - we have to handle errors ourselves; we can just die(), since that's
    what xfdopen() would have done (and we can even provide a more
    specific error message).

  - we no longer need to call fflush(); committing the lock-file
    auto-closes it, which will now do the flush for us. As a bonus, this
    will actually check that the flush was successful before renaming
    the file into place. Let's likewise report when committing the lock
    fails (rather than quietly returning success from the command).

  - we can get rid of the local "fd" variable, since we never look at it
    ourselves now

Signed-off-by: Jeff King <peff@peff.net>
---
Curiously, this is not reported as a leak by LSan, and it is included in
the "reachable" set by valgrind. I imagine this is because libc has to
maintain a list of open writable handles, since fflush(NULL) is supposed
to flush all of them. I peeked at other fdopen(..., "w") calls to see if
there were other similar spots, but didn't notice any.

I found this because I was building git on an Android system, and they
have an "fdsan" that complains when the underlying descriptor of a
handle is closed. It flagged some other spots, too, but the rest that I
looked at involved the tempfile atexit() handler closing the descriptors
manually (we don't care about flushing there, as our goal is to just
prevent the "can't delete an open file" problem on Windows).

 builtin/sparse-checkout.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 2604ab04df..f1bd31b2f7 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -327,7 +327,6 @@ static int write_patterns_and_update(struct pattern_list *pl)
 {
 	char *sparse_filename;
 	FILE *fp;
-	int fd;
 	struct lock_file lk = LOCK_INIT;
 	int result;
 
@@ -336,8 +335,7 @@ static int write_patterns_and_update(struct pattern_list *pl)
 	if (safe_create_leading_directories(sparse_filename))
 		die(_("failed to create directory for sparse-checkout file"));
 
-	fd = hold_lock_file_for_update(&lk, sparse_filename,
-				      LOCK_DIE_ON_ERROR);
+	hold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR);
 	free(sparse_filename);
 
 	result = update_working_directory(pl);
@@ -348,15 +346,17 @@ static int write_patterns_and_update(struct pattern_list *pl)
 		return result;
 	}
 
-	fp = xfdopen(fd, "w");
+	fp = fdopen_lock_file(&lk, "w");
+	if (!fp)
+		die_errno(_("unable to fdopen %s"), get_lock_file_path(&lk));
 
 	if (core_sparse_checkout_cone)
 		write_cone_to_file(fp, pl);
 	else
 		write_patterns_to_file(fp, pl);
 
-	fflush(fp);
-	commit_lock_file(&lk);
+	if (commit_lock_file(&lk))
+		die_errno(_("unable to write %s"), get_locked_file_path(&lk));
 
 	clear_pattern_list(pl);
 
-- 
2.46.0.802.g13da1a47c4

Re: [PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()

[Patrick Steinhardt]

On Thu, Sep 05, 2024 at 04:27:49AM -0400, Jeff King wrote:
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 2604ab04df..f1bd31b2f7 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -336,8 +335,7 @@ static int write_patterns_and_update(struct pattern_list *pl)
>  	if (safe_create_leading_directories(sparse_filename))
>  		die(_("failed to create directory for sparse-checkout file"));
>  
> -	fd = hold_lock_file_for_update(&lk, sparse_filename,
> -				      LOCK_DIE_ON_ERROR);
> +	hold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR);
>  	free(sparse_filename);
>  
>  	result = update_working_directory(pl);

Okay, we die on an error so there is no need to check `fd`, either.
Makes sense.

> @@ -348,15 +346,17 @@ static int write_patterns_and_update(struct pattern_list *pl)
>  		return result;
>  	}
>  
> -	fp = xfdopen(fd, "w");
> +	fp = fdopen_lock_file(&lk, "w");
> +	if (!fp)
> +		die_errno(_("unable to fdopen %s"), get_lock_file_path(&lk));
>  
>  	if (core_sparse_checkout_cone)
>  		write_cone_to_file(fp, pl);
>  	else
>  		write_patterns_to_file(fp, pl);
>  
> -	fflush(fp);
> -	commit_lock_file(&lk);
> +	if (commit_lock_file(&lk))
> +		die_errno(_("unable to write %s"), get_locked_file_path(&lk));
>  
>  	clear_pattern_list(pl);

I think the error handling is broken. `commit_lock_file()` calls
`rename_tempfile()`, which deletes the temporary file even in the error
case. The consequence is that `lk->tempfile` will be set to the `NULL`
pointer. When we call `get_locked_file_path()` we then dereference it
unconditionally and would thus segfault.

Other than that this patch looks sensible. Also kind of curious that we
never checked that `commit_lock_file()` was successful, so we wouldn't
even notice when renaming the file into place failed.

Patrick

Re: [PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We do have to adjust the code a bit:
>
>   - we have to handle errors ourselves; we can just die(), since that's
>     what xfdopen() would have done (and we can even provide a more
>     specific error message).
>
>   - we no longer need to call fflush(); committing the lock-file
>     auto-closes it, which will now do the flush for us. As a bonus, this
>     will actually check that the flush was successful before renaming
>     the file into place. Let's likewise report when committing the lock
>     fails (rather than quietly returning success from the command).
>
>   - we can get rid of the local "fd" variable, since we never look at it
>     ourselves now

OK.  The neessary change is surprisingly small.

> I found this because I was building git on an Android system,...

Sounds like fun.

Will queue.  Thanks.

Re: [PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> +	if (commit_lock_file(&lk))
>> +		die_errno(_("unable to write %s"), get_locked_file_path(&lk));
>>  
>>  	clear_pattern_list(pl);
>
> I think the error handling is broken. `commit_lock_file()` calls
> `rename_tempfile()`, which deletes the temporary file even in the error
> case. The consequence is that `lk->tempfile` will be set to the `NULL`
> pointer. When we call `get_locked_file_path()` we then dereference it
> unconditionally and would thus segfault.

Hmph.  Would this be sufficient as a band-aid, then?

 builtin/sparse-checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git c/builtin/sparse-checkout.c w/builtin/sparse-checkout.c
index f1bd31b2f7..60363fd056 100644
--- c/builtin/sparse-checkout.c
+++ w/builtin/sparse-checkout.c
@@ -356,7 +356,7 @@ static int write_patterns_and_update(struct pattern_list *pl)
 		write_patterns_to_file(fp, pl);
 
 	if (commit_lock_file(&lk))
-		die_errno(_("unable to write %s"), get_locked_file_path(&lk));
+		die_errno(_("unable to write %s"), sparse_filename);
 
 	clear_pattern_list(pl);
 

Re: [PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>>> +	if (commit_lock_file(&lk))
>>> +		die_errno(_("unable to write %s"), get_locked_file_path(&lk));
>>>  
>>>  	clear_pattern_list(pl);
>>
>> I think the error handling is broken. `commit_lock_file()` calls
>> `rename_tempfile()`, which deletes the temporary file even in the error
>> case. The consequence is that `lk->tempfile` will be set to the `NULL`
>> pointer. When we call `get_locked_file_path()` we then dereference it
>> unconditionally and would thus segfault.
>
> Hmph.  Would this be sufficient as a band-aid, then?

Of course not.  That would refer to a piece of memory that we
already free'ed in this function.

Perhaps like this?

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index f1bd31b2f7..27d181a612 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -328,7 +328,7 @@ static int write_patterns_and_update(struct pattern_list *pl)
 	char *sparse_filename;
 	FILE *fp;
 	struct lock_file lk = LOCK_INIT;
-	int result;
+	int result = 0;
 
 	sparse_filename = get_sparse_checkout_filename();
 
@@ -336,19 +336,18 @@ static int write_patterns_and_update(struct pattern_list *pl)
 		die(_("failed to create directory for sparse-checkout file"));
 
 	hold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR);
-	free(sparse_filename);
 
 	result = update_working_directory(pl);
 	if (result) {
 		rollback_lock_file(&lk);
 		clear_pattern_list(pl);
 		update_working_directory(NULL);
-		return result;
+		goto out;
 	}
 
 	fp = fdopen_lock_file(&lk, "w");
 	if (!fp)
-		die_errno(_("unable to fdopen %s"), get_lock_file_path(&lk));
+		die_errno(_("unable to fdopen %s"), sparse_filename);
 
 	if (core_sparse_checkout_cone)
 		write_cone_to_file(fp, pl);
@@ -356,11 +355,13 @@ static int write_patterns_and_update(struct pattern_list *pl)
 		write_patterns_to_file(fp, pl);
 
 	if (commit_lock_file(&lk))
-		die_errno(_("unable to write %s"), get_locked_file_path(&lk));
+		die_errno(_("unable to write %s"), sparse_filename);
 
 	clear_pattern_list(pl);
 
-	return 0;
+out:
+	free(sparse_filename);
+	return result;
 }
 
 enum sparse_checkout_mode {

Re: [PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()

[Jeff King]

On Thu, Sep 05, 2024 at 02:20:18PM -0700, Junio C Hamano wrote:

> >> I think the error handling is broken. `commit_lock_file()` calls
> >> `rename_tempfile()`, which deletes the temporary file even in the error
> >> case. The consequence is that `lk->tempfile` will be set to the `NULL`
> >> pointer. When we call `get_locked_file_path()` we then dereference it
> >> unconditionally and would thus segfault.
> >
> > Hmph.  Would this be sufficient as a band-aid, then?
> 
> Of course not.  That would refer to a piece of memory that we
> already free'ed in this function.
> 
> Perhaps like this?

That works, though...

>  	fp = fdopen_lock_file(&lk, "w");
>  	if (!fp)
> -		die_errno(_("unable to fdopen %s"), get_lock_file_path(&lk));
> +		die_errno(_("unable to fdopen %s"), sparse_filename);
>  
>  	if (core_sparse_checkout_cone)
>  		write_cone_to_file(fp, pl);
> @@ -356,11 +355,13 @@ static int write_patterns_and_update(struct pattern_list *pl)
>  		write_patterns_to_file(fp, pl);
>  
>  	if (commit_lock_file(&lk))
> -		die_errno(_("unable to write %s"), get_locked_file_path(&lk));
> +		die_errno(_("unable to write %s"), sparse_filename);

Note the difference between "get_lock" and "get_locked" in these two.
The first will mention the tempfile name, and the second the destination
filename (and sparse_filename is the latter).

I don't know that it really matters much in practice, though. If
fdopen() fails it probably has nothing to do with the file itself, and
is either lack of memory or a programming bug (e.g., bogus descriptor).

I'll pick up this change, but I'll split the whole error-handling fix
into its own patch, since it's getting more complicated.

Will send v2 later tonight. Thanks, Patrick, for noticing the problem in
the first place.

-Peff

[PATCH v2 0/3] sparse-checkout file handle leak fix

[Jeff King]

Here's a re-roll that fixes the use-after-free of the lock filename that
Patrick noticed. I pulled the error-checking fix into its own patch
(patch 2 here), and did Junio's suggested "goto out" as preparation in
patch 1. Patch 3 is the leak fix.

Range diff is below, but it's much harder to read than just looking at
the updated patch 3.

Thanks both for review on round 1.

  [1/3]: sparse-checkout: consolidate cleanup when writing patterns
  [2/3]: sparse-checkout: check commit_lock_file when writing patterns
  [3/3]: sparse-checkout: use fdopen_lock_file() instead of xfdopen()

 builtin/sparse-checkout.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

-:  ---------- > 1:  03a80d3748 sparse-checkout: consolidate cleanup when writing patterns
-:  ---------- > 2:  20628aa350 sparse-checkout: check commit_lock_file when writing patterns
1:  b4700ba1a9 ! 3:  fe41f9f02b sparse-checkout: use fdopen_lock_file() instead of xfdopen()
    @@ Commit message
           - we no longer need to call fflush(); committing the lock-file
             auto-closes it, which will now do the flush for us. As a bonus, this
             will actually check that the flush was successful before renaming
    -        the file into place. Let's likewise report when committing the lock
    -        fails (rather than quietly returning success from the command).
    +        the file into place.
     
           - we can get rid of the local "fd" variable, since we never look at it
             ourselves now
    @@ builtin/sparse-checkout.c: static int write_patterns_and_update(struct pattern_l
     -	fd = hold_lock_file_for_update(&lk, sparse_filename,
     -				      LOCK_DIE_ON_ERROR);
     +	hold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR);
    - 	free(sparse_filename);
      
      	result = update_working_directory(pl);
    + 	if (result) {
     @@ builtin/sparse-checkout.c: static int write_patterns_and_update(struct pattern_list *pl)
    - 		return result;
    + 		goto out;
      	}
      
     -	fp = xfdopen(fd, "w");
    @@ builtin/sparse-checkout.c: static int write_patterns_and_update(struct pattern_l
      		write_patterns_to_file(fp, pl);
      
     -	fflush(fp);
    --	commit_lock_file(&lk);
    -+	if (commit_lock_file(&lk))
    -+		die_errno(_("unable to write %s"), get_locked_file_path(&lk));
    - 
    - 	clear_pattern_list(pl);
    + 	if (commit_lock_file(&lk))
    + 		die_errno(_("unable to write %s"), sparse_filename);
      

[PATCH v2 1/3] sparse-checkout: consolidate cleanup when writing patterns

[Jeff King]

In write_patterns_and_update(), we always need to free the pattern list
before exiting the function.  Rather than handling it manually when we
return early, we can jump to an "out" label where cleanup happens. This
let us drop one line, but also establishes a pattern we can use for
other cleanup.

Signed-off-by: Jeff King <peff@peff.net>
---
This would also be helpful in the long run if all of the other die()
calls ended up as error returns, but I guess this is in builtin/, so it
may not be that likely a candidate for libification anyway.

 builtin/sparse-checkout.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 2604ab04df..dfefe609a1 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -343,9 +343,8 @@ static int write_patterns_and_update(struct pattern_list *pl)
 	result = update_working_directory(pl);
 	if (result) {
 		rollback_lock_file(&lk);
-		clear_pattern_list(pl);
 		update_working_directory(NULL);
-		return result;
+		goto out;
 	}
 
 	fp = xfdopen(fd, "w");
@@ -358,9 +357,9 @@ static int write_patterns_and_update(struct pattern_list *pl)
 	fflush(fp);
 	commit_lock_file(&lk);
 
+out:
 	clear_pattern_list(pl);
-
-	return 0;
+	return result;
 }
 
 enum sparse_checkout_mode {
-- 
2.46.0.802.g13da1a47c4

[PATCH v2 2/3] sparse-checkout: check commit_lock_file when writing patterns

[Jeff King]

When writing a new "sparse-checkout" file, we do the usual strategy of
writing to a lockfile and committing it into place. But we don't check
the outcome of commit_lock_file(). Failing there would prevent us from
writing a bogus file (good), but we would ignore the error and return a
successful exit code (bad).

Fix this by calling die(). Note that we need to keep the sparse_filename
variable valid for longer, since the filename stored in the lock_file
struct will be dropped when we run commit_lock_file().

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/sparse-checkout.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index dfefe609a1..b5e220cc44 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -338,7 +338,6 @@ static int write_patterns_and_update(struct pattern_list *pl)
 
 	fd = hold_lock_file_for_update(&lk, sparse_filename,
 				      LOCK_DIE_ON_ERROR);
-	free(sparse_filename);
 
 	result = update_working_directory(pl);
 	if (result) {
@@ -355,10 +354,12 @@ static int write_patterns_and_update(struct pattern_list *pl)
 		write_patterns_to_file(fp, pl);
 
 	fflush(fp);
-	commit_lock_file(&lk);
+	if (commit_lock_file(&lk))
+		die_errno(_("unable to write %s"), sparse_filename);
 
 out:
 	clear_pattern_list(pl);
+	free(sparse_filename);
 	return result;
 }
 
-- 
2.46.0.802.g13da1a47c4

[PATCH v2 3/3] sparse-checkout: use fdopen_lock_file() instead of xfdopen()

[Jeff King]

When updating sparse patterns, we open a lock_file to write out the new
data. The lock_file struct holds the file descriptor, but we call
fdopen() to get a stdio handle to do the actual write.

After we finish writing, we fflush() so that all of the data is on disk,
and then call commit_lock_file() which closes the descriptor. But we
never fclose() the stdio handle, leaking it.

The obvious solution seems like it would be to just call fclose(). But
when? If we do it before commit_lock_file(), then the lock_file code is
left thinking it owns the now-closed file descriptor, and will do an
extra close() on the descriptor. But if we do it before, we have the
opposite problem: the lock_file code will close the descriptor, and
fclose() will do the extra close().

We can handle this correctly by using fdopen_lock_file(). That leaves
ownership of the stdio handle with the lock_file, which knows not to
double-close it.

We do have to adjust the code a bit:

  - we have to handle errors ourselves; we can just die(), since that's
    what xfdopen() would have done (and we can even provide a more
    specific error message).

  - we no longer need to call fflush(); committing the lock-file
    auto-closes it, which will now do the flush for us. As a bonus, this
    will actually check that the flush was successful before renaming
    the file into place.

  - we can get rid of the local "fd" variable, since we never look at it
    ourselves now

Signed-off-by: Jeff King <peff@peff.net>
---
I left this error message using get_lock_file_path(&lk), which is still
valid, and is arguably correct than sparse_filename.

 builtin/sparse-checkout.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index b5e220cc44..5ccf696862 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -327,7 +327,6 @@ static int write_patterns_and_update(struct pattern_list *pl)
 {
 	char *sparse_filename;
 	FILE *fp;
-	int fd;
 	struct lock_file lk = LOCK_INIT;
 	int result;
 
@@ -336,8 +335,7 @@ static int write_patterns_and_update(struct pattern_list *pl)
 	if (safe_create_leading_directories(sparse_filename))
 		die(_("failed to create directory for sparse-checkout file"));
 
-	fd = hold_lock_file_for_update(&lk, sparse_filename,
-				      LOCK_DIE_ON_ERROR);
+	hold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR);
 
 	result = update_working_directory(pl);
 	if (result) {
@@ -346,14 +344,15 @@ static int write_patterns_and_update(struct pattern_list *pl)
 		goto out;
 	}
 
-	fp = xfdopen(fd, "w");
+	fp = fdopen_lock_file(&lk, "w");
+	if (!fp)
+		die_errno(_("unable to fdopen %s"), get_lock_file_path(&lk));
 
 	if (core_sparse_checkout_cone)
 		write_cone_to_file(fp, pl);
 	else
 		write_patterns_to_file(fp, pl);
 
-	fflush(fp);
 	if (commit_lock_file(&lk))
 		die_errno(_("unable to write %s"), sparse_filename);
 
-- 
2.46.0.802.g13da1a47c4

Re: [PATCH v2 0/3] sparse-checkout file handle leak fix

[Patrick Steinhardt]

On Thu, Sep 05, 2024 at 11:45:57PM -0400, Jeff King wrote:
> Here's a re-roll that fixes the use-after-free of the lock filename that
> Patrick noticed. I pulled the error-checking fix into its own patch
> (patch 2 here), and did Junio's suggested "goto out" as preparation in
> patch 1. Patch 3 is the leak fix.
> 
> Range diff is below, but it's much harder to read than just looking at
> the updated patch 3.
> 
> Thanks both for review on round 1.

Thanks for going the extra steps! This version looks good to me.

Patrick

Re: [PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>>  	fp = fdopen_lock_file(&lk, "w");
>>  	if (!fp)
>> -		die_errno(_("unable to fdopen %s"), get_lock_file_path(&lk));
>> +		die_errno(_("unable to fdopen %s"), sparse_filename);
>>  
>>  	if (core_sparse_checkout_cone)
>>  		write_cone_to_file(fp, pl);
>> @@ -356,11 +355,13 @@ static int write_patterns_and_update(struct pattern_list *pl)
>>  		write_patterns_to_file(fp, pl);
>>  
>>  	if (commit_lock_file(&lk))
>> -		die_errno(_("unable to write %s"), get_locked_file_path(&lk));
>> +		die_errno(_("unable to write %s"), sparse_filename);
>
> Note the difference between "get_lock" and "get_locked" in these two.
> The first will mention the tempfile name, and the second the destination
> filename (and sparse_filename is the latter).

I did consider ... to write %s.%s", sparse_filename, LOCK_SUFFIX)
but then thought that the final name is what is more relevant to the
end user.  Yes, while correcting unrelated error, I shouldn't have
tried to improve unrelated end-user experience ;-).

> Will send v2 later tonight. Thanks, Patrick, for noticing the problem in
> the first place.

Yeah, thanks, both.