[v4,2/2] stdio-common: Fix buffer overflow in scanf %mc [BZ #34008]
Checks
| Context |
Check |
Description |
| redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Build passed
|
| redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
| linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Test passed
|
Commit Message
* stdio-common/vfscanf-internal.c: When enlarging allocated buffer with
format %mc or %mC, glibc allocates one byte less, leading to
user-controlled one byte overflow. This commit fixes BZ #34008, or
CVE-2026-5450. Unify newsize calculation of allocated buffer.
Signed-off-by: Rocket Ma <marocketbd@gmail.com>
---
stdio-common/vfscanf-internal.c | 74 ++++++++++++++++++++-------------
1 file changed, 46 insertions(+), 28 deletions(-)
Comments
* Rocket Ma:
> * stdio-common/vfscanf-internal.c: When enlarging allocated buffer with
> format %mc or %mC, glibc allocates one byte less, leading to
> user-controlled one byte overflow. This commit fixes BZ #34008, or
> CVE-2026-5450. Unify newsize calculation of allocated buffer.
>
> Signed-off-by: Rocket Ma <marocketbd@gmail.com>
> ---
> stdio-common/vfscanf-internal.c | 74 ++++++++++++++++++++-------------
> 1 file changed, 46 insertions(+), 28 deletions(-)
>
> diff --git a/stdio-common/vfscanf-internal.c b/stdio-common/vfscanf-internal.c
> index 59fc8208aa..6bf2a55876 100644
> --- a/stdio-common/vfscanf-internal.c
> +++ b/stdio-common/vfscanf-internal.c
> @@ -265,6 +265,19 @@ char_buffer_add (struct char_buffer *buffer, CHAR_T ch)
> *buffer->current++ = ch;
> }
>
> +/* Calculate the result size of expanded char array in %ms, %mS,
> + %m[, %lm[, %mc or %mC. */
> +static __always_inline size_t
> +grow_to_fit (size_t oldsize, int need, int extra)
> +{
> + /* extra = 0 if %m[cC], %m[cC] always have positive width */
> + if ((extra && need < 0) || oldsize < need)
> + return oldsize * 2;
> + /* oldsize >= need:
> + grow requested capacity and `extra' byte for `\0' */
> + return oldsize + need + extra;
> +}
Thanks for working on this.
The last (extra) argument is constant. I'd suggest two functions with
descriptive names instead (maybe grow_to_fit_for_fixed for the %c
family, and grow_to_fit_with_null for the %s/%[] family that perform
null termination). The new functions probably shouldn't be inline, so
that the compiler can apply its heuristics.
As written, the interaction with the extra argument and a negative
need argument is not quite obvious from the function alone (code and
comments). The function seems correct because need (called width in
the caller) can only be -1 for the %s/%[] case, where extra is 1.
Florian Weimer <fw@deneb.enyo.de> 于2026年4月6日周一 17:00写道：
> The last (extra) argument is constant. I'd suggest two functions with
> descriptive names instead (maybe grow_to_fit_for_fixed for the %c
> family, and grow_to_fit_with_null for the %s/%[] family that perform
> null termination). The new functions probably shouldn't be inline, so
> that the compiler can apply its heuristics.
If the function need to be separated, then the old behavior, "size_t
newsize = strsize + (strsize >= width ? width : strsize)", is not
worth a new function. And the function only expands to several
instructions, less than 10, observed via Compiler Explorer. I think
it's OK to inline the function.
> As written, the interaction with the extra argument and a negative
> need argument is not quite obvious from the function alone (code and
> comments). The function seems correct because need (called width in
> the caller) can only be -1 for the %s/%[] case, where extra is 1.
Since the behavior mentioned has become some sort of convention, the
function should be OK? Readers has the constant to distinguish between
%ms and %mc, and they can understand the code via enough comments.
Personally I think it's worth to put these two actions together to do
one thing: calculate the size of expanded array.
> If the function need to be separated, then the old behavior, "size_t
> newsize = strsize + (strsize >= width ? width : strsize)", is not
> worth a new function. And the function only expands to several
> instructions, less than 10, observed via Compiler Explorer. I think
> it's OK to inline the function.
>
> Since the behavior mentioned has become some sort of convention, the
> function should be OK? Readers has the constant to distinguish between
> %ms and %mc, and they can understand the code via enough comments.
> Personally I think it's worth to put these two actions together to do
> one thing: calculate the size of expanded array.
Any idea?
On 4/8/26 12:13 PM, Rocket Ma wrote:
>> If the function need to be separated, then the old behavior, "size_t
>> newsize = strsize + (strsize >= width ? width : strsize)", is not
>> worth a new function. And the function only expands to several
>> instructions, less than 10, observed via Compiler Explorer. I think
>> it's OK to inline the function.
>>
>> Since the behavior mentioned has become some sort of convention, the
>> function should be OK? Readers has the constant to distinguish between
>> %ms and %mc, and they can understand the code via enough comments.
>> Personally I think it's worth to put these two actions together to do
>> one thing: calculate the size of expanded array.
>
> Any idea?
>
The best way forward is to simplify and just fix the %mc case for bug
34008 and the CVE.
Then we can have a follow-on changes that refactors?
@@ -265,6 +265,19 @@ char_buffer_add (struct char_buffer *buffer, CHAR_T ch)
*buffer->current++ = ch;
}
+/* Calculate the result size of expanded char array in %ms, %mS,
+ %m[, %lm[, %mc or %mC. */
+static __always_inline size_t
+grow_to_fit (size_t oldsize, int need, int extra)
+{
+ /* extra = 0 if %m[cC], %m[cC] always have positive width */
+ if ((extra && need < 0) || oldsize < need)
+ return oldsize * 2;
+ /* oldsize >= need:
+ grow requested capacity and `extra' byte for `\0' */
+ return oldsize + need + extra;
+}
+
/* Read formatted input from S according to the format string
FORMAT, using the argument list in ARG.
Return the number of assignments made, or -1 for an input error. */
@@ -804,7 +817,8 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
&& *strptr + strsize - str <= MB_LEN_MAX)
{
/* We have to enlarge the buffer if the `m' flag
- was given. */
+ was given. And we may not expand str by width
+ as the wcrtomb may return various bytes */
size_t strleng = str - *strptr;
char *newstr;
@@ -854,9 +868,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
&& (char *) str == *strptr + strsize)
{
/* Enlarge the buffer. */
- size_t newsize
- = strsize
- + (strsize >= width ? width - 1 : strsize);
+ size_t newsize = grow_to_fit (strsize, width, 0);
str = (char *) realloc (*strptr, newsize);
if (str == NULL)
@@ -928,8 +940,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
if ((flags & MALLOC)
&& wstr == (wchar_t *) *strptr + strsize)
{
- size_t newsize
- = strsize + (strsize > width ? width - 1 : strsize);
+ size_t newsize = grow_to_fit (strsize, width, 0);
/* Enlarge the buffer. */
wstr = (wchar_t *) realloc (*strptr,
newsize * sizeof (wchar_t));
@@ -983,8 +994,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
if (!(flags & SUPPRESS) && (flags & MALLOC)
&& wstr == (wchar_t *) *strptr + strsize)
{
- size_t newsize
- = strsize + (strsize > width ? width - 1 : strsize);
+ size_t newsize = grow_to_fit (strsize, width, 0);
/* Enlarge the buffer. */
wstr = (wchar_t *) realloc (*strptr,
newsize * sizeof (wchar_t));
@@ -1099,7 +1109,8 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
&& *strptr + strsize - str <= MB_LEN_MAX)
{
/* We have to enlarge the buffer if the `a' or `m'
- flag was given. */
+ flag was given. And we may not expand str by
+ width as the wcrtomb may return various bytes */
size_t strleng = str - *strptr;
char *newstr;
@@ -1157,7 +1168,8 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
&& (char *) str == *strptr + strsize)
{
/* Enlarge the buffer. */
- str = (char *) realloc (*strptr, 2 * strsize);
+ size_t newsize = grow_to_fit (strsize, width, 1);
+ str = (char *) realloc (*strptr, newsize);
if (str == NULL)
{
/* Can't allocate that much. Last-ditch
@@ -1189,7 +1201,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
{
*strptr = (char *) str;
str += strsize;
- strsize *= 2;
+ strsize = newsize;
}
}
}
@@ -1287,9 +1299,10 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
&& wstr == (wchar_t *) *strptr + strsize)
{
/* Enlarge the buffer. */
- wstr = (wchar_t *) realloc (*strptr,
- (2 * strsize)
- * sizeof (wchar_t));
+ size_t newsize = grow_to_fit (strsize, width, 1);
+
+ wstr = (wchar_t *) realloc (
+ *strptr, newsize * sizeof (wchar_t));
if (wstr == NULL)
{
/* Can't allocate that much. Last-ditch
@@ -1323,7 +1336,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
{
*strptr = (char *) wstr;
wstr += strsize;
- strsize *= 2;
+ strsize = newsize;
}
}
}
@@ -1363,9 +1376,10 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
&& wstr == (wchar_t *) *strptr + strsize)
{
/* Enlarge the buffer. */
+ size_t newsize = grow_to_fit (strsize, width, 1);
+
wstr = (wchar_t *) realloc (*strptr,
- (2 * strsize
- * sizeof (wchar_t)));
+ newsize * sizeof (wchar_t));
if (wstr == NULL)
{
/* Can't allocate that much. Last-ditch effort. */
@@ -1398,7 +1412,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
{
*strptr = (char *) wstr;
wstr += strsize;
- strsize *= 2;
+ strsize = newsize;
}
}
}
@@ -2755,9 +2769,10 @@ digits_extended_fail:
&& wstr == (wchar_t *) *strptr + strsize)
{
/* Enlarge the buffer. */
- wstr = (wchar_t *) realloc (*strptr,
- (2 * strsize)
- * sizeof (wchar_t));
+ size_t newsize = grow_to_fit (strsize, width, 1);
+
+ wstr = (wchar_t *) realloc (
+ *strptr, newsize * sizeof (wchar_t));
if (wstr == NULL)
{
/* Can't allocate that much. Last-ditch
@@ -2791,7 +2806,7 @@ digits_extended_fail:
{
*strptr = (char *) wstr;
wstr += strsize;
- strsize *= 2;
+ strsize = newsize;
}
}
}
@@ -2840,9 +2855,10 @@ digits_extended_fail:
&& wstr == (wchar_t *) *strptr + strsize)
{
/* Enlarge the buffer. */
- wstr = (wchar_t *) realloc (*strptr,
- (2 * strsize
- * sizeof (wchar_t)));
+ size_t newsize = grow_to_fit (strsize, width, 1);
+
+ wstr = (wchar_t *) realloc (
+ *strptr, newsize * sizeof (wchar_t));
if (wstr == NULL)
{
/* Can't allocate that much. Last-ditch
@@ -2876,7 +2892,7 @@ digits_extended_fail:
{
*strptr = (char *) wstr;
wstr += strsize;
- strsize *= 2;
+ strsize = newsize;
}
}
}
@@ -2984,7 +3000,9 @@ digits_extended_fail:
if ((flags & MALLOC)
&& *strptr + strsize - str <= MB_LEN_MAX)
{
- /* Enlarge the buffer. */
+ /* Enlarge the buffer. And we may not
+ expand str by width as the wcrtomb may
+ return various bytes */
size_t strleng = str - *strptr;
char *newstr;
@@ -3052,7 +3070,7 @@ digits_extended_fail:
&& (char *) str == *strptr + strsize)
{
/* Enlarge the buffer. */
- size_t newsize = 2 * strsize;
+ size_t newsize = grow_to_fit (strsize, width, 1);
allocagain:
str = (char *) realloc (*strptr, newsize);