| Message ID | 20220201000807.2453486-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | selftests/exec: Avoid future NULL argv execve warning | expand |
On Mon, Jan 31, 2022 at 04:08:07PM -0800, Kees Cook wrote: > Build actual argv for launching recursion test to avoid future warning > about using an empty argv in execve(). > --- a/tools/testing/selftests/exec/recursion-depth.c > +++ b/tools/testing/selftests/exec/recursion-depth.c > @@ -24,8 +24,14 @@ > #include <sys/mount.h> > #include <unistd.h> > > +#define FILENAME "/tmp/1" > +#define HASHBANG "#!" FILENAME "\n" > + > int main(void) > { > + char * const argv[] = { FILENAME, NULL }; > + int rv; Can we move out of -Wdeclaration-after-statement mentality in tests at least? > - int rv = execve(FILENAME, NULL, NULL); > + rv = execve(FILENAME, argv, NULL); int rv = execve(FILENAME, (char*[]){FILENAME, NULL}, NULL); is cleaner (and modern)! > if (rv == -1 && errno == ELOOP) { > return 0; > }
On 2/2/22 8:13 AM, Alexey Dobriyan wrote: > On Mon, Jan 31, 2022 at 04:08:07PM -0800, Kees Cook wrote: >> Build actual argv for launching recursion test to avoid future warning >> about using an empty argv in execve(). > >> --- a/tools/testing/selftests/exec/recursion-depth.c >> +++ b/tools/testing/selftests/exec/recursion-depth.c >> @@ -24,8 +24,14 @@ >> #include <sys/mount.h> >> #include <unistd.h> >> >> +#define FILENAME "/tmp/1" >> +#define HASHBANG "#!" FILENAME "\n" >> + >> int main(void) >> { >> + char * const argv[] = { FILENAME, NULL }; >> + int rv; > > Can we move out of -Wdeclaration-after-statement mentality in tests at least? selftest like the rest of the kernel follows the same coding guidelines. It will follow the moving "-Wdeclaration-after-statement mentality" when the rest of the kernel does. Looks like this topic was discussed in the following: https://patchwork.kernel.org/project/linux-kbuild/patch/c6fda26e8d134264b04fadc3386d6c32@gmail.com/ thanks, -- Shuah
On Wed, Feb 02, 2022 at 10:38:57AM -0700, Shuah Khan wrote: > On 2/2/22 8:13 AM, Alexey Dobriyan wrote: > > On Mon, Jan 31, 2022 at 04:08:07PM -0800, Kees Cook wrote: > > > Build actual argv for launching recursion test to avoid future warning > > > about using an empty argv in execve(). > > > > > --- a/tools/testing/selftests/exec/recursion-depth.c > > > +++ b/tools/testing/selftests/exec/recursion-depth.c > > > @@ -24,8 +24,14 @@ > > > #include <sys/mount.h> > > > #include <unistd.h> > > > +#define FILENAME "/tmp/1" > > > +#define HASHBANG "#!" FILENAME "\n" > > > + > > > int main(void) > > > { > > > + char * const argv[] = { FILENAME, NULL }; > > > + int rv; > > > > Can we move out of -Wdeclaration-after-statement mentality in tests at least? > > selftest like the rest of the kernel follows the same coding guidelines. > It will follow the moving "-Wdeclaration-after-statement mentality" when > the rest of the kernel does. > > Looks like this topic was discussed in the following: > https://patchwork.kernel.org/project/linux-kbuild/patch/c6fda26e8d134264b04fadc3386d6c32@gmail.com/ The only real argument is "gcc miscompiles /proc" to which adding -Wdeclaration-after-statement looks like a too big hammer. Why can't we have nice things?
On 2/2/22 2:00 PM, Alexey Dobriyan wrote: > On Wed, Feb 02, 2022 at 10:38:57AM -0700, Shuah Khan wrote: >> On 2/2/22 8:13 AM, Alexey Dobriyan wrote: >>> On Mon, Jan 31, 2022 at 04:08:07PM -0800, Kees Cook wrote: >>>> Build actual argv for launching recursion test to avoid future warning >>>> about using an empty argv in execve(). >>> >>>> --- a/tools/testing/selftests/exec/recursion-depth.c >>>> +++ b/tools/testing/selftests/exec/recursion-depth.c >>>> @@ -24,8 +24,14 @@ >>>> #include <sys/mount.h> >>>> #include <unistd.h> >>>> +#define FILENAME "/tmp/1" >>>> +#define HASHBANG "#!" FILENAME "\n" >>>> + >>>> int main(void) >>>> { >>>> + char * const argv[] = { FILENAME, NULL }; >>>> + int rv; >>> >>> Can we move out of -Wdeclaration-after-statement mentality in tests at least? >> >> selftest like the rest of the kernel follows the same coding guidelines. >> It will follow the moving "-Wdeclaration-after-statement mentality" when >> the rest of the kernel does. >> >> Looks like this topic was discussed in the following: >> https://patchwork.kernel.org/project/linux-kbuild/patch/c6fda26e8d134264b04fadc3386d6c32@gmail.com/ > > The only real argument is "gcc miscompiles /proc" to which adding -Wdeclaration-after-statement > looks like a too big hammer. > Either way - selftest will stay in sync with the kernel coding standards for good reasons. Doing its own thing confuses developers and makes it hard for maintainers. thanks, -- Shuah
diff --git a/tools/testing/selftests/exec/recursion-depth.c b/tools/testing/selftests/exec/recursion-depth.c index 2dbd5bc45b3e..35348db00c52 100644 --- a/tools/testing/selftests/exec/recursion-depth.c +++ b/tools/testing/selftests/exec/recursion-depth.c @@ -24,8 +24,14 @@ #include <sys/mount.h> #include <unistd.h> +#define FILENAME "/tmp/1" +#define HASHBANG "#!" FILENAME "\n" + int main(void) { + char * const argv[] = { FILENAME, NULL }; + int rv; + if (unshare(CLONE_NEWNS) == -1) { if (errno == ENOSYS || errno == EPERM) { fprintf(stderr, "error: unshare, errno %d\n", errno); @@ -44,21 +50,19 @@ int main(void) return 1; } -#define FILENAME "/tmp/1" int fd = creat(FILENAME, 0700); if (fd == -1) { fprintf(stderr, "error: creat, errno %d\n", errno); return 1; } -#define S "#!" FILENAME "\n" - if (write(fd, S, strlen(S)) != strlen(S)) { + if (write(fd, HASHBANG, strlen(HASHBANG)) != strlen(HASHBANG)) { fprintf(stderr, "error: write, errno %d\n", errno); return 1; } close(fd); - int rv = execve(FILENAME, NULL, NULL); + rv = execve(FILENAME, argv, NULL); if (rv == -1 && errno == ELOOP) { return 0; }
Build actual argv for launching recursion test to avoid future warning about using an empty argv in execve(). Cc: Eric Biederman <ebiederm@xmission.com> Cc: Alexey Dobriyan <adobriyan@gmail.com> Cc: Shuah Khan <shuah@kernel.org> Cc: linux-kselftest@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- tools/testing/selftests/exec/recursion-depth.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)