So, in summary, I need one of the following: 1. Guarentee that anything BB_EXECVPE does is not going to affect argv[], since some of them are defined as string literals, and those can be in .rodata. 2. Change all of the argv initializations to result in char ** without casts.
I think it would be easier to implement (1), since making a copy of argv[] is quite easy, but I must agree that implementing (2) is the right way to go. What do you suggest I do? Should I use xstrdup to duplicate literals? Nadav On Wed, Jan 29, 2025 at 12:32:29AM +0200, Nadav Tasher wrote: > 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, [email protected] > 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. _______________________________________________ busybox mailing list [email protected] https://lists.busybox.net/mailman/listinfo/busybox
