[PATCH 1/2] receive-pack: use find_commit_header() in check_cert_push_options()

[PATCH 1/2] receive-pack: use find_commit_header() in check_cert_push_options()

[René Scharfe]

Use the public function find_commit_header() instead of find_header() to
simplify the code.  This is possible and safe because we're operating on
a strbuf, which is always NUL-terminated, so there is no risk of running
over the end of the buffer.  It cannot contain NUL within the buffer, as
it is built using strbuf_addstr(), only.

The string comparison becomes more complicated because we need to check
for NUL explicitly after comparing the length-limited option, but on the
flip side we don't need to clean up allocations or track the remaining
buffer length.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/receive-pack.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e36b1d619f..8ebb3a872f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -718,35 +718,29 @@ static const char *check_nonce(const char *buf, size_t len)
 static int check_cert_push_options(const struct string_list *push_options)
 {
 	const char *buf = push_cert.buf;
-	int len = push_cert.len;

-	char *option;
-	const char *next_line;
+	const char *option;
+	size_t optionlen;
 	int options_seen = 0;

 	int retval = 1;

-	if (!len)
+	if (!*buf)
 		return 1;

-	while ((option = find_header(buf, len, "push-option", &next_line))) {
-		len -= (next_line - buf);
-		buf = next_line;
+	while ((option = find_commit_header(buf, "push-option", &optionlen))) {
+		buf = option + optionlen + 1;
 		options_seen++;
 		if (options_seen > push_options->nr
-		    || strcmp(option,
-			      push_options->items[options_seen - 1].string)) {
-			retval = 0;
-			goto leave;
-		}
-		free(option);
+		    || strncmp(push_options->items[options_seen - 1].string,
+			       option, optionlen)
+		    || push_options->items[options_seen - 1].string[optionlen])
+			return 0;
 	}

 	if (options_seen != push_options->nr)
 		retval = 0;

-leave:
-	free(option);
 	return retval;
 }

--
2.43.0

[PATCH 2/2] receive-pack: use find_commit_header() in check_nonce()

[René Scharfe]

Use the public function find_commit_header() and remove find_header(),
as it becomes unused.  This is safe and appropriate because we pass the
NUL-terminated payload buffer to check_nonce() instead of its start and
length.  The underlying strbuf push_cert cannot contain NULs, as it is
built using strbuf_addstr(), only.

We no longer need to call strlen(), as find_commit_header() returns the
length of nonce already.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
Formatted with -U5 for easier review.
Removing find_header_mem(), which basically becomes unused, left as an
exercise for the reader. ;)

 builtin/receive-pack.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 8ebb3a872f..dbee508775 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -591,25 +591,10 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)
 	/* RFC 2104 5. HMAC-SHA1 or HMAC-SHA256 */
 	strbuf_addf(&buf, "%"PRItime"-%.*s", stamp, (int)the_hash_algo->hexsz, hash_to_hex(hash));
 	return strbuf_detach(&buf, NULL);
 }

-static char *find_header(const char *msg, size_t len, const char *key,
-			 const char **next_line)
-{
-	size_t out_len;
-	const char *val = find_header_mem(msg, len, key, &out_len);
-
-	if (!val)
-		return NULL;
-
-	if (next_line)
-		*next_line = val + out_len + 1;
-
-	return xmemdupz(val, out_len);
-}
-
 /*
  * Return zero if a and b are equal up to n bytes and nonzero if they are not.
  * This operation is guaranteed to run in constant time to avoid leaking data.
  */
 static int constant_memequal(const char *a, const char *b, size_t n)
@@ -620,17 +605,18 @@ static int constant_memequal(const char *a, const char *b, size_t n)
 	for (i = 0; i < n; i++)
 		res |= a[i] ^ b[i];
 	return res;
 }

