| Message ID | pull.1652.v2.git.1707314246530.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] commit.c: ensure find_header_mem() doesn't scan beyond given range | expand |
Am 07.02.24 um 14:57 schrieb Chandra Pratap via GitGitGadget: > 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
René Scharfe <l.s.r@web.de> writes: >> - /* >> - * 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. All true. As we already use an extra variable 'i' for counting, we can do without eol and reference line[i] instead, which would make the whole thing something like while (line && line < msg + len) { size_t i; for (i = 0; i < len && line[i] && line[i] != '\n'; i++) ; if (key_len < i && !strncmp(line, key, ken_len) && linhe[key_len] == ' ') { *out_len = i - key_len - 1; return line + key_len + 1; } line = line[i] ? line + i + 1 : NULL; } which is not too bad, simply because the original already needed to know the length of the current line and due to lack of this "i" you introduced, it used "eol-line" instead. Now you have "i", the code may get even simpler by getting rid of "eol".
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++; + } if (line == eol) return NULL;