compat/win32: correct for incorrect compiler warning

Commit Message

Derrick Stolee July 19, 2022, 6:45 p.m. UTC

From: Derrick Stolee <[e-mail]>

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 <[e-mail]>
---
    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(-)


base-commit: 71a8fab31b70c417e8f5b5f716581f89955a7082

Comments

Eric Sunshine July 19, 2022, 7:48 p.m. UTC | #1

On Tue, Jul 19, 2022 at 2:48 PM Derrick Stolee via GitGitGadget
<[e-mail]> 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.

> 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 <[e-mail]>

Derrick Stolee July 19, 2022, 8:01 p.m. UTC | #2

On 7/19/2022 3:48 PM, Eric Sunshine wrote:
> On Tue, Jul 19, 2022 at 2:48 PM Derrick Stolee via GitGitGadget
> <[e-mail]> 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.
 
>> 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 July 19, 2022, 8:13 p.m. UTC | #3

"Derrick Stolee via GitGitGadget" <[e-mail]> writes:

> 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 July 21, 2022, 2:17 p.m. UTC | #4

Hi Stolee

> 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

Patch

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) {