-static const char *check_nonce(const char *buf, size_t len)
+static const char *check_nonce(const char *buf)
 {
-	char *nonce = find_header(buf, len, "nonce", NULL);
+	size_t noncelen;
+	const char *found = find_commit_header(buf, "nonce", &noncelen);
+	char *nonce = found ? xmemdupz(found, noncelen) : NULL;
 	timestamp_t stamp, ostamp;
 	char *bohmac, *expect = NULL;
 	const char *retval = NONCE_BAD;
-	size_t noncelen;

 	if (!nonce) {
 		retval = NONCE_MISSING;
 		goto leave;
 	} else if (!push_cert_nonce) {
@@ -668,11 +654,10 @@ static const char *check_nonce(const char *buf, size_t len)
 	if (bohmac == nonce || bohmac[0] != '-') {
 		retval = NONCE_BAD;
 		goto leave;
 	}

-	noncelen = strlen(nonce);
 	expect = prepare_push_cert_nonce(service_dir, stamp);
 	if (noncelen != strlen(expect)) {
 		/* This is not even the right size. */
 		retval = NONCE_BAD;
 		goto leave;
@@ -765,11 +750,11 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 		sigcheck.payload = xmemdupz(push_cert.buf, bogs);
 		sigcheck.payload_len = bogs;
 		check_signature(&sigcheck, push_cert.buf + bogs,
 				push_cert.len - bogs);

-		nonce_status = check_nonce(push_cert.buf, bogs);
+		nonce_status = check_nonce(sigcheck.payload);
 	}
 	if (!is_null_oid(&push_cert_oid)) {
 		strvec_pushf(&proc->env, "GIT_PUSH_CERT=%s",
 			     oid_to_hex(&push_cert_oid));
 		strvec_pushf(&proc->env, "GIT_PUSH_CERT_SIGNER=%s",
--
2.43.0

Re: [PATCH 1/2] receive-pack: use find_commit_header() in check_cert_push_options()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> The string comparison becomes more complicated because we need to check
> for NUL explicitly after comparing the length-limited option, but on the
> flip side we don't need to clean up allocations or track the remaining
> buffer length.

Yeah, the strncmp() followed by the termination check indeed is
trickier but not having to worry about allocation is nice.

>  		if (options_seen > push_options->nr
> -		    || strcmp(option,
> -			      push_options->items[options_seen - 1].string)) {

We used to allocate option[] with NUL termination, but ...

> -			retval = 0;
> -			goto leave;
> -		}
> -		free(option);
> +		    || strncmp(push_options->items[options_seen - 1].string,
> +			       option, optionlen)
> +		    || push_options->items[options_seen - 1].string[optionlen])

... now option[] is a borrowed memory, option[optionlen] would have
been NUL if we were allocating.  So to see if the last-seen string[]
is different from option[], we have to see that they match up to
optionlen and the last-seen string[] ends there.  Trickier than
before, but is correct.

> +			return 0;
>  	}
>
>  	if (options_seen != push_options->nr)
>  		retval = 0;
>
> -leave:
> -	free(option);
>  	return retval;
>  }
>
> --
> 2.43.0

Re: [PATCH 2/2] receive-pack: use find_commit_header() in check_nonce()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> @@ -620,17 +605,18 @@ static int constant_memequal(const char *a, const char *b, size_t n)
>  	for (i = 0; i < n; i++)
>  		res |= a[i] ^ b[i];
>  	return res;
>  }
>
> -static const char *check_nonce(const char *buf, size_t len)
> +static const char *check_nonce(const char *buf)
>  {
> -	char *nonce = find_header(buf, len, "nonce", NULL);
> +	size_t noncelen;
> +	const char *found = find_commit_header(buf, "nonce", &noncelen);
> +	char *nonce = found ? xmemdupz(found, noncelen) : NULL;

OK, the changes to this function are all quite trivially correct.

>  	timestamp_t stamp, ostamp;
>  	char *bohmac, *expect = NULL;
>  	const char *retval = NONCE_BAD;
> -	size_t noncelen;
>
>  	if (!nonce) {
>  		retval = NONCE_MISSING;
>  		goto leave;
>  	} else if (!push_cert_nonce) {
> @@ -668,11 +654,10 @@ static const char *check_nonce(const char *buf, size_t len)
>  	if (bohmac == nonce || bohmac[0] != '-') {
>  		retval = NONCE_BAD;
>  		goto leave;
>  	}
>
> -	noncelen = strlen(nonce);
>  	expect = prepare_push_cert_nonce(service_dir, stamp);
>  	if (noncelen != strlen(expect)) {
>  		/* This is not even the right size. */
>  		retval = NONCE_BAD;
>  		goto leave;
> @@ -765,11 +750,11 @@ static void prepare_push_cert_sha1(struct child_process *proc)
>  		sigcheck.payload = xmemdupz(push_cert.buf, bogs);
>  		sigcheck.payload_len = bogs;
>  		check_signature(&sigcheck, push_cert.buf + bogs,
>  				push_cert.len - bogs);
>
> -		nonce_status = check_nonce(push_cert.buf, bogs);
> +		nonce_status = check_nonce(sigcheck.payload);

Hmph.  sigc->payload is used as a read-only member in
check_signature(), and the xmemdupz() copy we made earlier is
readily available as a replacement for the counted (push_cert.buf,
bogs) string.  Very nice finding.

>  	}
>  	if (!is_null_oid(&push_cert_oid)) {
>  		strvec_pushf(&proc->env, "GIT_PUSH_CERT=%s",
>  			     oid_to_hex(&push_cert_oid));
>  		strvec_pushf(&proc->env, "GIT_PUSH_CERT_SIGNER=%s",
> --
> 2.43.0

Re: [PATCH 1/2] receive-pack: use find_commit_header() in check_cert_push_options()

[René Scharfe]

Am 09.02.24 um 23:11 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> The string comparison becomes more complicated because we need to check
>> for NUL explicitly after comparing the length-limited option, but on the
>> flip side we don't need to clean up allocations or track the remaining
>> buffer length.
>
> Yeah, the strncmp() followed by the termination check indeed is
> trickier but not having to worry about allocation is nice.
>
>>  		if (options_seen > push_options->nr
>> -		    || strcmp(option,
>> -			      push_options->items[options_seen - 1].string)) {
>
> We used to allocate option[] with NUL termination, but ...
>
>> -			retval = 0;
>> -			goto leave;
>> -		}
>> -		free(option);
>> +		    || strncmp(push_options->items[options_seen - 1].string,
>> +			       option, optionlen)
>> +		    || push_options->items[options_seen - 1].string[optionlen])
>
> ... now option[] is a borrowed memory, option[optionlen] would have
> been NUL if we were allocating.  So to see if the last-seen string[]
> is different from option[], we have to see that they match up to
> optionlen and the last-seen string[] ends there.  Trickier than
> before, but is correct.

I just discovered 14570dc67d (wrapper: add function to compare strings
with different NUL termination, 2020-05-25).  Perhaps squash this in to
simplify?

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

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index dbee508775..db65607485 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -717,9 +717,8 @@ static int check_cert_push_options(const struct string_list *push_options)
 		buf = option + optionlen + 1;
 		options_seen++;
 		if (options_seen > push_options->nr
-		    || strncmp(push_options->items[options_seen - 1].string,
-			       option, optionlen)
-		    || push_options->items[options_seen - 1].string[optionlen])
+		    || xstrncmpz(push_options->items[options_seen - 1].string,
+				 option, optionlen))
 			return 0;
 	}

--
2.43.0

Re: [PATCH 1/2] receive-pack: use find_commit_header() in check_cert_push_options()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> I just discovered 14570dc67d (wrapper: add function to compare strings
> with different NUL termination, 2020-05-25).  Perhaps squash this in to
> simplify?

Oooh, xstrncmpz() seems to target this exact use case.  Nice.


>
> ---
>  builtin/receive-pack.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index dbee508775..db65607485 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -717,9 +717,8 @@ static int check_cert_push_options(const struct string_list *push_options)
>  		buf = option + optionlen + 1;
>  		options_seen++;
>  		if (options_seen > push_options->nr
> -		    || strncmp(push_options->items[options_seen - 1].string,
> -			       option, optionlen)
> -		    || push_options->items[options_seen - 1].string[optionlen])
> +		    || xstrncmpz(push_options->items[options_seen - 1].string,
> +				 option, optionlen))
>  			return 0;
>  	}
>
> --
> 2.43.0

