[v5,14/14] selftests/nolibc: add mmap and munmap test cases

  Three mmap/munmap related test cases are added:

- mmap_bad: the length argument must be greater than 0, otherwise, fail
  with -EINVAL.

- munmap_bad: invalid (void *)-1 address fail with -EINVAL.

- mmap_munmap_good: mmap() a file with good offset and then munmap().

Note, it is not easy to find a unique file for mmap() in different
scenes, so, a file list is used to search the right one:

- /proc/1/exe, for 'run' and 'run-user' target
  'run-user' can not find '/proc/self/exe'

- /proc/self/exe, for 'libc-test' target
  normal program 'libc-test' has no permission to access '/proc/1/exe'

- the others, for kernel without procfs
  let it pass even with 'worst case' kernel configs

Suggested-by: Thomas Weißschuh <linux@weissschuh.net>
Link: https://lore.kernel.org/lkml/bff82ea6-610b-4471-a28b-6c76c28604a6@t-8ch.de/
Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/testing/selftests/nolibc/nolibc-test.c | 56 ++++++++++++++++++++
 1 file changed, 56 insertions(+)
  

Hi Zhangjin,

On Wed, Jun 28, 2023 at 09:51:57PM +0800, Zhangjin Wu wrote:
> Three mmap/munmap related test cases are added:
> 
> - mmap_bad: the length argument must be greater than 0, otherwise, fail
>   with -EINVAL.
> 
> - munmap_bad: invalid (void *)-1 address fail with -EINVAL.
> 
> - mmap_munmap_good: mmap() a file with good offset and then munmap().
> 
> Note, it is not easy to find a unique file for mmap() in different
> scenes, so, a file list is used to search the right one:
> 
> - /proc/1/exe, for 'run' and 'run-user' target
>   'run-user' can not find '/proc/self/exe'
> 
> - /proc/self/exe, for 'libc-test' target
>   normal program 'libc-test' has no permission to access '/proc/1/exe'

Strictly speaking, if your executable is not readable (e.g. chmod 111
due to a restrictive umask) it will also fail that one.

> - the others, for kernel without procfs
>   let it pass even with 'worst case' kernel configs

You should include /dev/zero, which is commonly used to allocate anonymous
memory and is more likely present and readable than any of the other files.
And another file of choice is obviously argv[0] ;-)  In this case you don't
need any of the other extra ones. Thus I could suggest that you try in this
order:

    /dev/zero, /proc/self/exe, /proc/1/exe, argv[0]

and be done with it. That doesn't prevent one from extending the list if
really needed later, but I doubt it would be needed. Also, it's already
arranged in a read-write, then read-only fallbacks mode, so if we later
need to add more complex tests involving writes, the writable /dev/zero
will have precedence.

Willy
  

> Hi Zhangjin,
> 
> On Wed, Jun 28, 2023 at 09:51:57PM +0800, Zhangjin Wu wrote:
> > Three mmap/munmap related test cases are added:
> > 
> > - mmap_bad: the length argument must be greater than 0, otherwise, fail
> >   with -EINVAL.
> > 
> > - munmap_bad: invalid (void *)-1 address fail with -EINVAL.
> > 
> > - mmap_munmap_good: mmap() a file with good offset and then munmap().
> > 
> > Note, it is not easy to find a unique file for mmap() in different
> > scenes, so, a file list is used to search the right one:
> > 
> > - /proc/1/exe, for 'run' and 'run-user' target
> >   'run-user' can not find '/proc/self/exe'
> > 
> > - /proc/self/exe, for 'libc-test' target
> >   normal program 'libc-test' has no permission to access '/proc/1/exe'
> 
> Strictly speaking, if your executable is not readable (e.g. chmod 111
> due to a restrictive umask) it will also fail that one.
>

ok.

> > - the others, for kernel without procfs
> >   let it pass even with 'worst case' kernel configs
> 
> You should include /dev/zero, which is commonly used to allocate anonymous
> memory and is more likely present and readable than any of the other files.
> And another file of choice is obviously argv[0] ;-)  In this case you don't
> need any of the other extra ones. Thus I could suggest that you try in this
> order:
> 
>     /dev/zero, /proc/self/exe, /proc/1/exe, argv[0]
> 
> and be done with it. That doesn't prevent one from extending the list if
> really needed later, but I doubt it would be needed. Also, it's already
> arranged in a read-write, then read-only fallbacks mode, so if we later
> need to add more complex tests involving writes, the writable /dev/zero
> will have precedence.
>

