linux-kernel - Re: [PATCH 24/32] tools/nolibc: add getopt()

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: <20250305081713-94bcf231-26a2-40fd-b54d-e0cc0251e521@linutronix.de>
Date: Wed, 5 Mar 2025 08:25:14 +0100
From: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
To: Willy Tarreau <w@....eu>
Cc: Shuah Khan <shuah@...nel.org>, Shuah Khan <skhan@...uxfoundation.org>, 
	Thomas Weißschuh <linux@...ssschuh.net>, linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 24/32] tools/nolibc: add getopt()

On Tue, Mar 04, 2025 at 08:54:29AM +0100, Willy Tarreau wrote:
> On Tue, Mar 04, 2025 at 08:10:54AM +0100, Thomas Weißschuh wrote:
> > diff --git a/tools/include/nolibc/getopt.h b/tools/include/nolibc/getopt.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..35aee582681b79e21bce8ddbf634ae9dfdef8f1d
> > --- /dev/null
> > +++ b/tools/include/nolibc/getopt.h
> > @@ -0,0 +1,105 @@
> > +/* SPDX-License-Identifier: LGPL-2.1 OR MIT */
> > +/*
> > + * getopt function definitions for NOLIBC, adapted from musl libc
> > + * Copyright (C) 2005-2020 Rich Felker, et al.
> > + * Copyright (C) 2025 Thomas Weißschuh <linux@...ssschuh.net>
> > + */
> > +
> > +#ifndef _NOLIBC_GETOPT_H
> > +#define _NOLIBC_GETOPT_H
> > +
> > +struct FILE;
> > +static struct FILE *const stderr;
> > +static int fprintf(struct FILE *stream, const char *fmt, ...);
> 
> Is there a particular reason why you had to define these here
> and include nolibc.h at the bottom instead of doing it the usual
> way with the include at the top ?
> 
> If that's due to a limitation in nolibc, we might want to have a
> closer look at it before it starts to affect other areas. Also if
> in the future we have to add some str* dependencies here, it would
> be easier if we can simply include the file as well.

Doing a regular #include "stdio.h" does fail with the following error:

In file included from sysroot/i386/include/nolibc.h:109,
                 from sysroot/i386/include/errno.h:26,
                 from sysroot/i386/include/stdio.h:12,
                 from harness-selftest.c:3,
                 from nolibc-test.c:5:
sysroot/i386/include/getopt.h: In function 'getopt':
sysroot/i386/include/getopt.h:72:25: error: implicit declaration of function 'fprintf' [-Werror=implicit-function-declaration]
   72 |                         fprintf(stderr, "%s: unrecognized option: %c\n", argv[0], *optchar);
      |                         ^~~~~~~
[+ some followup errors]

The include chain is important here.
The user code includes "stdio.h", which at the very beginning includes
errno.h->nolibc.h->getopt.h. Now getopt.h tries to use the definitions from
stdio.h. However as stdio.h was the entrypoint and is not yet fully parsed,
these definitions are not yet available.

> > +__attribute__((weak,unused,section(".data.nolibc_getopt")))
> > +char *optarg;
> > +__attribute__((weak,unused,section(".data.nolibc_getopt")))
> > +int optind = 1;
> > +__attribute__((weak,unused,section(".data.nolibc_getopt")))
> > +int opterr = 1;
> > +__attribute__((weak,unused,section(".data.nolibc_getopt")))
> > +int optopt;
> > +__attribute__((weak,unused,section(".data.nolibc_getopt")))
> > +int __optpos;
> 
> I think that for better readability, you'd need to either place
> them on the same line, or leave a blank line between each
> declaration.

Ack.

> > +static __inline__
> > +int getopt(int argc, char * const argv[], const char *optstring)
> 
> It would be better marked with the usual unused attribute. That's a
> bit large for inlining, and I'm not convinced that the compiler will
> see any opportunity for simplifying it given that it acts on a list
> of actions taken from a string.

Ack.