[PATCH 3/2] commit: remove find_header_mem()

[René Scharfe]

cfc5cf428b (receive-pack.c: consolidate find header logic, 2022-01-06)
introduced find_header_mem() and turned find_commit_header() into a thin
wrapper.  Since then, the latter has become the last remaining caller of
the former.  Remove it to restore find_commit_header() to the state
before cfc5cf428b, get rid of a strlen(3) call and resolve a NEEDSWORK
note in the process.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 commit.c | 16 ++--------------
 commit.h |  5 -----
 2 files changed, 2 insertions(+), 19 deletions(-)

diff --git a/commit.c b/commit.c
index 1d08951007..aacc401e72 100644
--- a/commit.c
+++ b/commit.c
@@ -1870,20 +1870,12 @@ struct commit_list **commit_list_append(struct commit *commit,
 	return &new_commit->next;
 }

-const char *find_header_mem(const char *msg, size_t len,
-			const char *key, size_t *out_len)
+const char *find_commit_header(const char *msg, const char *key, size_t *out_len)
 {
 	int key_len = strlen(key);
 	const char *line = msg;

-	/*
-	 * NEEDSWORK: It's possible for strchrnul() to scan beyond the range
-	 * given by len. However, current callers are safe because they compute
-	 * len by scanning a NUL-terminated block of memory starting at msg.
-	 * Nonetheless, it would be better to ensure the function does not look
-	 * at msg beyond the len provided by the caller.
-	 */
-	while (line && line < msg + len) {
+	while (line) {
 		const char *eol = strchrnul(line, '\n');

 		if (line == eol)
@@ -1900,10 +1892,6 @@ const char *find_header_mem(const char *msg, size_t len,
 	return NULL;
 }

-const char *find_commit_header(const char *msg, const char *key, size_t *out_len)
-{
-	return find_header_mem(msg, strlen(msg), key, out_len);
-}
 /*
  * Inspect the given string and determine the true "end" of the log message, in
  * order to find where to put a new Signed-off-by trailer.  Ignored are
diff --git a/commit.h b/commit.h
index 62fe0d77a7..3084f591fd 100644
--- a/commit.h
+++ b/commit.h
@@ -280,17 +280,12 @@ void free_commit_extra_headers(struct commit_extra_header *extra);

 /*
  * Search the commit object contents given by "msg" for the header "key".
- * Reads up to "len" bytes of "msg".
  * Returns a pointer to the start of the header contents, or NULL. The length
  * of the header, up to the first newline, is returned via out_len.
  *
  * Note that some headers (like mergetag) may be multi-line. It is the caller's
  * responsibility to parse further in this case!
  */
-const char *find_header_mem(const char *msg, size_t len,
-			const char *key,
-			size_t *out_len);
-
 const char *find_commit_header(const char *msg, const char *key,
 			       size_t *out_len);

--
2.45.2

Re: [PATCH 3/2] commit: remove find_header_mem()

[Jeff King]

On Wed, Jun 19, 2024 at 07:13:19PM +0200, René Scharfe wrote:

> cfc5cf428b (receive-pack.c: consolidate find header logic, 2022-01-06)
> introduced find_header_mem() and turned find_commit_header() into a thin
> wrapper.  Since then, the latter has become the last remaining caller of
> the former.  Remove it to restore find_commit_header() to the state
> before cfc5cf428b, get rid of a strlen(3) call and resolve a NEEDSWORK
> note in the process.

That of course made me wonder what happened to the other caller(s) of
find_header_mem(). The answer is that it went away in your 020456cb74
(receive-pack: use find_commit_header() in check_nonce(), 2024-02-09)

> -const char *find_header_mem(const char *msg, size_t len,
> -			const char *key, size_t *out_len)
> +const char *find_commit_header(const char *msg, const char *key, size_t *out_len)
>  {
>  	int key_len = strlen(key);
>  	const char *line = msg;

Not new in your patch, but assigning strlen() to int tingled my
spider-sense. It's OK, though, because "key" is always a small string
literal.

> -	/*
> -	 * NEEDSWORK: It's possible for strchrnul() to scan beyond the range
> -	 * given by len. However, current callers are safe because they compute
> -	 * len by scanning a NUL-terminated block of memory starting at msg.
> -	 * Nonetheless, it would be better to ensure the function does not look
> -	 * at msg beyond the len provided by the caller.
> -	 */
> -	while (line && line < msg + len) {
> +	while (line) {
>  		const char *eol = strchrnul(line, '\n');

OK, we no longer know the length of the message, but we don't need to
because it's NUL terminated, and strchrnul() will find the correct eol.
The length check might have saved us if we accidentally pushed "line"
past the NUL terminator, but it looks like we take care not to do so in
the loop body:

	line = *eol ? eol + 1 : NULL;

So the conversion looks good to me.

-Peff

Re: [PATCH 3/2] commit: remove find_header_mem()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +const char *find_commit_header(const char *msg, const char *key, size_t *out_len)
>>  {
>>  	int key_len = strlen(key);
>>  	const char *line = msg;
>
> Not new in your patch, but assigning strlen() to int tingled my
> spider-sense. It's OK, though, because "key" is always a small string
> literal.

Yup.  All callers of find_commit_header() give in-program constants
and never an externally sourced random string there.

> So the conversion looks good to me.

Thanks.  Will queue.