{"thread":{"id":"58190","subject":"[PATCH] compat/win32: correct for incorrect compiler warning","startedAt":"2022-07-19T18:46:00Z","lastAt":"2022-07-21T14:17:49Z","messageCount":5,"participants":["Derrick Stolee via GitGitGadget","Eric Sunshine","Derrick Stolee","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"459425","messageId":"pull.1294.git.1658256354725.gitgitgadget@gmail.com","threadId":"58190","inReplyTo":null,"subject":"[PATCH] compat/win32: correct for incorrect compiler warning","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-19T18:45:54Z","receivedAt":"2022-07-19T18:46:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nThe 'win build' job of our CI build is failing with the following error:\n\ncompat/win32/syslog.c: In function 'syslog':\ncompat/win32/syslog.c:53:17: error: pointer 'pos' may be used after \\\n\t\t\t\t    'realloc' [-Werror=use-after-free]\n   53 |                 memmove(pos + 2, pos + 1, strlen(pos));\n    CC compat/poll/poll.o\n      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\ncompat/win32/syslog.c:47:23: note: call to 'realloc' here\n   47 |                 str = realloc(str, st_add(++str_len, 1));\n      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\nHowever, between this realloc() and the use we have a line that resets\nthe value of 'pos'. Thus, this error is incorrect. It is likely due to a\nnew version of the compiler on the CI machines.\n\nInstead of waiting for a new compiler, create a new variable to avoid\nthis error.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n    Fix error in response to compiler bug\n    \n    See [1] for an example of this error in the wild.\n    \n    [1]\n    https://github.com/gitgitgadget/git/runs/7413762368?check_suite_focus=true#step:4:297\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1294%2Fderrickstolee%2Frealloc-error-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1294/derrickstolee/realloc-error-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1294\n\n compat/win32/syslog.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/compat/win32/syslog.c b/compat/win32/syslog.c\nindex 1f8d8934cc9..0af18d88825 100644\n--- a/compat/win32/syslog.c\n+++ b/compat/win32/syslog.c\n@@ -44,6 +44,7 @@ void syslog(int priority, const char *fmt, ...)\n \n \twhile ((pos = strstr(str, \"%1\")) != NULL) {\n \t\tsize_t offset = pos - str;\n+\t\tchar *new_pos;\n \t\tchar *oldstr = str;\n \t\tstr = realloc(str, st_add(++str_len, 1));\n \t\tif (!str) {\n@@ -51,9 +52,9 @@ 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\tnew_pos = str + offset;\n+\t\tmemmove(new_pos + 2, new_pos + 1, strlen(new_pos));\n+\t\tnew_pos[1] = ' ';\n \t}\n \n \tswitch (priority) {\n\nbase-commit: 71a8fab31b70c417e8f5b5f716581f89955a7082\n-- \ngitgitgadget\n"},{"id":"459429","messageId":"CAPig+cSPnQnw82DsDHhMP12sYW_XHUb6+MYt17V=0sWrW4j2iw@mail.gmail.com","threadId":"58190","inReplyTo":"pull.1294.git.1658256354725.gitgitgadget@gmail.com","subject":"Re: [PATCH] compat/win32: correct for incorrect compiler warning","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-07-19T19:48:12Z","receivedAt":"2022-07-19T19:48:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 19, 2022 at 2:48 PM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> The 'win build' job of our CI build is failing with the following error:\n>\n> compat/win32/syslog.c: In function 'syslog':\n> compat/win32/syslog.c:53:17: error: pointer 'pos' may be used after \\\n>                                     'realloc' [-Werror=use-after-free]\n>    53 |                 memmove(pos + 2, pos + 1, strlen(pos));\n>     CC compat/poll/poll.o\n>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> compat/win32/syslog.c:47:23: note: call to 'realloc' here\n>    47 |                 str = realloc(str, st_add(++str_len, 1));\n>       |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\nRemoving the unrelated \"CC compat/poll/poll.o\" line would help make\nthis output less confusing.\n\n> However, between this realloc() and the use we have a line that resets\n> the value of 'pos'. Thus, this error is incorrect. It is likely due to a\n> new version of the compiler on the CI machines.\n>\n> Instead of waiting for a new compiler, create a new variable to avoid\n> this error.\n\nIf possible, it is a good idea to mention the actual compiler version\nin the commit message as an aid to future readers who might want to\nknow if this sort of workaround is still needed.\n\n> Signed-off-by: Derrick Stolee <derrickstolee@github.com>\n"},{"id":"459430","messageId":"8dbcd979-0794-65c9-2192-62d4bfc8ef62@github.com","threadId":"58190","inReplyTo":"CAPig+cSPnQnw82DsDHhMP12sYW_XHUb6+MYt17V=0sWrW4j2iw@mail.gmail.com","subject":"Re: [PATCH] compat/win32: correct for incorrect compiler warning","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-07-19T20:01:38Z","receivedAt":"2022-07-19T20:01:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/19/2022 3:48 PM, Eric Sunshine wrote:\n> On Tue, Jul 19, 2022 at 2:48 PM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>> The 'win build' job of our CI build is failing with the following error:\n>>\n>> compat/win32/syslog.c: In function 'syslog':\n>> compat/win32/syslog.c:53:17: error: pointer 'pos' may be used after \\\n>>                                     'realloc' [-Werror=use-after-free]\n>>    53 |                 memmove(pos + 2, pos + 1, strlen(pos));\n>>     CC compat/poll/poll.o\n>>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n>> compat/win32/syslog.c:47:23: note: call to 'realloc' here\n>>    47 |                 str = realloc(str, st_add(++str_len, 1));\n>>       |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> \n> Removing the unrelated \"CC compat/poll/poll.o\" line would help make\n> this output less confusing.\n\nYes, sorry. It's output from the parallel build that I should have noticed.\n \n>> However, between this realloc() and the use we have a line that resets\n>> the value of 'pos'. Thus, this error is incorrect. It is likely due to a\n>> new version of the compiler on the CI machines.\n>>\n>> Instead of waiting for a new compiler, create a new variable to avoid\n>> this error.\n> \n> If possible, it is a good idea to mention the actual compiler version\n> in the commit message as an aid to future readers who might want to\n> know if this sort of workaround is still needed.\n\nI wish I knew. I can only guess that it's a new GCC version on the\nWindows agents for GitHub Actions, but I don't know exactly which one.\n\nThanks,\n-Stolee\n"},{"id":"459431","messageId":"xmqqk088amj4.fsf@gitster.g","threadId":"58190","inReplyTo":"pull.1294.git.1658256354725.gitgitgadget@gmail.com","subject":"Re: [PATCH] compat/win32: correct for incorrect compiler warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-19T20:13:35Z","receivedAt":"2022-07-19T20:13:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c\n> index 1f8d8934cc9..0af18d88825 100644\n> --- a/compat/win32/syslog.c\n> +++ b/compat/win32/syslog.c\n> @@ -44,6 +44,7 @@ void syslog(int priority, const char *fmt, ...)\n>  \n>  \twhile ((pos = strstr(str, \"%1\")) != NULL) {\n>  \t\tsize_t offset = pos - str;\n> +\t\tchar *new_pos;\n>  \t\tchar *oldstr = str;\n>  \t\tstr = realloc(str, st_add(++str_len, 1));\n>  \t\tif (!str) {\n> @@ -51,9 +52,9 @@ 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\tnew_pos = str + offset;\n> +\t\tmemmove(new_pos + 2, new_pos + 1, strlen(new_pos));\n> +\t\tnew_pos[1] = ' ';\n>  \t}\n\nHeh, after this sequence\n\n\tpos = strstr(str, ...);\n\tstr = realloc(str, ...);\n\tpos = str + something;\n\nthe compiler warns about use of pos[] as if resetting of pos based\non the new value of str[] did not even exist?\n\nDiscovering a working work-around must have been a pain.\n\nIf I were writing this, I'd probably\n\n * avoid re-scanning \"str\" from the beginning in every iteration;\n\n * possibly avoid reallocating for %1 by first scanning and counting\n   the problematic \"%1\";\n\n * also possibly give a deeper thought to see if \"%1\" -> \"% 1\" is\n   necessary.  We are munging the original string anyway---in the\n   output that has \"% 1\", you cannot tell if the original was \"%1\"\n   that was munged by this code, or it was \"% 1\" from the beginning.\n   In other words, we are already willing to lose information.\n   Perhaps we can find a different replacement sequence that is\n   still 2-byte-wide and that is unlikely to appear in the original,\n   eliminating the need for reallocation altogether?\n\nBut none of that is a problem with this patch.\n\nWill queue.  Thanks.\n\n\n"},{"id":"459627","messageId":"653c6a24-df30-2b06-4cf7-f01bd6012d55@gmail.com","threadId":"58190","inReplyTo":"pull.1294.git.1658256354725.gitgitgadget@gmail.com","subject":"Re: [PATCH] compat/win32: correct for incorrect compiler warning","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-07-21T14:17:43Z","receivedAt":"2022-07-21T14:17:49Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stolee\n\n> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c\n> index 1f8d8934cc9..0af18d88825 100644\n> --- a/compat/win32/syslog.c\n> +++ b/compat/win32/syslog.c\n> @@ -44,6 +44,7 @@ void syslog(int priority, const char *fmt, ...)\n>   \n>   \twhile ((pos = strstr(str, \"%1\")) != NULL) {\n>   \t\tsize_t offset = pos - str;\n> +\t\tchar *new_pos;\n>   \t\tchar *oldstr = str;\n>   \t\tstr = realloc(str, st_add(++str_len, 1));\n\nNot related to your patch but this context line looks suspicious as \nstr_len is incremented without checking for overflow. I did wonder if it \nshould be using a post increment instead and was using the st_add() to \ncheck if that overflowed but str_len is an int so we'd still have \nundefined behavior.\n\nBest Wishes\n\nPhillip\n"}]}