Re: [PATCH 1/4] compat/win32/syslog: fix use-after-realloc
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- May 24, 2022, 12:39 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2205241431400.352@tvgsbejvaqbjf.bet>
- In-Reply-To
- <d0ade6531bc77b654c28cf7b1bfa42523150c015.1653351786.git.gitgitgadget@gmail.com>
Hi,
On Tue, 24 May 2022, Johannes Schindelin via GitGitGadget wrote:
Show 11 quoted lines
> 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
Show 13 quoted lines
> 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
>
>