compat/win32: correct for incorrect compiler warning
PATCH 1/1 compat/win32: correct for incorrect compiler warning
From: Derrick Stolee <[email protected]>
The 'win build' job of our CI build is failing with the following error:
compat/win32/syslog.c: In function 'syslog':compat/win32/syslog.c:53:17: error: pointer 'pos' may be used after \
'realloc' [-Werror=use-after-free]
53 | memmove(pos + 2, pos + 1, strlen(pos));
CC compat/poll/poll.o
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compat/win32/syslog.c:47:23: note: call to 'realloc' here
47 | str = realloc(str, st_add(++str_len, 1));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~However, between this realloc() and the use we have a line that resets the value of 'pos'. Thus, this error is incorrect. It is likely due to a new version of the compiler on the CI machines.
Instead of waiting for a new compiler, create a new variable to avoid this error.
---
Fix error in response to compiler bug
See [1] for an example of this error in the wild.
[1]
https://github.com/gitgitgadget/git/runs/7413762368?check_suite_focus=true#step:4:297
Thanks, -Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1294%2Fderrickstolee%2Frealloc-error-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1294/derrickstolee/realloc-error-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1294
compat/win32/syslog.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
compat/win32/syslog.c +4 -3
@@ -44,6 +44,7 @@ void syslog(int priority, const char *fmt, ...)while ((pos =strstr (str,"%1" )) !=NULL ) {size_t offset = pos - str; +char *new_pos;char *oldstr = str; str =realloc (str,st_add (++str_len,1 ));if (!str) {
@@ -51,9 +52,9 @@ void syslog(int priority, const char *fmt, ...)warning_errno ("realloc failed" );return ; } - pos = str + offset; -memmove (pos +2 , pos +1 ,strlen (pos)); - pos[1 ] =' ' ; + new_pos = str + offset; +memmove (new_pos +2 , new_pos +1 ,strlen (new_pos)); + new_pos[1 ] =' ' ; }switch (priority) {
base-commit: 71a8fab31b70c417e8f5b5f716581f89955a7082
Signature · 1 line
gitgitgadget
Quoted 11 lines · Derrick Stolee via GitGitGadget
The 'win build' job of our CI build is failing with the following error:
compat/win32/syslog.c: In function 'syslog':compat/win32/syslog.c:53:17: error: pointer 'pos' may be used after \
'realloc' [-Werror=use-after-free]53 | memmove(pos + 2, pos + 1, strlen(pos)); CC compat/poll/poll.o | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compat/win32/syslog.c:47:23: note: call to 'realloc' here 47 | str = realloc(str, st_add(++str_len, 1)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Removing the unrelated "CC compat/poll/poll.o" line would help make this output less confusing.
Quoted 6 lines
However, between this
realloc()and the use we have a line that resets the value of 'pos'. Thus, this error is incorrect. It is likely due to a new version of the compiler on the CI machines.Instead of waiting for a new compiler, create a new variable to avoid this error.
If possible, it is a good idea to mention the actual compiler version in the commit message as an aid to future readers who might want to know if this sort of workaround is still needed.
Quoted 1 line
Signed-off-by Derrick Stolee <[email protected]>
Quoted 14 lines · Eric Sunshine
Quoted 11 lines · Derrick Stolee via GitGitGadget
The 'win build' job of our CI build is failing with the following error:
compat/win32/syslog.c: In function 'syslog':compat/win32/syslog.c:53:17: error: pointer 'pos' may be used after \
'realloc' [-Werror=use-after-free]53 | memmove(pos + 2, pos + 1, strlen(pos)); CC compat/poll/poll.o | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compat/win32/syslog.c:47:23: note: call to 'realloc' here 47 | str = realloc(str, st_add(++str_len, 1)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Quoted 2 lines
Removing the unrelated "CC
compat/poll/poll.o" line would help make this output less confusing.
Yes, sorry. It's output from the parallel build that I should have noticed.
Quoted 10 lines
Quoted 6 lines
However, between this
realloc()and the use we have a line that resets the value of 'pos'. Thus, this error is incorrect. It is likely due to a new version of the compiler on the CI machines.Instead of waiting for a new compiler, create a new variable to avoid this error.
If possible, it is a good idea to mention the actual compiler version in the commit message as an aid to future readers who might want to know if this sort of workaround is still needed.
I wish I knew. I can only guess that it's a new GCC version on the Windows agents for GitHub Actions, but I don't know exactly which one.
Thanks,
-Stolee
Quoted 13 lines · Derrick Stolee via GitGitGadget
compat/win32/syslog.c +1
-0@@ -44,6 +44,7 @@ void syslog(int priority, const char *fmt, ...)while ((pos =strstr (str,"%1" )) !=NULL ) {size_t offset = pos - str; +char *new_pos;char *oldstr = str; str =realloc (str,st_add (++str_len,1 ));if (!str) {
compat/win32/syslog.c void syslog(int priority, const char *fmt, ...) · line 58
Quoted 10 lines
compat/win32/syslog.c +3
-3@@ -51,9 +52,9 @@ void syslog(int priority, const char *fmt, ...)warning_errno ("realloc failed" );return ; } - pos = str + offset; -memmove (pos +2 , pos +1 ,strlen (pos)); - pos[1 ] =' ' ; + new_pos = str + offset; +memmove (new_pos +2 , new_pos +1 ,strlen (new_pos)); + new_pos[1 ] =' ' ; }
Heh, after this sequence
pos = strstr(str, ...); str = realloc(str, ...); pos = str + something;
the compiler warns about use of pos[] as if resetting of pos based on the new value of str[] did not even exist?
Discovering a working work-around must have been a pain.
If I were writing this, I'd probably
* avoid re-scanning "str" from the beginning in every iteration;
* possibly avoid reallocating for %1 by first scanning and counting the problematic "%1";
* also possibly give a deeper thought to see if "%1" -> "% 1" is necessary. We are munging the original string anyway---in the output that has "% 1", you cannot tell if the original was "%1" that was munged by this code, or it was "% 1" from the beginning. In other words, we are already willing to lose information. Perhaps we can find a different replacement sequence that is still 2-byte-wide and that is unlikely to appear in the original, eliminating the need for reallocation altogether?
But none of that is a problem with this patch.
Will queue. Thanks.
Hi Stolee
compat/win32/syslog.c void syslog(int priority, const char *fmt, ...) · line 49
Quoted 11 lines
compat/win32/syslog.c +1
-0@@ -44,6 +44,7 @@ void syslog(int priority, const char *fmt, ...)while ((pos =strstr (str,"%1" )) !=NULL ) {size_t offset = pos - str; +char *new_pos;char *oldstr = str; str =realloc (str,st_add (++str_len,1 ));
Not related to your patch but this context line looks suspicious as str_len is incremented without checking for overflow. I did wonder if it should be using a post increment instead and was using the st_add() to check if that overflowed but str_len is an int so we'd still have undefined behavior.
Best Wishes
Phillip