Re: [PATCH v2] commit.c: ensure find_header_mem() doesn't scan beyond given range
Am 07.02.24 um 14:57 schrieb Chandra Pratap via GitGitGadget:
Show 68 quoted lines
> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
> commit.c: ensure find_header_mem() doesn't scan beyond given range
>
> Thanks for the feedback, Kyle and René! I have update the patch to
> actually solve the problem at hand but I am not very sure about the
> resulting dropping of const-ness of 'eol' from this and how big of a
> problem it might create (if any). I wonder if a custom strchrnul() is
> the best solution to this after all.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1652%2FChand-ra%2Fstrchrnul-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1652/Chand-ra/strchrnul-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1652
>
> Range-diff vs v1:
>
> 1: 1c62f6ee353 ! 1: dcb2de3faea commit.c: ensure strchrnul() doesn't scan beyond range
> @@ Metadata
> Author: Chandra Pratap <chandrapratap3519@gmail.com>
>
> ## Commit message ##
> - commit.c: ensure strchrnul() doesn't scan beyond range
> + commit.c: ensure find_header_mem() doesn't scan beyond given range
>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
> @@ commit.c: const char *find_header_mem(const char *msg, size_t len,
> - * at msg beyond the len provided by the caller.
> - */
> while (line && line < msg + len) {
> - const char *eol = strchrnul(line, '\n');
> -+ assert(eol - line <= len);
> +- const char *eol = strchrnul(line, '\n');
> ++ char *eol = (char *) line;
> ++ for (size_t i = 0; i < len && *eol && *eol != '\n'; i++) {
> ++ eol++;
> ++ }
>
> if (line == eol)
> return NULL;
>
>
> commit.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index ef679a0b939..9a460b2fd6f 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1743,15 +1743,11 @@ const char *find_header_mem(const char *msg, size_t 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) {
> - const char *eol = strchrnul(line, '\n');
> + char *eol = (char *) line;
> + for (size_t i = 0; i < len && *eol && *eol != '\n'; i++) {
> + eol++;
> + }This uses the pointer eol only for reading, so you can keep it const.
The loop starts counting from 0 to len for each line, which cannot be right. find_header_mem("headers\nfoo bar", 9, "foo", &len) would still return "bar" instead of NULL.
You could initialize i to the offset of line within msg instead (i.e. i = line - msg). Or check eol < msg + len instead of i < len -- then you don't even need to introduce that separate counter.
Style nit: We tend to omit curly braces if they contain only a single statement.
> if (line == eol)
> return NULL;
>
> base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
