netdev - Re: [PATCH bpf-next v1 7/8] selftests/bpf: Fix using stdout, stderr as struct field names

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZqR2DuHdBXPX/yx8@kodidev-ubuntu>
Date: Fri, 26 Jul 2024 21:22:38 -0700
From: Tony Ambardar <tony.ambardar@...il.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: bpf@...r.kernel.org, linux-kselftest@...r.kernel.org,
	netdev@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Andrii Nakryiko <andrii@...nel.org>,
	Martin KaFai Lau <martin.lau@...ux.dev>,
	Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>,
	Yonghong Song <yonghong.song@...ux.dev>,
	John Fastabend <john.fastabend@...il.com>,
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>,
	Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
	Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>,
	Björn Töpel <bjorn@...nel.org>,
	Magnus Karlsson <magnus.karlsson@...el.com>,
	Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
	Jonathan Lemon <jonathan.lemon@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Yan Zhai <yan@...udflare.com>
Subject: Re: [PATCH bpf-next v1 7/8] selftests/bpf: Fix using stdout, stderr
 as struct field names

On Thu, Jul 25, 2024 at 01:27:03PM -0700, Andrii Nakryiko wrote:
> On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@...il.com> wrote:
> >
> > From: Tony Ambardar <tony.ambardar@...il.com>
> >
> > Typically stdin, stdout, stderr are treated as reserved identifiers under
> > ISO/ANSI C, and a libc implementation is free to define these as macros.
> 
> Ok, wow that. Do you have a pointer to where in the standard it is
> said that stdin/stdout/stderr is some sort of reserved identifier that
> can't be used as a field name?
> 

I'll need to dig around to share some references. The short answer IIRC
is there's enough potential variation in their definitions that their
use requires care (or better avoidance).

> 
> I really don't like these underscored field names. If we have to
> rename, I'd prefer something like env.saved_stdout instead of
> env._stdout. But I'd prefer even more if musl wasn't doing this macro
> definition, of course...

OK, I'll use clearer names for a v2.

I believe the macro definitions are quite common and old, but "how"
makes a difference: specifically, using parenthesis happens to break our
.stdxxx field names.


In glibc <stdio.h> we have for example:
...
/* Standard streams.  */
extern FILE *stdin;             /* Standard input stream.  */
extern FILE *stdout;            /* Standard output stream.  */
extern FILE *stderr;            /* Standard error output stream.  */
/* C89/C99 say they're macros.  Make them happy.  */
#define stdin stdin
#define stdout stdout
#define stderr stderr
...

while in musl <stdio.h> we have:
...
extern FILE *const stdin;
extern FILE *const stdout;
extern FILE *const stderr;

#define stdin  (stdin)
#define stdout (stdout)
#define stderr (stderr)
...

which borks code in test_progs.c:
...
env.stderr = stderr;
env.stdout = stdout;
...


> 
> > This is the case in musl libc and results in compile errors when these
> > names are reused as struct fields, as with 'struct test_env' and related
> > usage in test_progs.[ch] and reg_bounds.c.
> >
> > Rename the fields to _stdout and _stderr to avoid many errors seen building
> > against musl, e.g.:
> >
> >   In file included from test_progs.h:6,
> >                    from test_progs.c:5:
> >   test_progs.c: In function 'print_test_result':
> >   test_progs.c:237:21: error: expected identifier before '(' token
> >     237 |         fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
> >         |                     ^~~~~~
> >   test_progs.c:237:9: error: too few arguments to function 'fprintf'
> >     237 |         fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
> >         |         ^~~~~~~
> >
> > Signed-off-by: Tony Ambardar <tony.ambardar@...il.com>
> > ---
> >  .../selftests/bpf/prog_tests/reg_bounds.c     |  2 +-
> >  tools/testing/selftests/bpf/test_progs.c      | 66 +++++++++----------
> >  tools/testing/selftests/bpf/test_progs.h      |  8 +--
> >  3 files changed, 38 insertions(+), 38 deletions(-)
> >
> 
> [...]