threads / patch / 58190

patchcompat/win32: correct for incorrect compiler warning

Subject: [PATCH] compat/win32: correct for incorrect compiler warning

## tl;dr

5 messages between Jul 19, 2022 and Jul 21, 2022. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

Derrick Stolee via GitGitGadget· Jul 19, 2022, 18:45 UTC · lore
From: Derrick Stolee <derrickstolee@github.com>
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.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
    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(-)
Show changes to compat/win32/syslog.c +4 −3
diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index 1f8d8934cc9..0af18d88825 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -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

-- 
gitgitgadget
Eric Sunshine· Jul 19, 2022, 19:48 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH] compat/win32: correct for incorrect compiler warning

On Tue, Jul 19, 2022 at 2:48 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 11 quoted lines
> 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.

Show 6 quoted 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.

> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
Derrick Stolee· Jul 19, 2022, 20:01 UTC · re: Eric Sunshine · lore

Re: [PATCH] compat/win32: correct for incorrect compiler warning

On 7/19/2022 3:48 PM, Eric Sunshine wrote:
Show 16 quoted lines
> On Tue, Jul 19, 2022 at 2:48 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 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.
Yes, sorry. It's output from the parallel build that I should have noticed.
Show 10 quoted 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

Junio C Hamano· Jul 19, 2022, 20:13 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH] compat/win32: correct for incorrect compiler warning

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 23 quoted lines
> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
> index 1f8d8934cc9..0af18d88825 100644
> --- a/compat/win32/syslog.c
> +++ b/compat/win32/syslog.c
> @@ -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] = ' ';
>  	}
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.
Phillip Wood· Jul 21, 2022, 14:17 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH] compat/win32: correct for incorrect compiler warning

Hi Stolee
Show 11 quoted lines
> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
> index 1f8d8934cc9..0af18d88825 100644
> --- a/compat/win32/syslog.c
> +++ b/compat/win32/syslog.c
> @@ -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

← back to recent threads