From: Junio C Hamano <gitster@pobox.com>
To: "René Scharfe" <l.s.r@web.de>
Cc: Chandra Pratap via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org, Kyle Lippincott <spectral@google.com>,
Chandra Pratap <chandrapratap376@gmail.com>,
Chandra Pratap <chandrapratap3519@gmail.com>
Subject: Re: [PATCH v2] commit.c: ensure find_header_mem() doesn't scan beyond given range
Date: Wed, 07 Feb 2024 09:23:58 -0800 [thread overview]
Message-ID: <xmqqr0ho9oi9.fsf@gitster.g> (raw)
In-Reply-To: <e7b269ea-6a10-4f3c-ae97-a58eb7ccc6ef@web.de> ("René Scharfe"'s message of "Wed, 7 Feb 2024 18:09:54 +0100")
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".
prev parent reply other threads:[~2024-02-07 17:24 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-05 17:21 [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range Chandra Pratap via GitGitGadget
2024-02-05 19:57 ` René Scharfe
2024-02-06 18:44 ` Junio C Hamano
2024-02-08 1:00 ` Jeff King
2024-02-08 18:31 ` René Scharfe
2024-02-08 19:48 ` Junio C Hamano
2024-02-08 19:52 ` Kyle Lippincott
2024-02-08 21:41 ` Jeff King
2024-02-08 21:44 ` Junio C Hamano
2024-02-06 1:41 ` Kyle Lippincott
2024-02-07 13:57 ` [PATCH v2] commit.c: ensure find_header_mem() doesn't scan beyond given range Chandra Pratap via GitGitGadget
2024-02-07 17:09 ` René Scharfe
2024-02-07 17:23 ` Junio C Hamano [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqr0ho9oi9.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=chandrapratap3519@gmail.com \
--cc=chandrapratap376@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=l.s.r@web.de \
--cc=spectral@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).