Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 3 Jul 2023 10:20:41 +0200
From:   Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <thomas@t-8ch.de>
To:     Zhangjin Wu <falcon@tinylab.org>
Cc:     w@1wt.eu, arnd@arndb.de, david.laight@aculab.com,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-riscv@lists.infradead.org
Subject: Re: [PATCH v5 14/14] selftests/nolibc: add mmap and munmap test cases
Message-ID: <f74a85a5-ea9a-4819-a95e-aa9525bf84f9@t-8ch.de>
References: <ZKJ35DyPOG+LAy5j@1wt.eu>
 <20230703080647.491363-1-falcon@tinylab.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230703080647.491363-1-falcon@tinylab.org>
Precedence: bulk

On 2023-07-03 16:06:47+0800, Zhangjin Wu wrote:
> Hi, Willy

> [..]

> > > - argv[0]
> > > 
> > >   since nolibc has no realpath() currently, we can simply
> > >   support the current path and the absolute path like this:
> > > 
> > >     nolibc-test.c:
> > > 
> > >     /* assigned as argv[0] in main(), will be used by some tests */
> > >     static char exe[PATH_MAX + 1];
> > > 
> > >     main():
> > > 
> > >     /* get absolute path of myself, nolibc has no realpath() currently */
> > >     #ifndef NOLIBC
> > >             realpath(argv[0], exe);
> > >     #else
> > >             /* assume absolute path has no "./" */
> > >             if (strncmp(argv[0], "./", 2) != 0)
> > >                     strncat(exe, argv[0], strlen(argv[0]) + 1);
> > >             else {
> > >                     pwd = getenv("PWD");
> > >                     /* skip the ending '\0' */
> > >                     strncat(exe, getenv("PWD"), strlen(pwd));
> > >                     /* skip the first '.' */
> > >                     strncat(exe, argv[0] + 1, strlen(argv[0]));
> > >             }
> > >     #endif
> > 
> > No, please, not like this. Just copy argv[0] (the pointer not the
> > contents) and you're fine:
> >
> >     static const char *argv0;
> > 
> >     int main(int argc, char **argv, char **envp)
> >     {
> >             argv0 = argv[0];
> >             ...
> >     }
> > 
> > Nothing more, nothing less. Your program will always have its correct
> > path when being called unless someone purposely forces it to something
> > different, which is not our concern at all since this is a test program.
> > And I'd rather call it "argv0" which exactly tells us what it contains
> > than "exe" which can be misleading for that precise reason.
> >
> 
> Yeah, locally, I just used a global argv0 pointer directly, but
> chroot_exe("./nolibc-test") not work when run 'libc-test' in host
> system, that is why I tried to get an absolute path ;-)
> 
>     CASE_TEST(chroot_exe);        EXPECT_SYSER(1, chroot(exe), -1, ENOTDIR); break;
> 
>     -->
> 
>     19 chroot_exe = -1 ENOENT  != (-1 ENOTDIR)                      [FAIL]
> 
> I removed the "proc ?" check manually to test if it also work with
> CONFIG_PROC_FS=n. it doesn't work, without absolute path, we need to add
> the ENOENT errno back to the errno check list.
> 
> I'm not sure if the other syscalls require an absolute path, so, the
> realpath() is called in this proposed method.
> 
> > > A full functional realpath() is a little complex, such as '../' support and
> > > even symlink support, let's delay its requirement at current stage ;-)
> > 
> > Please do not even engage into this, and keep in mind that the sole
> > purpose of this test program is to help developers simply add tests to
> > the set of existing ones. If the program becomes complex for doing stuff
> > that is out of its scope, it will become much harder to extend and users
> > will lose interest and motivation for updating it.
> > 
> > > one or both of them may also help the other test cases:
> > > 
> > > - chroot_exe (used '/init' before)
> > > 
> > >     CASE_TEST(chroot_exe);        EXPECT_SYSER(1, chroot(proc ? "/proc/self/exe" : exe), -1, ENOTDIR); break;
> > > 
> > > - chmod_exe (replace the one: chmod_tmpdir in another patchset)
> > > 
> > >     CASE_TEST(chmod_exe);       EXPECT_SYSZR(1, chmod(proc ? "/proc/self/exe" : exe, 0555)); break;
> > > 
> > >     It should be safe enough to only remove the writable attribute for the test
> > >     program.
> > > 
> > > - stat_timestamps (used '/init' before)
> > > 
> > >     if (stat("/proc/self/", &st) && stat(exe, &st) && stat("/dev/zero", &st) && stat("/", &st))
> > 
> > Indeed, why not!
> >
> 
> Ok, without absolute path, the chroot_exe() will be changed back to
> something like this:
> 
>     CASE_TEST(chroot_exe);        EXPECT_SYSER2(1, chroot(proc ? "/proc/self/exe" : argv0), -1, ENOTDIR, ENOENT); break;

Are you sure the ENOENT is really correct?
I played with this before and got ENOENT because before the chroot test
we have a testcase that does chdir("/").
And therefore the relative name in argv[0] was not resolving correctly
anymore against the changed working directory.

(You can also test this by executing *only* the chroot test and it
should work)

In general chroot() should work just fine with relative paths.

This is really a lot of complexity and discussion only to avoid
depending on procfs for the tests.

Thomas