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

[Nadav Tasher]

I actually prefer to duplicate argv[] before calling run_noexec_applet_and_exit(),
because changing the signature of applets implies that future applets would need
more work done to be ported to busybox, as their main() function would need to
be adapted.

This copy will only happen before using run_noexec_applet_and_exit().
Every other flow will not duplicate the argv[].

On Tue, Jan 28, 2025 at 06:03:40PM +1000, d+busybox at adaptive-enterprises.com wrote:
> 
> 
> On Tue, 28 Jan 2025, Kang-Che Sung wrote:
> > > > 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.)
> 
> Ah, you're right. I was focused on the fact that the consumer of the
> char** argv was
> 
> 	int execvp(const char *file, char *const argv[]);
> 
> where I hoped, as a syscall, it would copy-out the argv[] in a read-only
> way which makes the cast safe.
> 
> But, as you point out, in this patchset run_applet_no_and_exit() will
> be passing a (casted) rodata argv straight to an applet's main().
> 
> To avoid unnecessary heap allocations and copying on tiny targets,
> (by str*dup'ing argv[] every time) maybe we can add const as another
> restriction required by NOEXEC. I mean we might find we're lucky that most
> of the ~167 NOEXEC applets could already change their main signature from
> 
> 	(int argc, char **argv)
> 
> to
> 
> 	(int argc, const char *const *argv)
> 
> and for those that can't, ie those that write to argv[], we may
> find they can be easily fixed to use a local copy instead.