[1/4] compat/win32/syslog: fix use-after-realloc

Commit Message

Johannes Schindelin May 24, 2022, 12:23 a.m. UTC

From: Johannes Schindelin <[e-mail]>

Git for Windows' SDK recently upgraded to GCC v12.x which points out
that the `pos` variable might be used even after the corresponding
memory was `realloc()`ed and therefore potentially no longer valid.

Since a subset of this SDK is used in Git's CI/PR builds, we need to fix
this to continue to be able to benefit from the CI/PR runs.

Note: This bug has been with us since 2a6b149c64f6 (mingw: avoid using
strbuf in syslog, 2011-10-06), and while it looks tempting to replace
the hand-rolled string manipulation with a `strbuf`-based one, that
commit's message explains why we cannot do that: The `syslog()` function
is called as part of the function in `daemon.c` which is set as the
`die()` routine, and since `strbuf_grow()` can call that function if it
runs out of memory, this would cause a nasty infinite loop that we do
not want to re-introduce.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 compat/win32/syslog.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Johannes Schindelin May 24, 2022, 12:39 p.m. UTC | #1

Hi,

On Tue, 24 May 2022, Johannes Schindelin via GitGitGadget wrote:

> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
> index 161978d720a..1f8d8934cc9 100644
> --- a/compat/win32/syslog.c
> +++ b/compat/win32/syslog.c
> @@ -43,6 +43,7 @@ void syslog(int priority, const char *fmt, ...)
>  	va_end(ap);
>
>  	while ((pos = strstr(str, "%1")) != NULL) {
> +		size_t offset = pos - str;
>  		char *oldstr = str;
>  		str = realloc(str, st_add(++str_len, 1));

Since it has been raised elsewhere: Why is that `++str_len` not turned
into an `st_add()`?

The commit adding that `st_add()` call (50a6c8efa2b (use st_add and
st_mult for allocation size computation, 2016-02-22)) does not really talk
about it, but the explanation is simple:

Before this `while()` loop, we allocate one more than `str_len` (see
compat/win32/syslog.c#L35), and we do that already using `st_add()`, so
that the string and the terminating NUL fit into the allocated memory.

Therefore, the first time we enter the loop, we know that `++str_len` is
safe.

Now, in this very line, we then increment `str_len` and then `reallocate`
one more than that, again guarding it via `st_add()`. So every subsequent
iteration will already have checked that `++str_len` is safe, too.

By induction (https://en.wikipedia.org/wiki/Mathematical_induction), it
follows that this line is safe, and we do not have to change it to a
clunkier two-step assignment where we first use `st_add()` to increment
`str_len` and then use `st_add()` to allocate enough memory to also fit
the trailing NUL.

Now you know,
Dscho

>  		if (!str) {
> @@ -50,6 +51,7 @@ void syslog(int priority, const char *fmt, ...)
>  			warning_errno("realloc failed");
>  			return;
>  		}
> +		pos = str + offset;
>  		memmove(pos + 2, pos + 1, strlen(pos));
>  		pos[1] = ' ';
>  	}
> --
> gitgitgadget
>
>

Junio C Hamano May 24, 2022, 8:58 p.m. UTC | #2

Johannes Schindelin <[e-mail]> writes:

>>  		str = realloc(str, st_add(++str_len, 1));
>
> Since it has been raised elsewhere: Why is that `++str_len` not turned
> into an `st_add()`?
> ...
> Now you know,

I'd be more worried about a macro looking thing evalutating its
parameters more than once, though.  But unlike st_addN(), st_add()
is an inline function so we do not have to worry about that ;-)

>> @@ -50,6 +51,7 @@ void syslog(int priority, const char *fmt, ...)
>>  			warning_errno("realloc failed");
>>  			return;
>>  		}
>> +		pos = str + offset;

The adjustment using ofs is very much straight-forward.  Nicely
spotted and nicely corrected.

Thanks.

Patch

diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index 161978d720a..1f8d8934cc9 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -43,6 +43,7 @@  void syslog(int priority, const char *fmt, ...)
 	va_end(ap);
 
 	while ((pos = strstr(str, "%1")) != NULL) {
+		size_t offset = pos - str;
 		char *oldstr = str;
 		str = realloc(str, st_add(++str_len, 1));
 		if (!str) {
@@ -50,6 +51,7 @@  void syslog(int priority, const char *fmt, ...)
 			warning_errno("realloc failed");
 			return;
 		}
+		pos = str + offset;
 		memmove(pos + 2, pos + 1, strlen(pos));
 		pos[1] = ' ';
 	}