{"thread":{"id":"60888","subject":"[PATCH 1/2] receive-pack: use find_commit_header() in check_cert_push_options()","startedAt":"2024-02-09T20:37:00Z","lastAt":"2024-06-20T18:12:42Z","messageCount":9,"participants":["René Scharfe","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"488316","messageId":"ff0db7e3-abce-44ea-a1e3-16e1fdaf4c75@web.de","threadId":"60888","inReplyTo":null,"subject":"[PATCH 1/2] receive-pack: use find_commit_header() in check_cert_push_options()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-09T20:36:40Z","receivedAt":"2024-02-09T20:37:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use the public function find_commit_header() instead of find_header() to\nsimplify the code.  This is possible and safe because we're operating on\na strbuf, which is always NUL-terminated, so there is no risk of running\nover the end of the buffer.  It cannot contain NUL within the buffer, as\nit is built using strbuf_addstr(), only.\n\nThe string comparison becomes more complicated because we need to check\nfor NUL explicitly after comparing the length-limited option, but on the\nflip side we don't need to clean up allocations or track the remaining\nbuffer length.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/receive-pack.c | 24 +++++++++---------------\n 1 file changed, 9 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e36b1d619f..8ebb3a872f 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -718,35 +718,29 @@ static const char *check_nonce(const char *buf, size_t len)\n static int check_cert_push_options(const struct string_list *push_options)\n {\n \tconst char *buf = push_cert.buf;\n-\tint len = push_cert.len;\n\n-\tchar *option;\n-\tconst char *next_line;\n+\tconst char *option;\n+\tsize_t optionlen;\n \tint options_seen = 0;\n\n \tint retval = 1;\n\n-\tif (!len)\n+\tif (!*buf)\n \t\treturn 1;\n\n-\twhile ((option = find_header(buf, len, \"push-option\", &next_line))) {\n-\t\tlen -= (next_line - buf);\n-\t\tbuf = next_line;\n+\twhile ((option = find_commit_header(buf, \"push-option\", &optionlen))) {\n+\t\tbuf = option + optionlen + 1;\n \t\toptions_seen++;\n \t\tif (options_seen > push_options->nr\n-\t\t    || strcmp(option,\n-\t\t\t      push_options->items[options_seen - 1].string)) {\n-\t\t\tretval = 0;\n-\t\t\tgoto leave;\n-\t\t}\n-\t\tfree(option);\n+\t\t    || strncmp(push_options->items[options_seen - 1].string,\n+\t\t\t       option, optionlen)\n+\t\t    || push_options->items[options_seen - 1].string[optionlen])\n+\t\t\treturn 0;\n \t}\n\n \tif (options_seen != push_options->nr)\n \t\tretval = 0;\n\n-leave:\n-\tfree(option);\n \treturn retval;\n }\n\n--\n2.43.0\n"},{"id":"488317","messageId":"8b350cae-2180-4ac7-a911-d40043576445@web.de","threadId":"60888","inReplyTo":"ff0db7e3-abce-44ea-a1e3-16e1fdaf4c75@web.de","subject":"[PATCH 2/2] receive-pack: use find_commit_header() in check_nonce()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-09T20:41:47Z","receivedAt":"2024-02-09T20:42:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use the public function find_commit_header() and remove find_header(),\nas it becomes unused.  This is safe and appropriate because we pass the\nNUL-terminated payload buffer to check_nonce() instead of its start and\nlength.  The underlying strbuf push_cert cannot contain NULs, as it is\nbuilt using strbuf_addstr(), only.\n\nWe no longer need to call strlen(), as find_commit_header() returns the\nlength of nonce already.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nFormatted with -U5 for easier review.\nRemoving find_header_mem(), which basically becomes unused, left as an\nexercise for the reader. ;)\n\n builtin/receive-pack.c | 25 +++++--------------------\n 1 file changed, 5 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 8ebb3a872f..dbee508775 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -591,25 +591,10 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)\n \t/* RFC 2104 5. HMAC-SHA1 or HMAC-SHA256 */\n \tstrbuf_addf(&buf, \"%\"PRItime\"-%.*s\", stamp, (int)the_hash_algo->hexsz, hash_to_hex(hash));\n \treturn strbuf_detach(&buf, NULL);\n }\n\n-static char *find_header(const char *msg, size_t len, const char *key,\n-\t\t\t const char **next_line)\n-{\n-\tsize_t out_len;\n-\tconst char *val = find_header_mem(msg, len, key, &out_len);\n-\n-\tif (!val)\n-\t\treturn NULL;\n-\n-\tif (next_line)\n-\t\t*next_line = val + out_len + 1;\n-\n-\treturn xmemdupz(val, out_len);\n-}\n-\n /*\n  * Return zero if a and b are equal up to n bytes and nonzero if they are not.\n  * This operation is guaranteed to run in constant time to avoid leaking data.\n  */\n static int constant_memequal(const char *a, const char *b, size_t n)\n@@ -620,17 +605,18 @@ static int constant_memequal(const char *a, const char *b, size_t n)\n \tfor (i = 0; i < n; i++)\n \t\tres |= a[i] ^ b[i];\n \treturn res;\n }\n\n-static const char *check_nonce(const char *buf, size_t len)\n+static const char *check_nonce(const char *buf)\n {\n-\tchar *nonce = find_header(buf, len, \"nonce\", NULL);\n+\tsize_t noncelen;\n+\tconst char *found = find_commit_header(buf, \"nonce\", &noncelen);\n+\tchar *nonce = found ? xmemdupz(found, noncelen) : NULL;\n \ttimestamp_t stamp, ostamp;\n \tchar *bohmac, *expect = NULL;\n \tconst char *retval = NONCE_BAD;\n-\tsize_t noncelen;\n\n \tif (!nonce) {\n \t\tretval = NONCE_MISSING;\n \t\tgoto leave;\n \t} else if (!push_cert_nonce) {\n@@ -668,11 +654,10 @@ static const char *check_nonce(const char *buf, size_t len)\n \tif (bohmac == nonce || bohmac[0] != '-') {\n \t\tretval = NONCE_BAD;\n \t\tgoto leave;\n \t}\n\n-\tnoncelen = strlen(nonce);\n \texpect = prepare_push_cert_nonce(service_dir, stamp);\n \tif (noncelen != strlen(expect)) {\n \t\t/* This is not even the right size. */\n \t\tretval = NONCE_BAD;\n \t\tgoto leave;\n@@ -765,11 +750,11 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n \t\tsigcheck.payload = xmemdupz(push_cert.buf, bogs);\n \t\tsigcheck.payload_len = bogs;\n \t\tcheck_signature(&sigcheck, push_cert.buf + bogs,\n \t\t\t\tpush_cert.len - bogs);\n\n-\t\tnonce_status = check_nonce(push_cert.buf, bogs);\n+\t\tnonce_status = check_nonce(sigcheck.payload);\n \t}\n \tif (!is_null_oid(&push_cert_oid)) {\n \t\tstrvec_pushf(&proc->env, \"GIT_PUSH_CERT=%s\",\n \t\t\t     oid_to_hex(&push_cert_oid));\n \t\tstrvec_pushf(&proc->env, \"GIT_PUSH_CERT_SIGNER=%s\",\n--\n2.43.0\n"},{"id":"488323","messageId":"xmqqsf21cmp4.fsf@gitster.g","threadId":"60888","inReplyTo":"ff0db7e3-abce-44ea-a1e3-16e1fdaf4c75@web.de","subject":"Re: [PATCH 1/2] receive-pack: use find_commit_header() in check_cert_push_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-09T22:11:35Z","receivedAt":"2024-02-09T22:11:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> The string comparison becomes more complicated because we need to check\n> for NUL explicitly after comparing the length-limited option, but on the\n> flip side we don't need to clean up allocations or track the remaining\n> buffer length.\n\nYeah, the strncmp() followed by the termination check indeed is\ntrickier but not having to worry about allocation is nice.\n\n>  \t\tif (options_seen > push_options->nr\n> -\t\t    || strcmp(option,\n> -\t\t\t      push_options->items[options_seen - 1].string)) {\n\nWe used to allocate option[] with NUL termination, but ...\n\n> -\t\t\tretval = 0;\n> -\t\t\tgoto leave;\n> -\t\t}\n> -\t\tfree(option);\n> +\t\t    || strncmp(push_options->items[options_seen - 1].string,\n> +\t\t\t       option, optionlen)\n> +\t\t    || push_options->items[options_seen - 1].string[optionlen])\n\n... now option[] is a borrowed memory, option[optionlen] would have\nbeen NUL if we were allocating.  So to see if the last-seen string[]\nis different from option[], we have to see that they match up to\noptionlen and the last-seen string[] ends there.  Trickier than\nbefore, but is correct.\n\n> +\t\t\treturn 0;\n>  \t}\n>\n>  \tif (options_seen != push_options->nr)\n>  \t\tretval = 0;\n>\n> -leave:\n> -\tfree(option);\n>  \treturn retval;\n>  }\n>\n> --\n> 2.43.0\n"},{"id":"488324","messageId":"xmqqmss9cmdt.fsf@gitster.g","threadId":"60888","inReplyTo":"8b350cae-2180-4ac7-a911-d40043576445@web.de","subject":"Re: [PATCH 2/2] receive-pack: use find_commit_header() in check_nonce()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-09T22:18:22Z","receivedAt":"2024-02-09T22:18:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> @@ -620,17 +605,18 @@ static int constant_memequal(const char *a, const char *b, size_t n)\n>  \tfor (i = 0; i < n; i++)\n>  \t\tres |= a[i] ^ b[i];\n>  \treturn res;\n>  }\n>\n> -static const char *check_nonce(const char *buf, size_t len)\n> +static const char *check_nonce(const char *buf)\n>  {\n> -\tchar *nonce = find_header(buf, len, \"nonce\", NULL);\n> +\tsize_t noncelen;\n> +\tconst char *found = find_commit_header(buf, \"nonce\", &noncelen);\n> +\tchar *nonce = found ? xmemdupz(found, noncelen) : NULL;\n\nOK, the changes to this function are all quite trivially correct.\n\n>  \ttimestamp_t stamp, ostamp;\n>  \tchar *bohmac, *expect = NULL;\n>  \tconst char *retval = NONCE_BAD;\n> -\tsize_t noncelen;\n>\n>  \tif (!nonce) {\n>  \t\tretval = NONCE_MISSING;\n>  \t\tgoto leave;\n>  \t} else if (!push_cert_nonce) {\n> @@ -668,11 +654,10 @@ static const char *check_nonce(const char *buf, size_t len)\n>  \tif (bohmac == nonce || bohmac[0] != '-') {\n>  \t\tretval = NONCE_BAD;\n>  \t\tgoto leave;\n>  \t}\n>\n> -\tnoncelen = strlen(nonce);\n>  \texpect = prepare_push_cert_nonce(service_dir, stamp);\n>  \tif (noncelen != strlen(expect)) {\n>  \t\t/* This is not even the right size. */\n>  \t\tretval = NONCE_BAD;\n>  \t\tgoto leave;\n> @@ -765,11 +750,11 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n>  \t\tsigcheck.payload = xmemdupz(push_cert.buf, bogs);\n>  \t\tsigcheck.payload_len = bogs;\n>  \t\tcheck_signature(&sigcheck, push_cert.buf + bogs,\n>  \t\t\t\tpush_cert.len - bogs);\n>\n> -\t\tnonce_status = check_nonce(push_cert.buf, bogs);\n> +\t\tnonce_status = check_nonce(sigcheck.payload);\n\nHmph.  sigc->payload is used as a read-only member in\ncheck_signature(), and the xmemdupz() copy we made earlier is\nreadily available as a replacement for the counted (push_cert.buf,\nbogs) string.  Very nice finding.\n\n>  \t}\n>  \tif (!is_null_oid(&push_cert_oid)) {\n>  \t\tstrvec_pushf(&proc->env, \"GIT_PUSH_CERT=%s\",\n>  \t\t\t     oid_to_hex(&push_cert_oid));\n>  \t\tstrvec_pushf(&proc->env, \"GIT_PUSH_CERT_SIGNER=%s\",\n> --\n> 2.43.0\n"},{"id":"488334","messageId":"7b0a87f1-4693-480e-90d4-14675e9a5f01@web.de","threadId":"60888","inReplyTo":"xmqqsf21cmp4.fsf@gitster.g","subject":"Re: [PATCH 1/2] receive-pack: use find_commit_header() in check_cert_push_options()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-02-10T07:42:48Z","receivedAt":"2024-02-10T07:43:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.02.24 um 23:11 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> The string comparison becomes more complicated because we need to check\n>> for NUL explicitly after comparing the length-limited option, but on the\n>> flip side we don't need to clean up allocations or track the remaining\n>> buffer length.\n>\n> Yeah, the strncmp() followed by the termination check indeed is\n> trickier but not having to worry about allocation is nice.\n>\n>>  \t\tif (options_seen > push_options->nr\n>> -\t\t    || strcmp(option,\n>> -\t\t\t      push_options->items[options_seen - 1].string)) {\n>\n> We used to allocate option[] with NUL termination, but ...\n>\n>> -\t\t\tretval = 0;\n>> -\t\t\tgoto leave;\n>> -\t\t}\n>> -\t\tfree(option);\n>> +\t\t    || strncmp(push_options->items[options_seen - 1].string,\n>> +\t\t\t       option, optionlen)\n>> +\t\t    || push_options->items[options_seen - 1].string[optionlen])\n>\n> ... now option[] is a borrowed memory, option[optionlen] would have\n> been NUL if we were allocating.  So to see if the last-seen string[]\n> is different from option[], we have to see that they match up to\n> optionlen and the last-seen string[] ends there.  Trickier than\n> before, but is correct.\n\nI just discovered 14570dc67d (wrapper: add function to compare strings\nwith different NUL termination, 2020-05-25).  Perhaps squash this in to\nsimplify?\n\n---\n builtin/receive-pack.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex dbee508775..db65607485 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -717,9 +717,8 @@ static int check_cert_push_options(const struct string_list *push_options)\n \t\tbuf = option + optionlen + 1;\n \t\toptions_seen++;\n \t\tif (options_seen > push_options->nr\n-\t\t    || strncmp(push_options->items[options_seen - 1].string,\n-\t\t\t       option, optionlen)\n-\t\t    || push_options->items[options_seen - 1].string[optionlen])\n+\t\t    || xstrncmpz(push_options->items[options_seen - 1].string,\n+\t\t\t\t option, optionlen))\n \t\t\treturn 0;\n \t}\n\n--\n2.43.0\n"},{"id":"488450","messageId":"xmqq4jed8wlc.fsf@gitster.g","threadId":"60888","inReplyTo":"7b0a87f1-4693-480e-90d4-14675e9a5f01@web.de","subject":"Re: [PATCH 1/2] receive-pack: use find_commit_header() in check_cert_push_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-12T16:40:31Z","receivedAt":"2024-02-12T16:40:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> I just discovered 14570dc67d (wrapper: add function to compare strings\n> with different NUL termination, 2020-05-25).  Perhaps squash this in to\n> simplify?\n\nOooh, xstrncmpz() seems to target this exact use case.  Nice.\n\n\n>\n> ---\n>  builtin/receive-pack.c | 5 ++---\n>  1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index dbee508775..db65607485 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -717,9 +717,8 @@ static int check_cert_push_options(const struct string_list *push_options)\n>  \t\tbuf = option + optionlen + 1;\n>  \t\toptions_seen++;\n>  \t\tif (options_seen > push_options->nr\n> -\t\t    || strncmp(push_options->items[options_seen - 1].string,\n> -\t\t\t       option, optionlen)\n> -\t\t    || push_options->items[options_seen - 1].string[optionlen])\n> +\t\t    || xstrncmpz(push_options->items[options_seen - 1].string,\n> +\t\t\t\t option, optionlen))\n>  \t\t\treturn 0;\n>  \t}\n>\n> --\n> 2.43.0\n"},{"id":"497362","messageId":"0d85712c-5beb-4a64-a7f4-797782c26694@web.de","threadId":"60888","inReplyTo":"8b350cae-2180-4ac7-a911-d40043576445@web.de","subject":"[PATCH 3/2] commit: remove find_header_mem()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-19T17:13:19Z","receivedAt":"2024-06-19T17:13:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"cfc5cf428b (receive-pack.c: consolidate find header logic, 2022-01-06)\nintroduced find_header_mem() and turned find_commit_header() into a thin\nwrapper.  Since then, the latter has become the last remaining caller of\nthe former.  Remove it to restore find_commit_header() to the state\nbefore cfc5cf428b, get rid of a strlen(3) call and resolve a NEEDSWORK\nnote in the process.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n commit.c | 16 ++--------------\n commit.h |  5 -----\n 2 files changed, 2 insertions(+), 19 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 1d08951007..aacc401e72 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1870,20 +1870,12 @@ struct commit_list **commit_list_append(struct commit *commit,\n \treturn &new_commit->next;\n }\n\n-const char *find_header_mem(const char *msg, size_t len,\n-\t\t\tconst char *key, size_t *out_len)\n+const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n {\n \tint key_len = strlen(key);\n \tconst char *line = msg;\n\n-\t/*\n-\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n-\t * given by len. However, current callers are safe because they compute\n-\t * len by scanning a NUL-terminated block of memory starting at msg.\n-\t * Nonetheless, it would be better to ensure the function does not look\n-\t * at msg beyond the len provided by the caller.\n-\t */\n-\twhile (line && line < msg + len) {\n+\twhile (line) {\n \t\tconst char *eol = strchrnul(line, '\\n');\n\n \t\tif (line == eol)\n@@ -1900,10 +1892,6 @@ const char *find_header_mem(const char *msg, size_t len,\n \treturn NULL;\n }\n\n-const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n-{\n-\treturn find_header_mem(msg, strlen(msg), key, out_len);\n-}\n /*\n  * Inspect the given string and determine the true \"end\" of the log message, in\n  * order to find where to put a new Signed-off-by trailer.  Ignored are\ndiff --git a/commit.h b/commit.h\nindex 62fe0d77a7..3084f591fd 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -280,17 +280,12 @@ void free_commit_extra_headers(struct commit_extra_header *extra);\n\n /*\n  * Search the commit object contents given by \"msg\" for the header \"key\".\n- * Reads up to \"len\" bytes of \"msg\".\n  * Returns a pointer to the start of the header contents, or NULL. The length\n  * of the header, up to the first newline, is returned via out_len.\n  *\n  * Note that some headers (like mergetag) may be multi-line. It is the caller's\n  * responsibility to parse further in this case!\n  */\n-const char *find_header_mem(const char *msg, size_t len,\n-\t\t\tconst char *key,\n-\t\t\tsize_t *out_len);\n-\n const char *find_commit_header(const char *msg, const char *key,\n \t\t\t       size_t *out_len);\n\n--\n2.45.2\n"},{"id":"497365","messageId":"20240619173106.GA1519400@coredump.intra.peff.net","threadId":"60888","inReplyTo":"0d85712c-5beb-4a64-a7f4-797782c26694@web.de","subject":"Re: [PATCH 3/2] commit: remove find_header_mem()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-19T17:31:06Z","receivedAt":"2024-06-19T17:31:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 19, 2024 at 07:13:19PM +0200, René Scharfe wrote:\n\n> cfc5cf428b (receive-pack.c: consolidate find header logic, 2022-01-06)\n> introduced find_header_mem() and turned find_commit_header() into a thin\n> wrapper.  Since then, the latter has become the last remaining caller of\n> the former.  Remove it to restore find_commit_header() to the state\n> before cfc5cf428b, get rid of a strlen(3) call and resolve a NEEDSWORK\n> note in the process.\n\nThat of course made me wonder what happened to the other caller(s) of\nfind_header_mem(). The answer is that it went away in your 020456cb74\n(receive-pack: use find_commit_header() in check_nonce(), 2024-02-09)\n\n> -const char *find_header_mem(const char *msg, size_t len,\n> -\t\t\tconst char *key, size_t *out_len)\n> +const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n>  {\n>  \tint key_len = strlen(key);\n>  \tconst char *line = msg;\n\nNot new in your patch, but assigning strlen() to int tingled my\nspider-sense. It's OK, though, because \"key\" is always a small string\nliteral.\n\n> -\t/*\n> -\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n> -\t * given by len. However, current callers are safe because they compute\n> -\t * len by scanning a NUL-terminated block of memory starting at msg.\n> -\t * Nonetheless, it would be better to ensure the function does not look\n> -\t * at msg beyond the len provided by the caller.\n> -\t */\n> -\twhile (line && line < msg + len) {\n> +\twhile (line) {\n>  \t\tconst char *eol = strchrnul(line, '\\n');\n\nOK, we no longer know the length of the message, but we don't need to\nbecause it's NUL terminated, and strchrnul() will find the correct eol.\nThe length check might have saved us if we accidentally pushed \"line\"\npast the NUL terminator, but it looks like we take care not to do so in\nthe loop body:\n\n\tline = *eol ? eol + 1 : NULL;\n\nSo the conversion looks good to me.\n\n-Peff\n"},{"id":"497422","messageId":"xmqqr0crv5yi.fsf@gitster.g","threadId":"60888","inReplyTo":"20240619173106.GA1519400@coredump.intra.peff.net","subject":"Re: [PATCH 3/2] commit: remove find_header_mem()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-20T18:12:37Z","receivedAt":"2024-06-20T18:12:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> +const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n>>  {\n>>  \tint key_len = strlen(key);\n>>  \tconst char *line = msg;\n>\n> Not new in your patch, but assigning strlen() to int tingled my\n> spider-sense. It's OK, though, because \"key\" is always a small string\n> literal.\n\nYup.  All callers of find_commit_header() give in-program constants\nand never an externally sourced random string there.\n\n> So the conversion looks good to me.\n\nThanks.  Will queue.\n\n"}]}