{"thread":{"id":"55897","subject":"[PATCH] add_pending_object_with_path(): work around \"gcc -O3\" complaint","startedAt":"2021-06-10T13:06:46Z","lastAt":"2021-06-11T03:52:14Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"426997","messageId":"YMIOY6Hp6//yGrFm@coredump.intra.peff.net","threadId":"55897","inReplyTo":null,"subject":"[PATCH] add_pending_object_with_path(): work around \"gcc -O3\" complaint","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-10T13:06:43Z","receivedAt":"2021-06-10T13:06:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When compiling with -O3, some gcc versions (10.2.1 here) complain about\nan out-of-bounds subscript:\n\n  revision.c: In function ‘do_add_index_objects_to_pending’:\n  revision.c:321:22: error: array subscript [1, 2147483647] is outside array bounds of ‘char[1]’ [-Werror=array-bounds]\n    321 |   if (0 < len && name[len] && buf.len)\n        |                  ~~~~^~~~~\n\nThe \"len\" parameter here comes from calling interpret_branch_name(),\nwhich intends to return the number of characters of \"name\" it parsed.\n\nBut the compiler doesn't realize this. It knows the size of the empty\nstring \"name\" passed in from do_add_index_objects_to_pending(), but it\nhas no clue that the \"len\" we get back will be constrained to \"0\" in\nthat case.\n\nAnd I don't think the warning is telling us about some subtle or clever\nbug. The implementation of interpret_branch_name() is in another file\nentirely, and the compiler can't see it (you can even verify there is no\nclever LTO going on by replacing it with \"return 0\" and still getting\nthe warning).\n\nWe can work around this by replacing our \"did we hit the trailing NUL\"\nsubscript dereference with a length check. We do not even have to pay\nthe cost for an extra strlen(), as we can pass our new length into\ninterpret_branch_name(), which was converting our \"0\" into a call to\nstrlen() anyway.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nTogether with ab/trace2-squelch-gcc-warning and the ll-merge fix I\nposted in [0], this makes the whole project compile cleanly with \"-O3\"\nfor me.\n\nI suspect after this patch we could drop the \"if len is 0, call strlen()\nas a courtesy\" code from interpret_branch_name() entirely. But the call\nstack gets pretty hairy (some of these len values come directly from\nthe get_oid functions directly!) so I wasn't brave enough to follow\nthrough on that.\n\nIt's even possible the current code in interpret_branch_name() could be\nbuggy here, if some caller is using a non-NUL-terminated buffer or a\nportion of a string, and we mistake a true \"0\" size for a request to\ncall strlen(). That might be worth poking at, but I didn't want to spend\ntime on it just now.\n\n[0] https://lore.kernel.org/git/YMIKwsEFnkqz6PWa@coredump.intra.peff.net/\n\n revision.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 8140561b6c..cddd0542a6 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -316,9 +316,10 @@ static void add_pending_object_with_path(struct rev_info *revs,\n \t\trevs->no_walk = 0;\n \tif (revs->reflog_info && obj->type == OBJ_COMMIT) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tint len = interpret_branch_name(name, 0, &buf, &options);\n+\t\tsize_t namelen = strlen(name);\n+\t\tint len = interpret_branch_name(name, namelen, &buf, &options);\n \n-\t\tif (0 < len && name[len] && buf.len)\n+\t\tif (0 < len && len < namelen && buf.len)\n \t\t\tstrbuf_addstr(&buf, name + len);\n \t\tadd_reflog_for_walk(revs->reflog_info,\n \t\t\t\t    (struct commit *)obj,\n-- \n2.32.0.529.g079a794268\n"},{"id":"426998","messageId":"YMIPb+9e2PtaLZhl@coredump.intra.peff.net","threadId":"55897","inReplyTo":"YMIOY6Hp6//yGrFm@coredump.intra.peff.net","subject":"Re: [PATCH] add_pending_object_with_path(): work around \"gcc -O3\" complaint","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-10T13:11:11Z","receivedAt":"2021-06-10T13:11:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 10, 2021 at 09:06:44AM -0400, Jeff King wrote:\n\n> We can work around this by replacing our \"did we hit the trailing NUL\"\n> subscript dereference with a length check. We do not even have to pay\n> the cost for an extra strlen(), as we can pass our new length into\n> interpret_branch_name(), which was converting our \"0\" into a call to\n> strlen() anyway.\n> [...]\n> -\t\tif (0 < len && name[len] && buf.len)\n> +\t\tif (0 < len && len < namelen && buf.len)\n>  \t\t\tstrbuf_addstr(&buf, name + len);\n\nI guess another option would be to drop the check entirely. It is only\nprotecting us from calling strbuf_addstr() with an empty string, which\nis a noop anyway (it would not even cause a useless allocation, since we\nknow that buf is non-empty, and that it won't need to grow).\n\nI think I still prefer my original solution, though.\n\n-Peff\n"},{"id":"427079","messageId":"xmqqzgvx3vo7.fsf@gitster.g","threadId":"55897","inReplyTo":"YMIPb+9e2PtaLZhl@coredump.intra.peff.net","subject":"Re: [PATCH] add_pending_object_with_path(): work around \"gcc -O3\" complaint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-11T03:52:08Z","receivedAt":"2021-06-11T03:52:14Z","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> On Thu, Jun 10, 2021 at 09:06:44AM -0400, Jeff King wrote:\n>\n>> We can work around this by replacing our \"did we hit the trailing NUL\"\n>> subscript dereference with a length check. We do not even have to pay\n>> the cost for an extra strlen(), as we can pass our new length into\n>> interpret_branch_name(), which was converting our \"0\" into a call to\n>> strlen() anyway.\n>> [...]\n>> -\t\tif (0 < len && name[len] && buf.len)\n>> +\t\tif (0 < len && len < namelen && buf.len)\n>>  \t\t\tstrbuf_addstr(&buf, name + len);\n>\n> I guess another option would be to drop the check entirely. It is only\n> protecting us from calling strbuf_addstr() with an empty string, which\n> is a noop anyway (it would not even cause a useless allocation, since we\n> know that buf is non-empty, and that it won't need to grow).\n>\n> I think I still prefer my original solution, though.\n\nIt may still work without the guard but it is not apparent to the\nreaders if it just happens to work by accident or by design.  At\nleast the guard makes it clear what is going on, I would think.\n\n"}]}