[PATCH v3 04/27] adduser: replace BB_EXECLP call with BB_EXECVP

[Nadav Tasher]

Hi!

I want to keep the signature of BB_EXECVPE similar to that of execvpe.
I can make sure no other function modifies argv by making a copy of argv,
which I will then pass to functions that expect char** (most notably 
run_noexec_applet_and_exit).

Regarding all of the places where I modified calls to BB_EXECVP using
newly created argvs, what should I change them to?
I think they should be char**, and be cast to const char **.
This seems safe to me.

Nadav

On Tue, Jan 28, 2025 at 02:13:08PM +0800, Kang-Che Sung wrote:
> On Mon, Jan 27, 2025 at 4:13 PM <d+busybox at adaptive-enterprises.com> wrote:
> >
> > On Mon, 27 Jan 2025, Kang-Che Sung wrote:
> > > On Mon, Jan 27, 2025 at 12:18 PM <d+busybox at adaptive-enterprises.com> wrote:
> > >>> +     passwd_argv[0] = (char *) "passwd";
> > >>> +     passwd_argv[1] = (char *) "--";
> > >>> +     passwd_argv[2] = (char *) login_name;
> > >>> +     passwd_argv[3] = NULL;
> > >>
> > >> The type of a string literal is already char *, so the first two don't need
> > >> the cast. You could even write it more compactly, like this:
> > >>
> > >>         char *passwd_argv[] = { "passwd", "--", (char *)login_name, NULL };
> > >
> > > Technically, string literals are of type "const char *", not "char *".
> >
> > Perhaps in C++ that is true, but not C. Ref C23, s6.4.5 "String literals",
> >
> >      For character string literals, the array elements have type char, ...
> >
> > You can try a few compiler experiments if you like.
> 
> Perhaps I didn't explain clearly enough. There is a distinction
> between when a string literal is specified in an array declaration and
> when it's passed as a pointer:
> 
> char str[] = "passwd"; // The string has type "char[7]", and is
> mutable (i.e. not placed in ".rodata" section)
> char *ptr = "passwd"; // This string is placed in ".rodata" section,
> and should be referred by the type "const char *".
> 
> > > Perhaps what you really intended is this?
> > >
> > > const char *passwd_argv[] = { "passwd", "--", login_name, NULL };
> >
> > That would be much more intuitive, readable, safer and so on, but then
> > you'll need a cast when pass it to the pre-const functions,
> > eg (char **)passwd_argv.
> 
> From the surrounding code I've read, you are attempting to pass a
> ".rodata" string into an "argv" character pointer array.
> 
> But "argv" is supposed to be mutable by the callee (i.e. the new
> program's main function). (I think it's required by the C standard.)
> 
> Unless there's a guarantee that the callee never modifies the "argv"
> string, passing ".rodata" strings with de-const casts would be unsafe.
> That's why when I initially saw the patch and code I was nervous. (A
> (char *) de-const cast without an in-code explanation of why is a red
> flag to me.)