git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] add_pending_object_with_path(): work around "gcc -O3" complaint
@ 2021-06-10 13:06 Jeff King
  2021-06-10 13:11 ` Jeff King
  0 siblings, 1 reply; 3+ messages in thread
From: Jeff King @ 2021-06-10 13:06 UTC (permalink / raw)
  To: git; +Cc: Ævar Arnfjörð Bjarmason

When compiling with -O3, some gcc versions (10.2.1 here) complain about
an out-of-bounds subscript:

  revision.c: In function ‘do_add_index_objects_to_pending’:
  revision.c:321:22: error: array subscript [1, 2147483647] is outside array bounds of ‘char[1]’ [-Werror=array-bounds]
    321 |   if (0 < len && name[len] && buf.len)
        |                  ~~~~^~~~~

The "len" parameter here comes from calling interpret_branch_name(),
which intends to return the number of characters of "name" it parsed.

But the compiler doesn't realize this. It knows the size of the empty
string "name" passed in from do_add_index_objects_to_pending(), but it
has no clue that the "len" we get back will be constrained to "0" in
that case.

And I don't think the warning is telling us about some subtle or clever
bug. The implementation of interpret_branch_name() is in another file
entirely, and the compiler can't see it (you can even verify there is no
clever LTO going on by replacing it with "return 0" and still getting
the warning).

We can work around this by replacing our "did we hit the trailing NUL"
subscript dereference with a length check. We do not even have to pay
the cost for an extra strlen(), as we can pass our new length into
interpret_branch_name(), which was converting our "0" into a call to
strlen() anyway.

Signed-off-by: Jeff King <peff@peff.net>
---
Together with ab/trace2-squelch-gcc-warning and the ll-merge fix I
posted in [0], this makes the whole project compile cleanly with "-O3"
for me.

I suspect after this patch we could drop the "if len is 0, call strlen()
as a courtesy" code from interpret_branch_name() entirely. But the call
stack gets pretty hairy (some of these len values come directly from
the get_oid functions directly!) so I wasn't brave enough to follow
through on that.

It's even possible the current code in interpret_branch_name() could be
buggy here, if some caller is using a non-NUL-terminated buffer or a
portion of a string, and we mistake a true "0" size for a request to
call strlen(). That might be worth poking at, but I didn't want to spend
time on it just now.

[0] https://lore.kernel.org/git/YMIKwsEFnkqz6PWa@coredump.intra.peff.net/

 revision.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index 8140561b6c..cddd0542a6 100644
--- a/revision.c
+++ b/revision.c
@@ -316,9 +316,10 @@ static void add_pending_object_with_path(struct rev_info *revs,
 		revs->no_walk = 0;
 	if (revs->reflog_info && obj->type == OBJ_COMMIT) {
 		struct strbuf buf = STRBUF_INIT;
-		int len = interpret_branch_name(name, 0, &buf, &options);
+		size_t namelen = strlen(name);
+		int len = interpret_branch_name(name, namelen, &buf, &options);
 
-		if (0 < len && name[len] && buf.len)
+		if (0 < len && len < namelen && buf.len)
 			strbuf_addstr(&buf, name + len);
 		add_reflog_for_walk(revs->reflog_info,
 				    (struct commit *)obj,
-- 
2.32.0.529.g079a794268

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] add_pending_object_with_path(): work around "gcc -O3" complaint
  2021-06-10 13:06 [PATCH] add_pending_object_with_path(): work around "gcc -O3" complaint Jeff King
@ 2021-06-10 13:11 ` Jeff King
  2021-06-11  3:52   ` Junio C Hamano
  0 siblings, 1 reply; 3+ messages in thread
From: Jeff King @ 2021-06-10 13:11 UTC (permalink / raw)
  To: git; +Cc: Ævar Arnfjörð Bjarmason

On Thu, Jun 10, 2021 at 09:06:44AM -0400, Jeff King wrote:

> We can work around this by replacing our "did we hit the trailing NUL"
> subscript dereference with a length check. We do not even have to pay
> the cost for an extra strlen(), as we can pass our new length into
> interpret_branch_name(), which was converting our "0" into a call to
> strlen() anyway.
> [...]
> -		if (0 < len && name[len] && buf.len)
> +		if (0 < len && len < namelen && buf.len)
>  			strbuf_addstr(&buf, name + len);

I guess another option would be to drop the check entirely. It is only
protecting us from calling strbuf_addstr() with an empty string, which
is a noop anyway (it would not even cause a useless allocation, since we
know that buf is non-empty, and that it won't need to grow).

I think I still prefer my original solution, though.

-Peff

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] add_pending_object_with_path(): work around "gcc -O3" complaint
  2021-06-10 13:11 ` Jeff King
@ 2021-06-11  3:52   ` Junio C Hamano
  0 siblings, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2021-06-11  3:52 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Ævar Arnfjörð Bjarmason

Jeff King <peff@peff.net> writes:

> On Thu, Jun 10, 2021 at 09:06:44AM -0400, Jeff King wrote:
>
>> We can work around this by replacing our "did we hit the trailing NUL"
>> subscript dereference with a length check. We do not even have to pay
>> the cost for an extra strlen(), as we can pass our new length into
>> interpret_branch_name(), which was converting our "0" into a call to
>> strlen() anyway.
>> [...]
>> -		if (0 < len && name[len] && buf.len)
>> +		if (0 < len && len < namelen && buf.len)
>>  			strbuf_addstr(&buf, name + len);
>
> I guess another option would be to drop the check entirely. It is only
> protecting us from calling strbuf_addstr() with an empty string, which
> is a noop anyway (it would not even cause a useless allocation, since we
> know that buf is non-empty, and that it won't need to grow).
>
> I think I still prefer my original solution, though.

It may still work without the guard but it is not apparent to the
readers if it just happens to work by accident or by design.  At
least the guard makes it clear what is going on, I would think.


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-06-11  3:52 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-10 13:06 [PATCH] add_pending_object_with_path(): work around "gcc -O3" complaint Jeff King
2021-06-10 13:11 ` Jeff King
2021-06-11  3:52   ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git