upload-pack: fix race condition in error messages

Commit Message

Derrick Stolee Aug. 10, 2023, 2:40 p.m. UTC

From: Derrick Stolee <[e-mail]>

Test t5516-fetch-push.sh has a test 'deny fetch unreachable SHA1,
allowtipsha1inwant=true' that checks stderr for a specific error
string from the remote. In some build environments the error sent
over the remote connection gets mingled with the error from the
die() statement. Since both signals are being output to the same
file descriptor (but from parent and child processes), the output
we are matching with grep gets split.

To reduce the risk of this failure, follow this process instead:

1. Write an error message to stderr.
2. Write an error message across the connection.
3. exit(1).

This reorders the events so the error is written entirely before
the client receives a message from the remote, removing the race
condition.

Signed-off-by: Derrick Stolee <[e-mail]>
---
    upload-pack: fix race condition in error messages
    
    Here is another quick patch that we've been holding in the microsoft/git
    fork for years because it helped prevent some test flakiness, especially
    in our more involved functional test environment.
    
    Thanks, -Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1572%2Fderrickstolee%2Fupload-pack-race-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1572/derrickstolee/upload-pack-race-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1572

 upload-pack.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)


base-commit: a82fb66fed250e16d3010c75404503bea3f0ab61

Comments

Junio C Hamano Aug. 10, 2023, 4:14 p.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> writes:

> To reduce the risk of this failure, follow this process instead:
>
> 1. Write an error message to stderr.
> 2. Write an error message across the connection.
> 3. exit(1).
>
> This reorders the events so the error is written entirely before
> the client receives a message from the remote, removing the race
> condition.

Makes sense.  error() eventually goes to our own vreportf() that
flushes, so by the time the call returns, our message should be
already out, and the other message has no chance to be mixed with
it.

Will queue.  Thanks.

> Signed-off-by: Derrick Stolee <[e-mail]>
> ---
>     upload-pack: fix race condition in error messages
>     
>     Here is another quick patch that we've been holding in the microsoft/git
>     fork for years because it helped prevent some test flakiness, especially
>     in our more involved functional test environment.
>     
>     Thanks, -Stolee
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1572%2Fderrickstolee%2Fupload-pack-race-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1572/derrickstolee/upload-pack-race-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1572
>
>  upload-pack.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index 94751477ab2..7b25129f0f6 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -801,11 +801,12 @@ error:
>  	for (i = 0; i < data->want_obj.nr; i++) {
>  		struct object *o = data->want_obj.objects[i].item;
>  		if (!is_our_ref(o, data->allow_uor)) {
> +			error("git upload-pack: not our ref %s",
> +			      oid_to_hex(&o->oid));
>  			packet_writer_error(&data->writer,
>  					    "upload-pack: not our ref %s",
>  					    oid_to_hex(&o->oid));
> -			die("git upload-pack: not our ref %s",
> -			    oid_to_hex(&o->oid));
> +			exit(1);
>  		}
>  	}
>  }
>
> base-commit: a82fb66fed250e16d3010c75404503bea3f0ab61

Patch

diff --git a/upload-pack.c b/upload-pack.c
index 94751477ab2..7b25129f0f6 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -801,11 +801,12 @@  error:
 	for (i = 0; i < data->want_obj.nr; i++) {
 		struct object *o = data->want_obj.objects[i].item;
 		if (!is_our_ref(o, data->allow_uor)) {
+			error("git upload-pack: not our ref %s",
+			      oid_to_hex(&o->oid));
 			packet_writer_error(&data->writer,
 					    "upload-pack: not our ref %s",
 					    oid_to_hex(&o->oid));
-			die("git upload-pack: not our ref %s",
-			    oid_to_hex(&o->oid));
+			exit(1);
 		}
 	}
 }