{"thread":{"id":"57914","subject":"[PATCH 0/4] ci: fix windows-build with GCC v12.x","startedAt":"2022-05-24T00:23:25Z","lastAt":"2022-05-25T18:27:43Z","messageCount":23,"participants":["Johannes Schindelin via GitGitGadget","Ævar Arnfjörð Bjarmason","Johannes Schindelin","Jeff Hostetler","René Scharfe","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"455863","messageId":"pull.1238.git.1653351786.gitgitgadget@gmail.com","threadId":"57914","inReplyTo":null,"subject":"[PATCH 0/4] ci: fix windows-build with GCC v12.x","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-24T00:23:02Z","receivedAt":"2022-05-24T00:23:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"A recent update of GCC in Git for Windows' SDK (a subset of which is used in\nGit's CI/PR builds) broke the build.\n\nThese patches address that, and they are based on maint-2.34 (earlier\nmaintenance tracks would have a trivial merge conflict due to 013c7e2b070\n(http: drop support for curl < 7.16.0, 2021-07-30) removing support for\nUSE_CURL_MULTI).\n\nJohannes Schindelin (4):\n compat/win32/syslog: fix use-after-realloc\n nedmalloc: avoid new compile error\n http.c: avoid danging pointer to local variable `finished`\n dir.c: avoid \"exceeds maximum object size\" error with GCC v12.x\n\n compat/nedmalloc/nedmalloc.c | 1 -\n compat/win32/syslog.c | 2 ++\n dir.c | 9 +++++++++\n http-walker.c | 4 ----\n http.c | 15 +++++++--------\n http.h | 2 +-\n 6 files changed, 19 insertions(+), 14 deletions(-)\n\n\nbase-commit: 2f0dde7852b7866bb044926f73334ff3fc30654b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1238%2Fdscho%2Ffix-win-build-with-gcc-12-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1238/dscho/fix-win-build-with-gcc-12-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1238\n-- \ngitgitgadget\n"},{"id":"455864","messageId":"d0ade6531bc77b654c28cf7b1bfa42523150c015.1653351786.git.gitgitgadget@gmail.com","threadId":"57914","inReplyTo":"pull.1238.git.1653351786.gitgitgadget@gmail.com","subject":"[PATCH 1/4] compat/win32/syslog: fix use-after-realloc","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-24T00:23:03Z","receivedAt":"2022-05-24T00:23:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nGit for Windows' SDK recently upgraded to GCC v12.x which points out\nthat the `pos` variable might be used even after the corresponding\nmemory was `realloc()`ed and therefore potentially no longer valid.\n\nSince a subset of this SDK is used in Git's CI/PR builds, we need to fix\nthis to continue to be able to benefit from the CI/PR runs.\n\nNote: This bug has been with us since 2a6b149c64f6 (mingw: avoid using\nstrbuf in syslog, 2011-10-06), and while it looks tempting to replace\nthe hand-rolled string manipulation with a `strbuf`-based one, that\ncommit's message explains why we cannot do that: The `syslog()` function\nis called as part of the function in `daemon.c` which is set as the\n`die()` routine, and since `strbuf_grow()` can call that function if it\nruns out of memory, this would cause a nasty infinite loop that we do\nnot want to re-introduce.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/win32/syslog.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/compat/win32/syslog.c b/compat/win32/syslog.c\nindex 161978d720a..1f8d8934cc9 100644\n--- a/compat/win32/syslog.c\n+++ b/compat/win32/syslog.c\n@@ -43,6 +43,7 @@ void syslog(int priority, const char *fmt, ...)\n \tva_end(ap);\n \n \twhile ((pos = strstr(str, \"%1\")) != NULL) {\n+\t\tsize_t offset = pos - str;\n \t\tchar *oldstr = str;\n \t\tstr = realloc(str, st_add(++str_len, 1));\n \t\tif (!str) {\n@@ -50,6 +51,7 @@ void syslog(int priority, const char *fmt, ...)\n \t\t\twarning_errno(\"realloc failed\");\n \t\t\treturn;\n \t\t}\n+\t\tpos = str + offset;\n \t\tmemmove(pos + 2, pos + 1, strlen(pos));\n \t\tpos[1] = ' ';\n \t}\n-- \ngitgitgadget\n\n"},{"id":"455865","messageId":"8963c6fa625bbaf5153990939ea06742304ddcd2.1653351786.git.gitgitgadget@gmail.com","threadId":"57914","inReplyTo":"pull.1238.git.1653351786.gitgitgadget@gmail.com","subject":"[PATCH 2/4] nedmalloc: avoid new compile error","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-24T00:23:04Z","receivedAt":"2022-05-24T00:23:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nGCC v12.x complains thusly:\n\ncompat/nedmalloc/nedmalloc.c: In function 'DestroyCaches':\ncompat/nedmalloc/nedmalloc.c:326:12: error: the comparison will always\n evaluate as 'true' for the address of 'caches'\n will never be NULL [-Werror=address]\n 326 | if(p->caches)\n | ^\ncompat/nedmalloc/nedmalloc.c:196:22: note: 'caches' declared here\n 196 | threadcache *caches[THREADCACHEMAXCACHES];\n | ^~~~~~\n\n... and it is correct, of course.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/nedmalloc/nedmalloc.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c\nindex edb438a7776..2c0ace7075a 100644\n--- a/compat/nedmalloc/nedmalloc.c\n+++ b/compat/nedmalloc/nedmalloc.c\n@@ -323,7 +323,6 @@ static NOINLINE void RemoveCacheEntries(nedpool *p, threadcache *tc, unsigned in\n }\n static void DestroyCaches(nedpool *p) THROWSPEC\n {\n-\tif(p->caches)\n \t{\n \t\tthreadcache *tc;\n \t\tint n;\n-- \ngitgitgadget\n\n"},{"id":"455866","messageId":"4a4e0aa0a49a54eea88f9c2d8e1db6a697012718.1653351786.git.gitgitgadget@gmail.com","threadId":"57914","inReplyTo":"pull.1238.git.1653351786.gitgitgadget@gmail.com","subject":"[PATCH 3/4] http.c: avoid danging pointer to local variable `finished`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-24T00:23:05Z","receivedAt":"2022-05-24T00:23:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn http.c, the run_active_slot() function allows the given \"slot\" to\nmake progress by calling step_active_slots() in a loop repeatedly, and\nthe loop is not left until the request held in the slot completes.\n\nAges ago, we used to use the slot->in_use member to get out of the loop,\nwhich misbehaved when the request in \"slot\" completes (at which time,\nthe result of the request is copied away from the slot, and the in_use\nmember is cleared, making the slot ready to be reused), and the \"slot\"\ngets reused to service a different request (at which time, the \"slot\"\nbecomes in_use again, even though it is for a different request). The\nloop terminating condition mistakenly thought that the original request\nhas yet to be completed.\n\nToday's code, after baa7b67d (HTTP slot reuse fixes, 2006-03-10) fixed\nthis issue, uses a separate \"slot->finished\" member that is set in\nrun_active_slot() to point to an on-stack variable, and the code that\ncompletes the request in finish_active_slot() clears the on-stack\nvariable via the pointer to signal that the particular request held by\nthe slot has completed. It also clears the in_use member (as before\nthat fix), so that the slot itself can safely be reused for an unrelated\nrequest.\n\nOne thing that is not quite clean in this arrangement is that, unless\nthe slot gets reused, at which point the finished member is reset to\nNULL, the member keeps the value of &finished, which becomes a dangling\npointer into the stack when run_active_slot() returns.\n\nLet's drop that local variable and introduce a new flag in the slot that\nis used to indicate that even while the slot is no longer in use, it is\nstill reserved until further notice. It is the responsibility of\n`run_active_slot()` to clear that flag once it is done with that slot.\n\nInitial-patch-by: Junio C Hamano \nSigned-off-by: Johannes Schindelin \n---\n http-walker.c | 4 ----\n http.c | 15 +++++++--------\n http.h | 2 +-\n 3 files changed, 8 insertions(+), 13 deletions(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex 910fae539b8..5cc369dea85 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -225,13 +225,9 @@ static void process_alternates_response(void *callback_data)\n \t\t\t\t\t alt_req->url->buf);\n \t\t\tactive_requests++;\n \t\t\tslot->in_use = 1;\n-\t\t\tif (slot->finished != NULL)\n-\t\t\t\t(*slot->finished) = 0;\n \t\t\tif (!start_active_slot(slot)) {\n \t\t\t\tcdata->got_alternates = -1;\n \t\t\t\tslot->in_use = 0;\n-\t\t\t\tif (slot->finished != NULL)\n-\t\t\t\t\t(*slot->finished) = 1;\n \t\t\t}\n \t\t\treturn;\n \t\t}\ndiff --git a/http.c b/http.c\nindex f92859f43fa..00206676597 100644\n--- a/http.c\n+++ b/http.c\n@@ -197,8 +197,7 @@ static void finish_active_slot(struct active_request_slot *slot)\n \tclosedown_active_slot(slot);\n \tcurl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);\n \n-\tif (slot->finished != NULL)\n-\t\t(*slot->finished) = 1;\n+\tslot->in_use = 0;\n \n \t/* Store slot results so they can be read after the slot is reused */\n \tif (slot->results != NULL) {\n@@ -1176,13 +1175,14 @@ struct active_request_slot *get_active_slot(void)\n \t\t\tprocess_curl_messages();\n \t}\n \n-\twhile (slot != NULL && slot->in_use)\n+\twhile (slot != NULL && (slot->in_use || slot->reserved_for_use))\n \t\tslot = slot->next;\n \n \tif (slot == NULL) {\n \t\tnewslot = xmalloc(sizeof(*newslot));\n \t\tnewslot->curl = NULL;\n \t\tnewslot->in_use = 0;\n+\t\tnewslot->reserved_for_use = 0;\n \t\tnewslot->next = NULL;\n \n \t\tslot = active_queue_head;\n@@ -1204,7 +1204,6 @@ struct active_request_slot *get_active_slot(void)\n \tactive_requests++;\n \tslot->in_use = 1;\n \tslot->results = NULL;\n-\tslot->finished = NULL;\n \tslot->callback_data = NULL;\n \tslot->callback_func = NULL;\n \tcurl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file);\n@@ -1296,7 +1295,7 @@ void fill_active_slots(void)\n \t}\n \n \twhile (slot != NULL) {\n-\t\tif (!slot->in_use && slot->curl != NULL\n+\t\tif (!slot->in_use && !slot->reserved_for_use && slot->curl\n \t\t\t&& curl_session_count > min_curl_sessions) {\n \t\t\tcurl_easy_cleanup(slot->curl);\n \t\t\tslot->curl = NULL;\n@@ -1327,10 +1326,9 @@ void run_active_slot(struct active_request_slot *slot)\n \tfd_set excfds;\n \tint max_fd;\n \tstruct timeval select_timeout;\n-\tint finished = 0;\n \n-\tslot->finished = &finished;\n-\twhile (!finished) {\n+\tslot->reserved_for_use = 1;\n+\twhile (slot->in_use) {\n \t\tstep_active_slots();\n \n \t\tif (slot->in_use) {\n@@ -1367,6 +1365,7 @@ void run_active_slot(struct active_request_slot *slot)\n \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n \t\t}\n \t}\n+\tslot->reserved_for_use = 0;\n }\n \n static void release_active_slot(struct active_request_slot *slot)\ndiff --git a/http.h b/http.h\nindex df1590e53a4..3b2f6da570c 100644\n--- a/http.h\n+++ b/http.h\n@@ -22,9 +22,9 @@ struct slot_results {\n struct active_request_slot {\n \tCURL *curl;\n \tint in_use;\n+\tint reserved_for_use;\n \tCURLcode curl_result;\n \tlong http_code;\n-\tint *finished;\n \tstruct slot_results *results;\n \tvoid *callback_data;\n \tvoid (*callback_func)(void *data);\n-- \ngitgitgadget\n\n"},{"id":"455867","messageId":"365889ee96e37dc9dcbe60d98880eb256dae90ee.1653351786.git.gitgitgadget@gmail.com","threadId":"57914","inReplyTo":"pull.1238.git.1653351786.gitgitgadget@gmail.com","subject":"[PATCH 4/4] dir.c: avoid \"exceeds maximum object size\" error with GCC v12.x","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-24T00:23:06Z","receivedAt":"2022-05-24T00:24:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nTechnically, the pointer difference `end - start` _could_ be negative,\nand when cast to an (unsigned) `size_t` that would cause problems. In\nthis instance, the symptom is:\n\ndir.c: In function 'git_url_basename':\ndir.c:3087:13: error: 'memchr' specified bound [9223372036854775808, 0]\n exceeds maximum object size 9223372036854775807\n [-Werror=stringop-overread]\n CC ewah/bitmap.o\n 3087 | if (memchr(start, '/', end - start) == NULL\n | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\nWhile it is a bit far-fetched to think that `end` (which is defined as\n`repo + strlen(repo)`) and `start` (which starts at `repo` and never\nsteps beyond the NUL terminator) could result in such a negative\ndifference, GCC has no way of knowing that.\n\nSee also https://gcc.gnu.org/bugzilla//show_bug.cgi?id=85783.\n\nLet's just add a safety check, primarily for GCC's benefit.\n\nSigned-off-by: Johannes Schindelin \n---\n dir.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/dir.c b/dir.c\nindex 5aa6fbad0b7..ea78f606230 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -3076,6 +3076,15 @@ char *git_url_basename(const char *repo, int is_bundle, int is_bare)\n \t\t\tend--;\n \t}\n \n+\t/*\n+\t * It should not be possible to overflow `ptrdiff_t` by passing in an\n+\t * insanely long URL, but GCC does not know that and will complain\n+\t * without this check.\n+\t */\n+\tif (end - start < 0)\n+\t\tdie(_(\"No directory name could be guessed.\\n\"\n+\t\t \"Please specify a directory on the command line\"));\n+\n \t/*\n \t * Strip trailing port number if we've got only a\n \t * hostname (that is, there is no dir separator but a\n-- \ngitgitgadget\n"},{"id":"455870","messageId":"220524.86k0abxyqa.gmgdl@evledraar.gmail.com","threadId":"57914","inReplyTo":"365889ee96e37dc9dcbe60d98880eb256dae90ee.1653351786.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] dir.c: avoid \"exceeds maximum object size\" error with GCC v12.x","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-24T05:53:20Z","receivedAt":"2022-05-24T05:53:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 24 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> Technically, the pointer difference `end - start` _could_ be negative,\n> and when cast to an (unsigned) `size_t` that would cause problems. In\n> this instance, the symptom is:\n>\n> dir.c: In function 'git_url_basename':\n> dir.c:3087:13: error: 'memchr' specified bound [9223372036854775808, 0]\n> exceeds maximum object size 9223372036854775807\n> [-Werror=stringop-overread]\n> CC ewah/bitmap.o\n> 3087 | if (memchr(start, '/', end - start) == NULL\n> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n>\n> While it is a bit far-fetched to think that `end` (which is defined as\n> `repo + strlen(repo)`) and `start` (which starts at `repo` and never\n> steps beyond the NUL terminator) could result in such a negative\n> difference, GCC has no way of knowing that.\n>\n> See also https://gcc.gnu.org/bugzilla//show_bug.cgi?id=85783.\n>\n> Let's just add a safety check, primarily for GCC's benefit.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> dir.c | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n>\n> diff --git a/dir.c b/dir.c\n> index 5aa6fbad0b7..ea78f606230 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -3076,6 +3076,15 @@ char *git_url_basename(const char *repo, int is_bundle, int is_bare)\n> \t\t\tend--;\n> \t}\n> \n> +\t/*\n> +\t * It should not be possible to overflow `ptrdiff_t` by passing in an\n> +\t * insanely long URL, but GCC does not know that and will complain\n> +\t * without this check.\n> +\t */\n> +\tif (end - start < 0)\n> +\t\tdie(_(\"No directory name could be guessed.\\n\"\n\nThis should start with a lower-case letter, see CodingGuidelines.\n\n> +\t\t \"Please specify a directory on the command line\"));\n> +\n> \t/*\n> \t * Strip trailing port number if we've got only a\n> \t * hostname (that is, there is no dir separator but a\n\n"},{"id":"455875","messageId":"220524.86fskzxsvq.gmgdl@evledraar.gmail.com","threadId":"57914","inReplyTo":"4a4e0aa0a49a54eea88f9c2d8e1db6a697012718.1653351786.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] http.c: avoid danging pointer to local variable `finished`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-24T07:58:51Z","receivedAt":"2022-05-24T08:00:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 24 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> [...]\n> Let's drop that local variable and introduce a new flag in the slot that\n> is used to indicate that even while the slot is no longer in use, it is\n> still reserved until further notice. It is the responsibility of\n> `run_active_slot()` to clear that flag once it is done with that slot.\n>\n> Initial-patch-by: Junio C Hamano \n\nDon't you mean by me?\nI.e. https://lore.kernel.org/git/patch-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com/\n\nThis seems to be derived from that, or perhaps you just came up with\nsomething similar independently. Junio then came up with the smaller\nhttps://lore.kernel.org/git/xmqq8rv2nggn.fsf@gitster.g/\n"},{"id":"455876","messageId":"220524.86bkvnxsm5.gmgdl@evledraar.gmail.com","threadId":"57914","inReplyTo":"8963c6fa625bbaf5153990939ea06742304ddcd2.1653351786.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] nedmalloc: avoid new compile error","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-24T08:00:45Z","receivedAt":"2022-05-24T08:06:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 24 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> GCC v12.x complains thusly:\n>\n> compat/nedmalloc/nedmalloc.c: In function 'DestroyCaches':\n> compat/nedmalloc/nedmalloc.c:326:12: error: the comparison will always\n> evaluate as 'true' for the address of 'caches'\n> will never be NULL [-Werror=address]\n> 326 | if(p->caches)\n> | ^\n> compat/nedmalloc/nedmalloc.c:196:22: note: 'caches' declared here\n> 196 | threadcache *caches[THREADCACHEMAXCACHES];\n> | ^~~~~~\n>\n> ... and it is correct, of course.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> compat/nedmalloc/nedmalloc.c | 1 -\n> 1 file changed, 1 deletion(-)\n>\n> diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c\n> index edb438a7776..2c0ace7075a 100644\n> --- a/compat/nedmalloc/nedmalloc.c\n> +++ b/compat/nedmalloc/nedmalloc.c\n> @@ -323,7 +323,6 @@ static NOINLINE void RemoveCacheEntries(nedpool *p, threadcache *tc, unsigned in\n> }\n> static void DestroyCaches(nedpool *p) THROWSPEC\n> {\n> -\tif(p->caches)\n> \t{\n> \t\tthreadcache *tc;\n> \t\tint n;\n\nThis seems sensible, I thought \"why not submit it to upstream\",\ni.e. see:\nhttps://github.com/ned14/nedmalloc/blob/master/nedmalloc.c#L1298\n\nBut that repository was last updated in 2014, I wonder if it's just\nbecause nobody's submitted a patch since then, or if it's inactive. Have\nyou tried making Njall Douglas (the nedmalloc author) aware of this\nissue?\n\n"},{"id":"455896","messageId":"nycvar.QRO.7.76.6.2205241431400.352@tvgsbejvaqbjf.bet","threadId":"57914","inReplyTo":"d0ade6531bc77b654c28cf7b1bfa42523150c015.1653351786.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] compat/win32/syslog: fix use-after-realloc","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-24T12:39:41Z","receivedAt":"2022-05-24T12:39:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 24 May 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c\n> index 161978d720a..1f8d8934cc9 100644\n> --- a/compat/win32/syslog.c\n> +++ b/compat/win32/syslog.c\n> @@ -43,6 +43,7 @@ void syslog(int priority, const char *fmt, ...)\n> \tva_end(ap);\n>\n> \twhile ((pos = strstr(str, \"%1\")) != NULL) {\n> +\t\tsize_t offset = pos - str;\n> \t\tchar *oldstr = str;\n> \t\tstr = realloc(str, st_add(++str_len, 1));\n\nSince it has been raised elsewhere: Why is that `++str_len` not turned\ninto an `st_add()`?\n\nThe commit adding that `st_add()` call (50a6c8efa2b (use st_add and\nst_mult for allocation size computation, 2016-02-22)) does not really talk\nabout it, but the explanation is simple:\n\nBefore this `while()` loop, we allocate one more than `str_len` (see\ncompat/win32/syslog.c#L35), and we do that already using `st_add()`, so\nthat the string and the terminating NUL fit into the allocated memory.\n\nTherefore, the first time we enter the loop, we know that `++str_len` is\nsafe.\n\nNow, in this very line, we then increment `str_len` and then `reallocate`\none more than that, again guarding it via `st_add()`. So every subsequent\niteration will already have checked that `++str_len` is safe, too.\n\nBy induction (https://en.wikipedia.org/wiki/Mathematical_induction), it\nfollows that this line is safe, and we do not have to change it to a\nclunkier two-step assignment where we first use `st_add()` to increment\n`str_len` and then use `st_add()` to allocate enough memory to also fit\nthe trailing NUL.\n\nNow you know,\nDscho\n\n> \t\tif (!str) {\n> @@ -50,6 +51,7 @@ void syslog(int priority, const char *fmt, ...)\n> \t\t\twarning_errno(\"realloc failed\");\n> \t\t\treturn;\n> \t\t}\n> +\t\tpos = str + offset;\n> \t\tmemmove(pos + 2, pos + 1, strlen(pos));\n> \t\tpos[1] = ' ';\n> \t}\n> --\n> gitgitgadget\n>\n>\n"},{"id":"455912","messageId":"a9062373-5347-4bfc-226a-fc9e892384f2@jeffhostetler.com","threadId":"57914","inReplyTo":"pull.1238.git.1653351786.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] ci: fix windows-build with GCC v12.x","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2022-05-24T15:54:57Z","receivedAt":"2022-05-24T15:55:02Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 5/23/22 8:23 PM, Johannes Schindelin via GitGitGadget wrote:\n> A recent update of GCC in Git for Windows' SDK (a subset of which is used in\n> Git's CI/PR builds) broke the build.\n> \n> These patches address that, and they are based on maint-2.34 (earlier\n> maintenance tracks would have a trivial merge conflict due to 013c7e2b070\n> (http: drop support for curl < 7.16.0, 2021-07-30) removing support for\n> USE_CURL_MULTI).\n> \n> Johannes Schindelin (4):\n> compat/win32/syslog: fix use-after-realloc\n> nedmalloc: avoid new compile error\n> http.c: avoid danging pointer to local variable `finished`\n> dir.c: avoid \"exceeds maximum object size\" error with GCC v12.x\n> \n> compat/nedmalloc/nedmalloc.c | 1 -\n> compat/win32/syslog.c | 2 ++\n> dir.c | 9 +++++++++\n> http-walker.c | 4 ----\n> http.c | 15 +++++++--------\n> http.h | 2 +-\n> 6 files changed, 19 insertions(+), 14 deletions(-)\n> \n> \n> base-commit: 2f0dde7852b7866bb044926f73334ff3fc30654b\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1238%2Fdscho%2Ffix-win-build-with-gcc-12-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1238/dscho/fix-win-build-with-gcc-12-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1238\n> \n\nThis looks good to me. I reviewed it earlier in one of our\ndownstream forks.\n\nJeff\n"},{"id":"455913","messageId":"972c3c82-0c10-ee95-9a7f-082de02a49c0@web.de","threadId":"57914","inReplyTo":"220524.86bkvnxsm5.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/4] nedmalloc: avoid new compile error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-05-24T15:59:56Z","receivedAt":"2022-05-24T16:00:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.05.22 um 10:00 schrieb Ævar Arnfjörð Bjarmason:\n>\n> This seems sensible, I thought \"why not submit it to upstream\",\n> i.e. see:\n> https://github.com/ned14/nedmalloc/blob/master/nedmalloc.c#L1298\n>\n> But that repository was last updated in 2014, I wonder if it's just\n> because nobody's submitted a patch since then, or if it's inactive. Have\n> you tried making Njall Douglas (the nedmalloc author) aware of this\n> issue?\n>\n\nhttps://github.com/ned14/nedmalloc says at the top: \"This repository has\nbeen archived by the owner. It is now read-only.\".\n\nRené\n"},{"id":"455946","messageId":"xmqq35gyhf11.fsf@gitster.g","threadId":"57914","inReplyTo":"220524.86fskzxsvq.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 3/4] http.c: avoid danging pointer to local variable `finished`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-24T20:06:02Z","receivedAt":"2022-05-24T20:06:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Tue, May 24 2022, Johannes Schindelin via GitGitGadget wrote:\n>\n>> From: Johannes Schindelin \n>> [...]\n>> Let's drop that local variable and introduce a new flag in the slot that\n>> is used to indicate that even while the slot is no longer in use, it is\n>> still reserved until further notice. It is the responsibility of\n>> `run_active_slot()` to clear that flag once it is done with that slot.\n>>\n>> Initial-patch-by: Junio C Hamano \n>\n> Don't you mean by me?\n> I.e. https://lore.kernel.org/git/patch-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com/\n\nMost likely, but this version is so distant from the \"clear\nslot->finished before leaving run_active_slot()\" Dscho and I were\nrecently discussing, that I do not think it can be said to have been\nderived from that one. This is completely a different patch that\nmakes different changes.\n\nThe \"clear slot->finished\", by the way, is what I think is the right\nthing to do, especially that the objective is to squelch the false\npositive warning from a new compiler. If there is a way to annotate\nthe line for the compiler to tell it not to warn about it, that would\nhave been even better.\n\n> This seems to be derived from that, or perhaps you just came up with\n> something similar independently. Junio then came up with the smaller\n> https://lore.kernel.org/git/xmqq8rv2nggn.fsf@gitster.g/\n\nI actually do not think so. Yours is revert of the existing fix the\ncompiler is confused about, and I have a feeling that if the original\nfix is still relevant, the problem the original fix wanted to address\nwill resurface as a regression.\n\nIf I am reading the patch correctly, Dscho's is to avoid [*] reusing\na slot while any run_active_slot() is still waiting for its\ncompletion. The approach would solve the problem the original fix\nwanted to solve in a different way. Personally I do not think such\na surgery is necessary only to squelch false positives from a new\nwarning compiler, though.\n\n\n[Footnote] \n\n * I said \"is to avoid\", not \"avoids\", because I haven't studied the\n patch with sufficient degree of carefulness to say for sure, even\n though I can see that is the intent.\n\n"},{"id":"455954","messageId":"nycvar.QRO.7.76.6.2205242239150.352@tvgsbejvaqbjf.bet","threadId":"57914","inReplyTo":"972c3c82-0c10-ee95-9a7f-082de02a49c0@web.de","subject":"Re: [PATCH 2/4] nedmalloc: avoid new compile error","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-24T20:46:58Z","receivedAt":"2022-05-24T20:47:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Tue, 24 May 2022, René Scharfe wrote:\n\n> Am 24.05.22 um 10:00 schrieb Ævar Arnfjörð Bjarmason:\n> >\n> > This seems sensible, I thought \"why not submit it to upstream\",\n> > i.e. see:\n> > https://github.com/ned14/nedmalloc/blob/master/nedmalloc.c#L1298\n> >\n> > But that repository was last updated in 2014, I wonder if it's just\n> > because nobody's submitted a patch since then, or if it's inactive. Have\n> > you tried making Njall Douglas (the nedmalloc author) aware of this\n> > issue?\n> >\n>\n> https://github.com/ned14/nedmalloc says at the top: \"This repository has\n> been archived by the owner. It is now read-only.\".\n\nIndeed, maintenance of nedmalloc has stopped a few years ago (see e.g.\nhttps://github.com/ned14/nedmalloc/issues/20#issuecomment-343432314).\n\nAbout five years ago I tried to upgrade us to the latest nedmalloc version\nbut ran into a performance regression that I was unable to justify the\ntime to investigate further.\n\nCiao,\nDscho\n"},{"id":"455957","messageId":"xmqqy1yqfy0x.fsf@gitster.g","threadId":"57914","inReplyTo":"nycvar.QRO.7.76.6.2205241431400.352@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/4] compat/win32/syslog: fix use-after-realloc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-24T20:58:38Z","receivedAt":"2022-05-24T20:58:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> \t\tstr = realloc(str, st_add(++str_len, 1));\n>\n> Since it has been raised elsewhere: Why is that `++str_len` not turned\n> into an `st_add()`?\n> ...\n> Now you know,\n\nI'd be more worried about a macro looking thing evalutating its\nparameters more than once, though. But unlike st_addN(), st_add()\nis an inline function so we do not have to worry about that ;-)\n\n>> @@ -50,6 +51,7 @@ void syslog(int priority, const char *fmt, ...)\n>> \t\t\twarning_errno(\"realloc failed\");\n>> \t\t\treturn;\n>> \t\t}\n>> +\t\tpos = str + offset;\n\nThe adjustment using ofs is very much straight-forward. Nicely\nspotted and nicely corrected.\n\nThanks.\n"},{"id":"455959","messageId":"nycvar.QRO.7.76.6.2205242302020.352@tvgsbejvaqbjf.bet","threadId":"57914","inReplyTo":"220524.86k0abxyqa.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/4] dir.c: avoid \"exceeds maximum object size\" error with GCC v12.x","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-24T21:05:35Z","receivedAt":"2022-05-24T21:05:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Tue, 24 May 2022, Ævar Arnfjörð Bjarmason wrote:\n\n>\n> On Tue, May 24 2022, Johannes Schindelin via GitGitGadget wrote:\n>\n> > From: Johannes Schindelin \n> >\n> > Technically, the pointer difference `end - start` _could_ be negative,\n> > and when cast to an (unsigned) `size_t` that would cause problems. In\n> > this instance, the symptom is:\n> >\n> > dir.c: In function 'git_url_basename':\n> > dir.c:3087:13: error: 'memchr' specified bound [9223372036854775808, 0]\n> > exceeds maximum object size 9223372036854775807\n> > [-Werror=stringop-overread]\n> > CC ewah/bitmap.o\n> > 3087 | if (memchr(start, '/', end - start) == NULL\n> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> >\n> > While it is a bit far-fetched to think that `end` (which is defined as\n> > `repo + strlen(repo)`) and `start` (which starts at `repo` and never\n> > steps beyond the NUL terminator) could result in such a negative\n> > difference, GCC has no way of knowing that.\n> >\n> > See also https://gcc.gnu.org/bugzilla//show_bug.cgi?id=85783.\n> >\n> > Let's just add a safety check, primarily for GCC's benefit.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > dir.c | 9 +++++++++\n> > 1 file changed, 9 insertions(+)\n> >\n> > diff --git a/dir.c b/dir.c\n> > index 5aa6fbad0b7..ea78f606230 100644\n> > --- a/dir.c\n> > +++ b/dir.c\n> > @@ -3076,6 +3076,15 @@ char *git_url_basename(const char *repo, int is_bundle, int is_bare)\n> > \t\t\tend--;\n> > \t}\n> >\n> > +\t/*\n> > +\t * It should not be possible to overflow `ptrdiff_t` by passing in an\n> > +\t * insanely long URL, but GCC does not know that and will complain\n> > +\t * without this check.\n> > +\t */\n> > +\tif (end - start < 0)\n> > +\t\tdie(_(\"No directory name could be guessed.\\n\"\n>\n> This should start with a lower-case letter, see CodingGuidelines.\n\nThis message is copied from existing code later in the same function.\nSince it is a translateable message, I do not want to edit it because that\nwould cause unnecessary work of the translators. Especially given that we\ndo not even expect this message to be shown, ever, but we only add this\nhunk for GCC's benefit.\n\nThank you,\nJohannes\n\n>\n> > +\t\t \"Please specify a directory on the command line\"));\n> > +\n> > \t/*\n> > \t * Strip trailing port number if we've got only a\n> > \t * hostname (that is, there is no dir separator but a\n>\n>\n"},{"id":"455960","messageId":"nycvar.QRO.7.76.6.2205242309160.352@tvgsbejvaqbjf.bet","threadId":"57914","inReplyTo":"xmqq35gyhf11.fsf@gitster.g","subject":"Re: [PATCH 3/4] http.c: avoid danging pointer to local variable `finished`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-24T21:15:58Z","receivedAt":"2022-05-24T21:16:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 24 May 2022, Junio C Hamano wrote:\n\n> The \"clear slot->finished\", by the way, is what I think is the right\n> thing to do, especially that the objective is to squelch the false\n> positive warning from a new compiler. If there is a way to annotate\n> the line for the compiler to tell it not to warn about it, that would\n> have been even better.\n\nWe could do something like this:\n\n-- snip --\ndiff --git a/http.c b/http.c\nindex b08795715f8a..2ac8d51d3668 100644\n--- a/http.c\n+++ b/http.c\n@@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot)\n \tstruct timeval select_timeout;\n \tint finished = 0;\n\n+#if __GNUC__ >= 12\n+#pragma GCC diagnostic push\n+#pragma GCC diagnostic ignored \"-Wdangling-pointer\"\n+#endif\n \tslot->finished = &finished;\n+#if __GNUC__ >= 12\n+#pragma GCC diagnostic pop\n+#endif\n \twhile (!finished) {\n \t\tstep_active_slots();\n-- snap --\n\nThat's quite ugly, though. And what's worse, it is pretty unreadable, too.\n\nCiao,\nDscho\n"},{"id":"455964","messageId":"220524.86ee0ivch2.gmgdl@evledraar.gmail.com","threadId":"57914","inReplyTo":"nycvar.QRO.7.76.6.2205242239150.352@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/4] nedmalloc: avoid new compile error","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-24T21:33:33Z","receivedAt":"2022-05-24T21:37:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 24 2022, Johannes Schindelin wrote:\n\n> Hi René,\n>\n> On Tue, 24 May 2022, René Scharfe wrote:\n>\n>> Am 24.05.22 um 10:00 schrieb Ævar Arnfjörð Bjarmason:\n>> >\n>> > This seems sensible, I thought \"why not submit it to upstream\",\n>> > i.e. see:\n>> > https://github.com/ned14/nedmalloc/blob/master/nedmalloc.c#L1298\n>> >\n>> > But that repository was last updated in 2014, I wonder if it's just\n>> > because nobody's submitted a patch since then, or if it's inactive. Have\n>> > you tried making Njall Douglas (the nedmalloc author) aware of this\n>> > issue?\n>> >\n>>\n>> https://github.com/ned14/nedmalloc says at the top: \"This repository has\n>> been archived by the owner. It is now read-only.\".\n>\n> Indeed, maintenance of nedmalloc has stopped a few years ago (see e.g.\n> https://github.com/ned14/nedmalloc/issues/20#issuecomment-343432314).\n\nThe author says:\n\n nedmalloc is pretty much EOL. Happy to accept patches, but unwilling to fix.\n\nThe \"Happy to accept patches\" seems to suggest that they're willing to\ntake a PR, just not willing to do spend time on it themselves.\n\nAnyway, I see that we've accumulated quite a few patches on top, and\ngiven...\n\n> About five years ago I tried to upgrade us to the latest nedmalloc version\n> but ran into a performance regression that I was unable to justify the\n> time to investigate further.\n\n...perhaps it's not worth it.\n\nMaybe someone should get to updating the readme we carry from it to\nchange the first line from:\n\n nedalloc v1.05 15th June 2008:\n\nTo:\n\n Git's (perma-)fork & local hacks on top of nedalloc v1.05 15th June 2008:\n\nOr something :)\n"},{"id":"455969","messageId":"220524.865yluvby1.gmgdl@evledraar.gmail.com","threadId":"57914","inReplyTo":"nycvar.QRO.7.76.6.2205242309160.352@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 3/4] http.c: avoid danging pointer to local variable `finished`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-24T21:45:47Z","receivedAt":"2022-05-24T21:49:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 24 2022, Johannes Schindelin wrote:\n\n> Hi Junio,\n>\n> On Tue, 24 May 2022, Junio C Hamano wrote:\n>\n>> The \"clear slot->finished\", by the way, is what I think is the right\n>> thing to do, especially that the objective is to squelch the false\n>> positive warning from a new compiler. If there is a way to annotate\n>> the line for the compiler to tell it not to warn about it, that would\n>> have been even better.\n>\n> We could do something like this:\n>\n> -- snip --\n> diff --git a/http.c b/http.c\n> index b08795715f8a..2ac8d51d3668 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot)\n> \tstruct timeval select_timeout;\n> \tint finished = 0;\n>\n> +#if __GNUC__ >= 12\n> +#pragma GCC diagnostic push\n> +#pragma GCC diagnostic ignored \"-Wdangling-pointer\"\n> +#endif\n> \tslot->finished = &finished;\n> +#if __GNUC__ >= 12\n> +#pragma GCC diagnostic pop\n> +#endif\n> \twhile (!finished) {\n> \t\tstep_active_slots();\n> -- snap --\n>\n> That's quite ugly, though. And what's worse, it is pretty unreadable, too.\n\nUnfortunately that sort of thing is a logic error as clang, ICC and\nprobably others are on a mission to make __GNUC__ as useless as\npossible:\nhttps://stackoverflow.com/questions/38499462/how-to-tell-clang-to-stop-pretending-to-be-other-compilers\n\nI think it *might* work in practice though, my local clang claims to be\ngcc 4, so maybe everyone faking it stops at a low enough version?\n\nBut did you spot 9c539d1027d (config.mak.dev: alternative workaround to\ngcc 12 warning in http.c, 2022-04-15)? We already disable this file-wide\nin config.mak.dev, but I didn't check if the Windows code was using that\n(or if you were targeting those without DEVELOPER=1).\n\nWe could move that to thake main Makefile, but then we'd have to call\ndetect-compiler there. I have some local patches to do something like\nthat if there's interest (rather, to bootstrap compilation by compiling\na C object and getting the macro values, instead of relying on that\nshellscript).\n"},{"id":"455983","messageId":"xmqqczg2eeth.fsf@gitster.g","threadId":"57914","inReplyTo":"nycvar.QRO.7.76.6.2205242309160.352@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 3/4] http.c: avoid danging pointer to local variable `finished`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-24T22:38:50Z","receivedAt":"2022-05-24T22:38:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Tue, 24 May 2022, Junio C Hamano wrote:\n>\n>> The \"clear slot->finished\", by the way, is what I think is the right\n>> thing to do, especially that the objective is to squelch the false\n>> positive warning from a new compiler. If there is a way to annotate\n>> the line for the compiler to tell it not to warn about it, that would\n>> have been even better.\n>\n> We could do something like this:\n\nYuck.\n\n> -- snip --\n> diff --git a/http.c b/http.c\n> index b08795715f8a..2ac8d51d3668 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot)\n> \tstruct timeval select_timeout;\n> \tint finished = 0;\n>\n> +#if __GNUC__ >= 12\n> +#pragma GCC diagnostic push\n> +#pragma GCC diagnostic ignored \"-Wdangling-pointer\"\n> +#endif\n> \tslot->finished = &finished;\n> +#if __GNUC__ >= 12\n> +#pragma GCC diagnostic pop\n> +#endif\n> \twhile (!finished) {\n> \t\tstep_active_slots();\n> -- snap --\n>\n> That's quite ugly, though. And what's worse, it is pretty unreadable, too.\n\nYes, very ugly. Would an unconditional\n\n\tslot->finished = NULL;\n\nat the end squelch the warning?\n\nOr there is a way to say \"we make all warnings into errors with\n-Werror, but we do not want to turn this dangling-pointer warning to\nan error, because it has false positives\"?\n\nOr we could add \"-Wno-dangling-pointer\" globally, perhaps.\n\n"},{"id":"456038","messageId":"nycvar.QRO.7.76.6.2205251208560.352@tvgsbejvaqbjf.bet","threadId":"57914","inReplyTo":"xmqqczg2eeth.fsf@gitster.g","subject":"Re: [PATCH 3/4] http.c: avoid danging pointer to local variable `finished`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-25T10:16:07Z","receivedAt":"2022-05-25T10:16:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 24 May 2022, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > On Tue, 24 May 2022, Junio C Hamano wrote:\n> >\n> >> The \"clear slot->finished\", by the way, is what I think is the right\n> >> thing to do, especially that the objective is to squelch the false\n> >> positive warning from a new compiler. If there is a way to annotate\n> >> the line for the compiler to tell it not to warn about it, that would\n> >> have been even better.\n> >\n> > We could do something like this:\n>\n> Yuck.\n>\n> > -- snip --\n> > diff --git a/http.c b/http.c\n> > index b08795715f8a..2ac8d51d3668 100644\n> > --- a/http.c\n> > +++ b/http.c\n> > @@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot)\n> > \tstruct timeval select_timeout;\n> > \tint finished = 0;\n> >\n> > +#if __GNUC__ >= 12\n> > +#pragma GCC diagnostic push\n> > +#pragma GCC diagnostic ignored \"-Wdangling-pointer\"\n> > +#endif\n> > \tslot->finished = &finished;\n> > +#if __GNUC__ >= 12\n> > +#pragma GCC diagnostic pop\n> > +#endif\n> > \twhile (!finished) {\n> > \t\tstep_active_slots();\n> > -- snap --\n> >\n> > That's quite ugly, though. And what's worse, it is pretty unreadable, too.\n>\n> Yes, very ugly. Would an unconditional\n>\n> \tslot->finished = NULL;\n>\n> at the end squelch the warning?\n\nNo, unfortunately not:\nhttps://github.com/dscho/git/actions/runs/2383492484\n\nAs you mentioned elsewhere, the error is clearly about the assignment in\nthe first place, and it does not matter that the function will rectify the\nsituation. It's the correct thing to do for the compiler, too: since\n`slot->finished` already has the pointer, and since the\n`active_request_slot` struct is declared globally, code outside the\ncurrent file might squirrel that pointer away for later use.\n\n> Or there is a way to say \"we make all warnings into errors with\n> -Werror, but we do not want to turn this dangling-pointer warning to\n> an error, because it has false positives\"?\n>\n> Or we could add \"-Wno-dangling-pointer\" globally, perhaps.\n\nI'd like to avoid that because it would quite likely hide legitimate\nissues elsewhere.\n\nIt currently seems to be the easiest solution to simply turn the local\nvariable into a heap variable:\n\n-- snip --\ndiff --git a/http.c b/http.c\nindex f92859f43fa..0712debd558 100644\n--- a/http.c\n+++ b/http.c\n@@ -1327,10 +1327,10 @@ void run_active_slot(struct active_request_slot *slot)\n \tfd_set excfds;\n \tint max_fd;\n \tstruct timeval select_timeout;\n-\tint finished = 0;\n+\tint *finished = xcalloc(1, sizeof(int));\n\n-\tslot->finished = &finished;\n-\twhile (!finished) {\n+\tslot->finished = finished;\n+\twhile (!*finished) {\n \t\tstep_active_slots();\n\n \t\tif (slot->in_use) {\n@@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)\n \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n \t\t}\n \t}\n+\tif (slot->finished == finished)\n+\t\tslot->finished = NULL;\n+\tfree(finished);\n }\n\n static void release_active_slot(struct active_request_slot *slot)\n-- snap --\n\nThis pacifies GCC (https://github.com/dscho/git/runs/6589617700) and is\nthe most minimally-intrusive work-around of which I am currently aware.\n\nCiao,\nDscho\n"},{"id":"456061","messageId":"220525.86k0a9u5rw.gmgdl@evledraar.gmail.com","threadId":"57914","inReplyTo":"nycvar.QRO.7.76.6.2205251208560.352@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 3/4] http.c: avoid danging pointer to local variable `finished`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-25T12:48:36Z","receivedAt":"2022-05-25T12:59:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, May 25 2022, Johannes Schindelin wrote:\n\n> Hi Junio,\n>\n> On Tue, 24 May 2022, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>>\n>> > On Tue, 24 May 2022, Junio C Hamano wrote:\n>> >\n>> >> The \"clear slot->finished\", by the way, is what I think is the right\n>> >> thing to do, especially that the objective is to squelch the false\n>> >> positive warning from a new compiler. If there is a way to annotate\n>> >> the line for the compiler to tell it not to warn about it, that would\n>> >> have been even better.\n>> >\n>> > We could do something like this:\n>>\n>> Yuck.\n>>\n>> > -- snip --\n>> > diff --git a/http.c b/http.c\n>> > index b08795715f8a..2ac8d51d3668 100644\n>> > --- a/http.c\n>> > +++ b/http.c\n>> > @@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot)\n>> > \tstruct timeval select_timeout;\n>> > \tint finished = 0;\n>> >\n>> > +#if __GNUC__ >= 12\n>> > +#pragma GCC diagnostic push\n>> > +#pragma GCC diagnostic ignored \"-Wdangling-pointer\"\n>> > +#endif\n>> > \tslot->finished = &finished;\n>> > +#if __GNUC__ >= 12\n>> > +#pragma GCC diagnostic pop\n>> > +#endif\n>> > \twhile (!finished) {\n>> > \t\tstep_active_slots();\n>> > -- snap --\n>> >\n>> > That's quite ugly, though. And what's worse, it is pretty unreadable, too.\n>>\n>> Yes, very ugly. Would an unconditional\n>>\n>> \tslot->finished = NULL;\n>>\n>> at the end squelch the warning?\n>\n> No, unfortunately not:\n> https://github.com/dscho/git/actions/runs/2383492484 \n\nYes it does. Didn't you just have a PBCAK between writing that test &\npushing it? Your\nhttps://github.com/dscho/git/blob/tmp/http.c#L1370-L1371 shows that you\ndidn't make that edit.\n\nThis on top of \"seen\" makes the warning go away:\n\t\n\t- if (slot->finished == &finished)\n\t- slot->finished = NULL;\n\t+ slot->finished = NULL;\n\nThis is also all covered in the initial thread from back in January,\nwhich if you're slowly re-discovering the learnings from there I\nencourage you to read in more detail... :)\n\n> As you mentioned elsewhere, the error is clearly about the assignment in\n> the first place, and it does not matter that the function will rectify the\n> situation. It's the correct thing to do for the compiler, too: since\n> `slot->finished` already has the pointer, and since the\n> `active_request_slot` struct is declared globally, code outside the\n> current file might squirrel that pointer away for later use.\n\nPer the above this isn't true, and in some side-thread replies (and in\nthe initial thread) I've linked to the GCC code in question. NULL-ing\nthe slot is sufficient, it doesn't matter that the struct it's in\nsurvives the function, just that the pointer isn't exposed.\n\n>> Or there is a way to say \"we make all warnings into errors with\n>> -Werror, but we do not want to turn this dangling-pointer warning to\n>> an error, because it has false positives\"?\n>>\n>> Or we could add \"-Wno-dangling-pointer\" globally, perhaps.\n>\n> I'd like to avoid that because it would quite likely hide legitimate\n> issues elsewhere.\n>\n> It currently seems to be the easiest solution to simply turn the local\n> variable into a heap variable:\n>\n> -- snip --\n> diff --git a/http.c b/http.c\n> index f92859f43fa..0712debd558 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1327,10 +1327,10 @@ void run_active_slot(struct active_request_slot *slot)\n> \tfd_set excfds;\n> \tint max_fd;\n> \tstruct timeval select_timeout;\n> -\tint finished = 0;\n> +\tint *finished = xcalloc(1, sizeof(int));\n>\n> -\tslot->finished = &finished;\n> -\twhile (!finished) {\n> +\tslot->finished = finished;\n> +\twhile (!*finished) {\n> \t\tstep_active_slots();\n>\n> \t\tif (slot->in_use) {\n> @@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)\n> \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n> \t\t}\n> \t}\n> +\tif (slot->finished == finished)\n> +\t\tslot->finished = NULL;\n> +\tfree(finished);\n> }\n\nAlso, if we were going to tweak this extensively I'd think this slightly\nlarger POC patch would be a better direction, i.e. we don't need a\npointer at all, we're just (ab)using it for a tri-state NULL/0/1, why\nnot just use an enum?\n\nI think the \"if it's what we just set it to\" is just cargo-culting, is\nthere anything to show that run_active_slot() is reentrant? Wouldn't we\nbe better off with a static variable + BUG() that we increment/decremant\nand panic if it's anything but 0 and 1 if we'd like to add paranoia\naround that?\n\ndiff --git a/http-walker.c b/http-walker.c\nindex b8f0f98ae14..26184a82ddb 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -225,13 +225,13 @@ static void process_alternates_response(void *callback_data)\n \t\t\t\t\t alt_req->url->buf);\n \t\t\tactive_requests++;\n \t\t\tslot->in_use = 1;\n-\t\t\tif (slot->finished)\n-\t\t\t\t(*slot->finished) = 0;\n+\t\t\tif (slot->f != FIN_UNSET)\n+ slot->f = FIN_NO;\n \t\t\tif (!start_active_slot(slot)) {\n \t\t\t\tcdata->got_alternates = -1;\n \t\t\t\tslot->in_use = 0;\n-\t\t\t\tif (slot->finished)\n-\t\t\t\t\t(*slot->finished) = 1;\n+ if (slot->f != FIN_UNSET)\n+ slot->f = FIN_YES;\n \t\t\t}\n \t\t\treturn;\n \t\t}\ndiff --git a/http.c b/http.c\nindex b148468b267..845dd40768c 100644\n--- a/http.c\n+++ b/http.c\n@@ -197,8 +197,8 @@ static void finish_active_slot(struct active_request_slot *slot)\n \tclosedown_active_slot(slot);\n \tcurl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);\n \n-\tif (slot->finished)\n-\t\t(*slot->finished) = 1;\n+\tif (slot->f != FIN_UNSET)\n+\t\tslot->f = FIN_YES;\n \n \t/* Store slot results so they can be read after the slot is reused */\n \tif (slot->results) {\n@@ -1204,7 +1204,7 @@ struct active_request_slot *get_active_slot(void)\n \tactive_requests++;\n \tslot->in_use = 1;\n \tslot->results = NULL;\n-\tslot->finished = NULL;\n+\tslot->f = FIN_UNSET;\n \tslot->callback_data = NULL;\n \tslot->callback_func = NULL;\n \tcurl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file);\n@@ -1327,10 +1327,9 @@ void run_active_slot(struct active_request_slot *slot)\n \tfd_set excfds;\n \tint max_fd;\n \tstruct timeval select_timeout;\n-\tint finished = 0;\n \n-\tslot->finished = &finished;\n-\twhile (!finished) {\n+\tslot->f = FIN_NO;\n+\twhile (slot->f == FIN_NO) {\n \t\tstep_active_slots();\n \n \t\tif (slot->in_use) {\ndiff --git a/http.h b/http.h\nindex df1590e53a4..fc664d90bc9 100644\n--- a/http.h\n+++ b/http.h\n@@ -19,12 +19,13 @@ struct slot_results {\n \tlong http_connectcode;\n };\n \n+enum fin { FIN_UNSET, FIN_NO, FIN_YES };\n struct active_request_slot {\n \tCURL *curl;\n \tint in_use;\n \tCURLcode curl_result;\n \tlong http_code;\n-\tint *finished;\n+\tenum fin f;\n \tstruct slot_results *results;\n \tvoid *callback_data;\n \tvoid (*callback_func)(void *data);\n"},{"id":"456066","messageId":"d7ee0bf0-22bc-d494-2699-7075fc92f28a@github.com","threadId":"57914","inReplyTo":"nycvar.QRO.7.76.6.2205242302020.352@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 4/4] dir.c: avoid \"exceeds maximum object size\" error with GCC v12.x","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-25T13:39:53Z","receivedAt":"2022-05-25T13:40:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/24/2022 5:05 PM, Johannes Schindelin wrote:> On Tue, 24 May 2022, Ævar Arnfjörð Bjarmason wrote:\n>> On Tue, May 24 2022, Johannes Schindelin via GitGitGadget wrote:\n>>> +\t/*\n>>> +\t * It should not be possible to overflow `ptrdiff_t` by passing in an\n>>> +\t * insanely long URL, but GCC does not know that and will complain\n>>> +\t * without this check.\n>>> +\t */\n>>> +\tif (end - start < 0)\n>>> +\t\tdie(_(\"No directory name could be guessed.\\n\"\n>>\n>> This should start with a lower-case letter, see CodingGuidelines.\n> \n> This message is copied from existing code later in the same function.\n> Since it is a translateable message, I do not want to edit it because that\n> would cause unnecessary work of the translators. Especially given that we\n> do not even expect this message to be shown, ever, but we only add this\n> hunk for GCC's benefit.\n\nPerhaps this should be a BUG() statement, then? Without any\ntranslation?\n\nThanks,\n-Stolee\n"},{"id":"456111","messageId":"xmqqzgj579ie.fsf@gitster.g","threadId":"57914","inReplyTo":"d7ee0bf0-22bc-d494-2699-7075fc92f28a@github.com","subject":"Re: [PATCH 4/4] dir.c: avoid \"exceeds maximum object size\" error with GCC v12.x","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-25T18:27:37Z","receivedAt":"2022-05-25T18:27:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 5/24/2022 5:05 PM, Johannes Schindelin wrote:> On Tue, 24 May 2022, Ævar Arnfjörð Bjarmason wrote:\n>>> On Tue, May 24 2022, Johannes Schindelin via GitGitGadget wrote:\n>>>> +\t/*\n>>>> +\t * It should not be possible to overflow `ptrdiff_t` by passing in an\n>>>> +\t * insanely long URL, but GCC does not know that and will complain\n>>>> +\t * without this check.\n>>>> +\t */\n>>>> +\tif (end - start < 0)\n>>>> +\t\tdie(_(\"No directory name could be guessed.\\n\"\n>>>\n>>> This should start with a lower-case letter, see CodingGuidelines.\n>> \n>> This message is copied from existing code later in the same function.\n>> Since it is a translateable message, I do not want to edit it because that\n>> would cause unnecessary work of the translators. Especially given that we\n>> do not even expect this message to be shown, ever, but we only add this\n>> hunk for GCC's benefit.\n>\n> Perhaps this should be a BUG() statement, then? Without any\n> translation?\n\nYeah, both are good. If somehow the caller managed to pass such a\nlong URL then it can be considered a data error at runtime, and not\nthat the user detected a bug in our code, so in that sense die()\nwould be appropriate. It is like xmalloc() running out of memory.\n\nOn the other hand, the \"should not be possible to overflow\" in the\ncomment implicitly assumes that it is impossible to pass insanely\nlong URL to trigger the condition from places we think of offhand,\nlike the command line, where the input is limited to a much shorter\nstring. As \"we detected a situation that should not happen unless\nthere is a programming or design bug\" is what BUG() means, it is\nalso good here---our assumption that this should not be possible\nturned out to be faulty, so we noticed a design bug.\n\nI wonder if we can add a separate macro to add more to the\ndocumentation value, though. With something like\n\n #define FALSE_WARNING(expression, message) \\\n\tdo { if (expression) { BUG(message); } while (0)\n\nthe above would just become\n\n\tFALSE_WARNING(end - start < 0, \"ptrdiff_t would not overflow here\");\n\nwithout a need for a big comment before it. We might even be able\nto optimize it out when building with compilers that do not need the\nworkaround.\n\n"}]}