[musl] Invalid read of nl_arg in printf_core()

[musl] Invalid read of nl_arg in printf_core()

[Markus Wichmann]

[-- Attachment #1: Type: text/plain, Size: 871 bytes --]

Hi all,

reading some code today, I noticed undefined behavior in printf_core().
vfprintf() creates an array called nl_arg automatically and does not
initialize it. That is fine, but it means that reads from each array
member are undefined behavior until that member gets assigned a value.

printf_core() gets the array passed in as argument, and will read it in
both passes. Unfortunately, it only assigns values to the array at the
end of the first pass. Therefore the reads from nl_arg in the first pass
are undefined.

I also noticed that the assignments to nl_type in the second pass, while
not undefined behavior, are just futile, since nl_type is only read
during the initialization of nl_arg, at the end of the first pass.
Therefore we can simply alternate the assignments depending on what pass
we are in. Please have a look at the attached patch.

Ciao,
Markus

[-- Attachment #2: 0001-Prevent-invalid-reads-of-nl_arg-in-printf_core.patch --]
[-- Type: text/x-diff, Size: 2374 bytes --]

From da0554ae8d415e5c6f9fbd9c256b8ad60f8e19d4 Mon Sep 17 00:00:00 2001
From: Markus Wichmann <nullplan@gmx.net>
Date: Thu, 3 Nov 2022 20:29:12 +0100
Subject: [PATCH] Prevent invalid reads of nl_arg in printf_core().

printf_core() runs twice, and during its first run, nl_arg is
uninitialized and must not be read. It gets initialized at the end of
the first run. Conversely, nl_type does not need to be set during the
second run, as its useful life has ended at that point, since the only
time it is read is during that exact same initialization. Therefore we
can simply alternate the assignments.

p and w do still need to get values assigned to them, since at least one
line in the same if-statement depends on that, but they can be dummy
values. arg does not need to be assigned, since in the first run, we
encounter a continue statement before using the argument.
---
 src/stdio/vfprintf.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/stdio/vfprintf.c b/src/stdio/vfprintf.c
index 9b961e7f..45557951 100644
--- a/src/stdio/vfprintf.c
+++ b/src/stdio/vfprintf.c
@@ -478,8 +478,8 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
 		if (*s=='*') {
 			if (isdigit(s[1]) && s[2]=='$') {
 				l10n=1;
-				nl_type[s[1]-'0'] = INT;
-				w = nl_arg[s[1]-'0'].i;
+				if (!f) nl_type[s[1]-'0'] = INT, w = 0;
+				else w = nl_arg[s[1]-'0'].i;
 				s+=3;
 			} else if (!l10n) {
 				w = f ? va_arg(*ap, int) : 0;
@@ -491,8 +491,8 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
 		/* Read precision */
 		if (*s=='.' && s[1]=='*') {
 			if (isdigit(s[2]) && s[3]=='$') {
-				nl_type[s[2]-'0'] = INT;
-				p = nl_arg[s[2]-'0'].i;
+				if (!f) nl_type[s[2]-'0'] = INT, p = 0;
+				else p = nl_arg[s[2]-'0'].i;
 				s+=4;
 			} else if (!l10n) {
 				p = f ? va_arg(*ap, int) : 0;
@@ -521,8 +521,10 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
 		if (st==NOARG) {
 			if (argpos>=0) goto inval;
 		} else {
-			if (argpos>=0) nl_type[argpos]=st, arg=nl_arg[argpos];
-			else if (f) pop_arg(&arg, st, ap);
+			if (argpos>=0) {
+				if (!f) nl_type[argpos]=st;
+				else arg=nl_arg[argpos];
+			} else if (f) pop_arg(&arg, st, ap);
 			else return 0;
 		}

--
2.17.1

Re: [musl] Invalid read of nl_arg in printf_core()

[Szabolcs Nagy]

* Markus Wichmann <nullplan@gmx.net> [2022-11-03 20:42:16 +0100]:
> Hi all,
> 
> reading some code today, I noticed undefined behavior in printf_core().
> vfprintf() creates an array called nl_arg automatically and does not
> initialize it. That is fine, but it means that reads from each array
> member are undefined behavior until that member gets assigned a value.

an automatic storage object that was not initialized has indeterminate
value, so accessing it is not undefined behaviour. (unless the object
could have been declared with the register storage class)

so technically the code is correct (if int has no trap representations)
but i think the fix still makes sense: leaving unspecified values around
is error prone.


> 
> printf_core() gets the array passed in as argument, and will read it in
> both passes. Unfortunately, it only assigns values to the array at the
> end of the first pass. Therefore the reads from nl_arg in the first pass
> are undefined.
> 
> I also noticed that the assignments to nl_type in the second pass, while
> not undefined behavior, are just futile, since nl_type is only read
> during the initialization of nl_arg, at the end of the first pass.
> Therefore we can simply alternate the assignments depending on what pass
> we are in. Please have a look at the attached patch.
> 
> Ciao,
> Markus

> From da0554ae8d415e5c6f9fbd9c256b8ad60f8e19d4 Mon Sep 17 00:00:00 2001
> From: Markus Wichmann <nullplan@gmx.net>
> Date: Thu, 3 Nov 2022 20:29:12 +0100
> Subject: [PATCH] Prevent invalid reads of nl_arg in printf_core().
> 
> printf_core() runs twice, and during its first run, nl_arg is
> uninitialized and must not be read. It gets initialized at the end of
> the first run. Conversely, nl_type does not need to be set during the
> second run, as its useful life has ended at that point, since the only
> time it is read is during that exact same initialization. Therefore we
> can simply alternate the assignments.
> 
> p and w do still need to get values assigned to them, since at least one
> line in the same if-statement depends on that, but they can be dummy
> values. arg does not need to be assigned, since in the first run, we
> encounter a continue statement before using the argument.
> ---
>  src/stdio/vfprintf.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/src/stdio/vfprintf.c b/src/stdio/vfprintf.c
> index 9b961e7f..45557951 100644
> --- a/src/stdio/vfprintf.c
> +++ b/src/stdio/vfprintf.c
> @@ -478,8 +478,8 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
>  		if (*s=='*') {
>  			if (isdigit(s[1]) && s[2]=='$') {
>  				l10n=1;
> -				nl_type[s[1]-'0'] = INT;
> -				w = nl_arg[s[1]-'0'].i;
> +				if (!f) nl_type[s[1]-'0'] = INT, w = 0;
> +				else w = nl_arg[s[1]-'0'].i;
>  				s+=3;
>  			} else if (!l10n) {
>  				w = f ? va_arg(*ap, int) : 0;
> @@ -491,8 +491,8 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
>  		/* Read precision */
>  		if (*s=='.' && s[1]=='*') {
>  			if (isdigit(s[2]) && s[3]=='$') {
> -				nl_type[s[2]-'0'] = INT;
> -				p = nl_arg[s[2]-'0'].i;
> +				if (!f) nl_type[s[2]-'0'] = INT, p = 0;
> +				else p = nl_arg[s[2]-'0'].i;
>  				s+=4;
>  			} else if (!l10n) {
>  				p = f ? va_arg(*ap, int) : 0;
> @@ -521,8 +521,10 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
>  		if (st==NOARG) {
>  			if (argpos>=0) goto inval;
>  		} else {
> -			if (argpos>=0) nl_type[argpos]=st, arg=nl_arg[argpos];
> -			else if (f) pop_arg(&arg, st, ap);
> +			if (argpos>=0) {
> +				if (!f) nl_type[argpos]=st;
> +				else arg=nl_arg[argpos];
> +			} else if (f) pop_arg(&arg, st, ap);
>  			else return 0;
>  		}
> 
> --
> 2.17.1
> 

Re: [musl] Invalid read of nl_arg in printf_core()

[Alex Xu (Hello71)]

Excerpts from Szabolcs Nagy's message of November 5, 2022 6:09 am:
> * Markus Wichmann <nullplan@gmx.net> [2022-11-03 20:42:16 +0100]:
>> Hi all,
>> 
>> reading some code today, I noticed undefined behavior in printf_core().
>> vfprintf() creates an array called nl_arg automatically and does not
>> initialize it. That is fine, but it means that reads from each array
>> member are undefined behavior until that member gets assigned a value.
> 
> an automatic storage object that was not initialized has indeterminate
> value, so accessing it is not undefined behaviour. (unless the object
> could have been declared with the register storage class)
> 
> so technically the code is correct (if int has no trap representations)
> but i think the fix still makes sense: leaving unspecified values around
> is error prone.

N1570 J.2 [Undefined behavior]:

1   The behavior is undefined in the following circumstances:
[ ... ]
— The value of an object with automatic storage duration is used while it is
  indeterminate (6.2.4, 6.7.9, 6.8).

Re: [musl] Invalid read of nl_arg in printf_core()

[Jₑₙₛ Gustedt]

[-- Attachment #1: Type: text/plain, Size: 2369 bytes --]

Alex,

on Sat, 05 Nov 2022 07:52:09 -0400 you ("Alex Xu (Hello71)"
<alex_y_xu@yahoo.ca>) wrote:

> Excerpts from Szabolcs Nagy's message of November 5, 2022 6:09 am:
> > * Markus Wichmann <nullplan@gmx.net> [2022-11-03 20:42:16 +0100]:  
> >> Hi all,
> >> 
> >> reading some code today, I noticed undefined behavior in
> >> printf_core(). vfprintf() creates an array called nl_arg
> >> automatically and does not initialize it. That is fine, but it
> >> means that reads from each array member are undefined behavior
> >> until that member gets assigned a value.  
> > 
> > an automatic storage object that was not initialized has
> > indeterminate value, so accessing it is not undefined behaviour.
> > (unless the object could have been declared with the register
> > storage class)
> > 
> > so technically the code is correct (if int has no trap
> > representations) but i think the fix still makes sense: leaving
> > unspecified values around is error prone.  
> 
> N1570 J.2 [Undefined behavior]:
> 
> 1   The behavior is undefined in the following circumstances:
> [ ... ]
> — The value of an object with automatic storage duration is used
> while it is indeterminate (6.2.4, 6.7.9, 6.8).

N1570 is old and annex J is only informative. The version for C23
reads

        The value of an object with automatic storage duration is used while
        the object has an indeterminate representation (6.2.4, 6.7.10, 6.8).

which is already a bit more subtle. Observe that it says "use" and
does not say that it is undefined to do the lvalue (object
representation -> value) conversion.

There have been multiple interpretations and endless discussions over
the years of this kind of question and no unique conclusion about its
"official" status w.r.t to the standard has been acknowledged. But the
inofficial status of this is that it is "bad" and reading
uninitialized values should be avoided.

This holds in particular where the possible gain by optimization
(speed or code size) is marginal, so the gain versus risk balance is
not very favorable.

Thanks
Jₑₙₛ

-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]