Cool, both /dev/zero and argv[0] are very good candidates ;-)

Just verified both of them, works perfectly.

- /dev/zero

  we need to mknod it in prepare() and also, in test_mmap_munmap(),
  stat() return a zero size of /dev/zero, in this case, we should assign
  a non-zero file_size ourselves.

    -       file_size = stat_buf.st_size;
    +       /* file size of the special /dev/zero is 0, let's assign one manually */
    +       if (i == 0)
    +               file_size = 3*page_size - 1;
    +       else
    +               file_size = stat_buf.st_size;


- 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

A full functional realpath() is a little complex, such as '../' support and
even symlink support, let's delay its requirement at current stage ;-)

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))

Will update the related patches with them.

Thanks,
Zhangjin

> Willy
  

On Mon, Jul 03, 2023 at 02:03:23PM +0800, Zhangjin Wu wrote:
> > > - the others, for kernel without procfs
> > >   let it pass even with 'worst case' kernel configs
> > 
> > You should include /dev/zero, which is commonly used to allocate anonymous
> > memory and is more likely present and readable than any of the other files.
> > And another file of choice is obviously argv[0] ;-)  In this case you don't
> > need any of the other extra ones. Thus I could suggest that you try in this
> > order:
> > 
> >     /dev/zero, /proc/self/exe, /proc/1/exe, argv[0]
> > 
> > and be done with it. That doesn't prevent one from extending the list if
> > really needed later, but I doubt it would be needed. Also, it's already
> > arranged in a read-write, then read-only fallbacks mode, so if we later
> > need to add more complex tests involving writes, the writable /dev/zero
> > will have precedence.
> >
> 
> Cool, both /dev/zero and argv[0] are very good candidates ;-)
> 
> Just verified both of them, works perfectly.
> 
> - /dev/zero
> 
>   we need to mknod it in prepare()

Indeed.

>   and also, in test_mmap_munmap(),
>   stat() return a zero size of /dev/zero, in this case, we should assign
>   a non-zero file_size ourselves.
> 
>     -       file_size = stat_buf.st_size;
>     +       /* file size of the special /dev/zero is 0, let's assign one manually */
>     +       if (i == 0)
>     +               file_size = 3*page_size - 1;
>     +       else
>     +               file_size = stat_buf.st_size;

OK but why this -1 ? That doesn't sound right, unless you explicitly want
a file size that's not multiple of a page size for whatever reason ?

> - 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.

> 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!

> Will update the related patches with them.

OK thanks!
Willy
  

Hi, Willy

> On Mon, Jul 03, 2023 at 02:03:23PM +0800, Zhangjin Wu wrote:
> > > > - the others, for kernel without procfs
> > > >   let it pass even with 'worst case' kernel configs
> > > 
> > > You should include /dev/zero, which is commonly used to allocate anonymous
> > > memory and is more likely present and readable than any of the other files.
> > > And another file of choice is obviously argv[0] ;-)  In this case you don't
> > > need any of the other extra ones. Thus I could suggest that you try in this
> > > order:
> > > 
> > >     /dev/zero, /proc/self/exe, /proc/1/exe, argv[0]
> > > 
> > > and be done with it. That doesn't prevent one from extending the list if
> > > really needed later, but I doubt it would be needed. Also, it's already
> > > arranged in a read-write, then read-only fallbacks mode, so if we later
> > > need to add more complex tests involving writes, the writable /dev/zero
> > > will have precedence.
> > >
> > 
> > Cool, both /dev/zero and argv[0] are very good candidates ;-)
> > 
> > Just verified both of them, works perfectly.
> > 
> > - /dev/zero
> > 
> >   we need to mknod it in prepare()
> 
> Indeed.
> 
> >   and also, in test_mmap_munmap(),
> >   stat() return a zero size of /dev/zero, in this case, we should assign
> >   a non-zero file_size ourselves.
> > 
> >     -       file_size = stat_buf.st_size;
> >     +       /* file size of the special /dev/zero is 0, let's assign one manually */
> >     +       if (i == 0)
> >     +               file_size = 3*page_size - 1;
> >     +       else
> >     +               file_size = stat_buf.st_size;
> 
> OK but why this -1 ? That doesn't sound right, unless you explicitly want
> a file size that's not multiple of a page size for whatever reason ?
>

