| Message ID | 90179484b62c0bafb0fad9b03680136bd6fedee3.1687957589.git.falcon@tinylab.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tools/nolibc: add a new syscall helper | expand |
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
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 80ab29e2887c..b178bfa29ad9 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -592,6 +592,59 @@ static int test_stat_timestamps(void) return 0; } +int test_mmap_munmap(void) +{ + int ret, fd, i; + void *mem; + size_t page_size, file_size, length; + off_t offset, pa_offset; + struct stat stat_buf; + static const char * const files[] = { + "/proc/1/exe", "/proc/self/exe", + "/init", "/sbin/init", "/etc/init", "/bin/init", "/bin/sh", + NULL + }; + + page_size = getpagesize(); + if (page_size < 0) + return -1; + + /* find a right file to mmap, existed and accessible */ + for (i = 0; files[i] != NULL; i++) { + ret = fd = open(files[i], O_RDONLY); + if (ret == -1) + continue; + else + break; + } + if (ret == -1) + return ret; + + ret = stat(files[i], &stat_buf); + if (ret == -1) + goto end; + + file_size = stat_buf.st_size; + offset = file_size - 1; + if (offset < 0) + offset = 0; + length = file_size - offset; + pa_offset = offset & ~(page_size - 1); + + mem = mmap(NULL, length + offset - pa_offset, PROT_READ, MAP_SHARED, fd, pa_offset); + if (mem == MAP_FAILED) { + ret = -1; + goto end; + } + + ret = munmap(mem, length + offset - pa_offset); + +end: + close(fd); + return ret; +} + + /* Run syscall tests between IDs <min> and <max>. * Return 0 on success, non-zero on failure. */ @@ -666,6 +719,9 @@ int run_syscall(int min, int max) CASE_TEST(lseek_m1); EXPECT_SYSER(1, lseek(-1, 0, SEEK_SET), -1, EBADF); break; CASE_TEST(lseek_0); EXPECT_SYSER(1, lseek(0, 0, SEEK_SET), -1, ESPIPE); break; CASE_TEST(mkdir_root); EXPECT_SYSER(1, mkdir("/", 0755), -1, EEXIST); break; + CASE_TEST(mmap_bad); EXPECT_PTRER(1, mmap(NULL, 0, PROT_READ, MAP_PRIVATE, 0, 0), MAP_FAILED, EINVAL); break; + CASE_TEST(munmap_bad); EXPECT_SYSER(1, munmap((void *)-1, 0), -1, EINVAL); break; + CASE_TEST(mmap_munmap_good); EXPECT_SYSZR(1, test_mmap_munmap()); break; CASE_TEST(open_tty); EXPECT_SYSNE(1, tmp = open("/dev/null", 0), -1); if (tmp != -1) close(tmp); break; CASE_TEST(open_blah); EXPECT_SYSER(1, tmp = open("/proc/self/blah", 0), -1, ENOENT); if (tmp != -1) close(tmp); break; CASE_TEST(poll_null); EXPECT_SYSZR(1, poll(NULL, 0, 0)); break;
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(+)