Just make sure the file size is a litle random, not just aligned with
PAGE_SIZE, it is ok without '-1' ;-)

> > - 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;

Best regards,
Zhangjin

> > Will update the related patches with them.
> 
> OK thanks!
> Willy
  

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
  

Hi, Thomas

> 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("/").

Yes, there are some chdir tests before chroot, it does answer why
relative path not work and return ENOENT: no such file in the relative
path changed by chdir(), it differs from the one in PWD environment
variable.

> 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)
>

Yeah, If chdir() back to current path, it does work:

    CASE_TEST(chroot_exe);        chdir(getenv("PWD")); EXPECT_SYSER(1, chroot(exe), -1, ENOTDIR); break;

    -->

    11 chdir_root = 0                                                [OK]
    12 chdir_dot = 0                                                 [OK]
    13 chdir_blah = -1 ENOENT                                        [OK]
    14 chmod_self = -1 EPERM                                         [OK]
    15 chmod_exe = 0                                                 [OK]
    16 chown_self = -1 EPERM                                         [OK]
    17 chroot_root                                                  [SKIPPED]
    18 chroot_blah = -1 ENOENT                                       [OK]
    19 chroot_exe
    pwd: /home/ubuntu/Develop/src/examples/musl
    exe: ./nolibc-test
     = -1 ENOTDIR                                       [OK]


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

it does work with relative path, to make sure argv0 always work as
expected, an extra 'chdir()' may really required, or let's clean up the
previous chdir() test cases to call chdir(getenv("PWD")) after every
test.

    CASE_TEST(chdir_root);        EXPECT_SYSZR(1, chdir("/")); break;
    CASE_TEST(chdir_dot);         EXPECT_SYSZR(1, chdir(".")); break;
    CASE_TEST(chdir_blah);        EXPECT_SYSER(1, chdir("/blah"), -1, ENOENT); break;

perhaps only call 'chdir(getenv("PWD"))' after chdir_root() is enough,
because the chdir(".") doesn't really change the directory:

    CASE_TEST(chdir_root);        EXPECT_SYSZR(1, chdir("/")); chdir(getenv("PWD")); break;
    CASE_TEST(chdir_dot);         EXPECT_SYSZR(1, chdir(".")); break;
    CASE_TEST(chdir_blah);        EXPECT_SYSER(1, chdir("/blah"), -1, ENOENT); break;

Which one do you prefer?

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

Yes, one step further to find more interesting info, thanks a lot ;-)

Mixing chdir and relative path really need to be more careful.

Best regards,
Zhangjin

> Thomas
  

On Mon, Jul 03, 2023 at 04:06:47PM +0800, Zhangjin Wu wrote:
> > >     /* 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]

Then we have a problem somewhere else and the test should be debugger
instead. Are you sure there isn't a successful chdir() test before it
for example, that would change the directory ? If so maybe we just need
to save the current dir before calling it and restore it later.

> 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.

Same as above.

> I'm not sure if the other syscalls require an absolute path, so, the
> realpath() is called in this proposed method.

No, please do not overengineer tests. That's only hiding the dust under
the carpet and people adding more tests later that will randomly fail
will have a very hard time trying to figure what's happening under the
hood. If a test doesn't work as expected, we must not try to work around
it, but arrange to fix it.

Thanks,
Willy
  

> On Mon, Jul 03, 2023 at 04:06:47PM +0800, Zhangjin Wu wrote:
> > > >     /* 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]
> 
> Then we have a problem somewhere else and the test should be debugger
> instead. Are you sure there isn't a successful chdir() test before it
> for example, that would change the directory ? If so maybe we just need
> to save the current dir before calling it and restore it later.
>

Yes, as Thomas pointed out, the chdir test cases changed current
directory to "/" just before chroot_exe(), so, restore it with
chdir(getenv("PWD")) solves the issue.

> > 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.
> 
> Same as above.
> 
> > I'm not sure if the other syscalls require an absolute path, so, the
> > realpath() is called in this proposed method.
> 
> No, please do not overengineer tests. That's only hiding the dust under
> the carpet and people adding more tests later that will randomly fail
> will have a very hard time trying to figure what's happening under the
> hood. If a test doesn't work as expected, we must not try to work around
> it, but arrange to fix it.

That's right, thanks.

Best regards,
Zhangjin

> 
> Thanks,
> Willy