fcntl37: test posix lock across execve
PATCH 1/1 fcntl37: test posix lock across execve
--- testcases/kernel/syscalls/fcntl/Makefile | 3 + testcases/kernel/syscalls/fcntl/fcntl37.c | 146 ++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+) create mode 100644 testcases/kernel/syscalls/fcntl/fcntl37.c
testcases/kernel/syscalls/fcntl/Makefile +3 -0
@@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread fcntl36: LDLIBS += -lpthread fcntl36_64: LDLIBS += -lpthread +fcntl37: LDLIBS += -lpthread +fcntl37_64: LDLIBS += -lpthread + include $(top_srcdir)/include/mk/testcases.mk include $(abs_srcdir)/../utils/newer_64.mk
testcases/kernel/syscalls/fcntl/fcntl37.c new file +146 -0
146 lines not on this page · raw message
Signature · 1 line
1.8.3.1
PATCH 1/1 fcntl37: test posix lock across execve
Quoted 71 lines · Xiong Zhou
Signed-off-by Xiong Zhou <[email protected]>--- testcases/kernel/syscalls/fcntl/Makefile | 3 + testcases/kernel/syscalls/fcntl/fcntl37.c | 146 ++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+) create mode 100644 testcases/kernel/syscalls/fcntl/fcntl37.ctestcases/kernel/syscalls/fcntl/Makefile +3
-0@@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread fcntl36: LDLIBS += -lpthread fcntl36_64: LDLIBS += -lpthread +fcntl37: LDLIBS += -lpthread +fcntl37_64: LDLIBS += -lpthread + include $(top_srcdir)/include/mk/testcases.mk include $(abs_srcdir)/../utils/newer_64.mktestcases/kernel/syscalls/fcntl/fcntl37.c new file +44
-0@@ -0,0 +1,146 @@ +/* +* Copyright (c) 2018 RedHat Inc. All Rights Reserved. +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License as +* published by the Free Software Foundation; either version 2 of +* the License, or (at your option) any later version. +* +* This program is distributed in the hope that it would be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program. If not, see <http://www.gnu.org/licenses/>. +* +* Author: Xiong Zhou <[email protected]> +* +* This is testing for +* +* "Record locks are not inherited by a child created via fork(2), +* but are preserved across an execve(2)." +* +* from fcntl(2) man page. +* +*/ + +#include <sys/types.h> +#include <sys/stat.h> +#include <sys/wait.h> +#include <stdio.h> +#include <stdlib.h> +#include <errno.h> +#include <unistd.h> +#include <fcntl.h> +#include <pthread.h> +#include <sched.h> +#include <limits.h> + +#include "lapi/fcntl.h" +#include "tst_safe_pthread.h" +#include "tst_test.h" + +static const char fname[] ="tst_lock_execve" ;
testcases/kernel/syscalls/fcntl/fcntl37.c line 53
Quoted 10 lines
testcases/kernel/syscalls/fcntl/fcntl37.c new file +9
-0+static const char flag_fname[] ="/tmp/execved" ; +static void cleanup (void ); + +static void *thread_fn (void *arg) +{ +int fd = *(int *)arg; +tst_res (TINFO ,"Thread running. fd %d" , fd); +/* Just need to be alive when execve. */ +sleep (5 );
Since we don't expect this thread to complete running before execve() takes place, feels more robust to put put it into an infinite loop eg
while (1) sleep (1);
testcases/kernel/syscalls/fcntl/fcntl37.c
Quoted 1 line
testcases/kernel/syscalls/fcntl/fcntl37.c new file +1
-0+SAFE_CLOSE (fd);
and ditch this since it is not supposed to be reachable if the test is operating correctly.
Quoted 25 lines
testcases/kernel/syscalls/fcntl/fcntl37.c new file +25
-0+return NULL ; +} + +static void checklock (int fd) +{ +pid_t pid =SAFE_FORK (); +if (pid ==0 ) { +struct flock flck = { + .l_type =F_WRLCK , + .l_whence =SEEK_SET , + .l_start =0 , + .l_len =1 , + }; +SAFE_FCNTL (fd,F_GETLK , &flck); +if (flck.l_type ==F_UNLCK ) +tst_res (TFAIL ,"Record lock gets lost after execve" ); +else +tst_res (TPASS ,"Record lock survives execve" ); +SAFE_CLOSE (fd); +_exit (0 ); + } +waitpid (pid,NULL ,0 ); +} + +static void test01 (void )
testcases/kernel/syscalls/fcntl/fcntl37.c line 89
Quoted 10 lines
testcases/kernel/syscalls/fcntl/fcntl37.c new file +10
-0+{ +int fd, ret; +struct stat stat_buf; + +/* +* If tmp/tst_lock_execve exists, execve to run checklock. +*/ + ret =stat (flag_fname, &stat_buf); +if (ret ==0 ) { +checklock (fd);
Nothing seems to have initialized the 'fd' variable when it is used here.
testcases/kernel/syscalls/fcntl/fcntl37.c line 98
Quoted 9 lines
testcases/kernel/syscalls/fcntl/fcntl37.c new file +9
-0+cleanup (); +_exit (0 ); + } + +/* +* If tmp/tst_lock_execve does not exist, initialize it. +*/ +SAFE_OPEN (flag_fname,O_RDWR |O_CREAT ,0755 ); + fd =SAFE_OPEN (fname,O_RDWR |O_CREAT ,0755 );
What's the benefit in using 2 temporary files - feels like using one would be sufficient, particularly since this code is never calling unlink(fname) so leaking the 2nd file on disk
Quoted 51 lines
testcases/kernel/syscalls/fcntl/fcntl37.c new file +48
-0+struct flock64 flck = { + .l_type =F_WRLCK , + .l_whence =SEEK_SET , + .l_start =0 , + .l_len =1 , + }; +SAFE_FCNTL (fd,F_SETLK , &flck); + +/* +* Creat thread and keep it running after placing posix +* write lock. +*/ +pthread_t th; +SAFE_PTHREAD_CREATE (&th,NULL , thread_fn, (void *)&fd); +sleep (1 ); + +/* +* Clear CLOEXEC +*/ +int flags=SAFE_FCNTL (fd,F_GETFD ); + flags &= ~FD_CLOEXEC ; +SAFE_FCNTL (fd,F_SETFD , flags); + +/* +* Get full path name of running programm then execve. +*/ +char prog_name[PATH_MAX ]; + ret =readlink ("/proc/self/exe" , prog_name,PATH_MAX ); +char *const newargv[] = { prog_name,NULL ,NULL }; +tst_res (TINFO ,"execve %s with %s locked" , prog_name, fname); +execve (prog_name, newargv,NULL ); +/* +* Failure info for debug +*/ +perror ("execve " ); +} + +static void cleanup (void ) +{ +SAFE_UNLINK (flag_fname); +} + +static struct tst_test test = { + .needs_tmpdir =1 , + .forks_child =1 , + .test_all = test01, + .cleanup = cleanup, +};Signature · 1 line
1.8.3.1
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
PATCH 1/1 fcntl37: test posix lock across execve
In reply to Xiong ZhouHi!
testcases/kernel/syscalls/fcntl/Makefile fcntl34_64: LDLIBS += -lpthread · line 31
Quoted 10 lines
testcases/kernel/syscalls/fcntl/Makefile +2
-0@@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread fcntl36: LDLIBS += -lpthread fcntl36_64: LDLIBS += -lpthread +fcntl37: LDLIBS += -lpthread +fcntl37_64: LDLIBS += -lpthread
This is wrong, we should use CFLAGS += -pthread instead, also the rest of the Makefile should be fixed as well.
Quoted 2 lines
testcases/kernel/syscalls/fcntl/Makefile +0
-0include $(top_srcdir)/include/mk/testcases.mk include $(abs_srcdir)/../utils/newer_64.mk
...
testcases/kernel/syscalls/fcntl/Makefile
Quoted 6 lines
testcases/kernel/syscalls/fcntl/Makefile +6
-0+#include "lapi/fcntl.h" +#include "tst_safe_pthread.h" +#include "tst_test.h" + +static const char fname[] = "tst_lock_execve"; +static const char flag_fname[] = "/tmp/execved";
^
You must not create files in random places. Once you set the .needs_tmpdir in the test structure the test is executed with unique temporary directory as working directory so all you need to do for temporary files is to work with relative filenames.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 5 lines
testcases/kernel/syscalls/fcntl/Makefile +5
-0+static void cleanup(void); + +static void *thread_fn(void *arg) +{ + int fd = *(int *)arg;
^
This is broken, you have to use intptr_t
Quoted 14 lines
testcases/kernel/syscalls/fcntl/Makefile +14
-0+ tst_res(TINFO, "Thread running. fd %d", fd); + /* Just need to be alive when execve. */ + sleep(5); + SAFE_CLOSE(fd); + return NULL; +} + +static void checklock(int fd) +{ + pid_t pid = SAFE_FORK(); + if (pid == 0) { + struct flock flck = { + .l_type = F_WRLCK, + .l_whence = SEEK_SET,
testcases/kernel/syscalls/fcntl/Makefile
Quoted 10 lines
testcases/kernel/syscalls/fcntl/Makefile +10
-0+ .l_start = 0, + .l_len = 1, + }; + SAFE_FCNTL(fd, F_GETLK, &flck); + if (flck.l_type == F_UNLCK) + tst_res(TFAIL, "Record lock gets lost after execve"); + else + tst_res(TPASS, "Record lock survives execve"); + SAFE_CLOSE(fd); + _exit(0);
Why _exit() ?
testcases/kernel/syscalls/fcntl/Makefile
Quoted 2 lines
testcases/kernel/syscalls/fcntl/Makefile +2
-0+ } + waitpid(pid, NULL, 0);
SAFE_WAITPID()
Quoted 1 line
+}
Also having the check in a separate function and calling it several times makes it kind of hard for debugging. if one of the assertions fails the error messages would be the same. Maybe we should pass a string with describes assertion type to the function and use it int the tst_res() messages.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 12 lines
testcases/kernel/syscalls/fcntl/Makefile +12
-0+static void test01(void) +{ + int fd, ret; + struct stat stat_buf; + + /* + * If tmp/tst_lock_execve exists, execve to run checklock. + */ + ret = stat(flag_fname, &stat_buf); + if (ret == 0) { + checklock(fd); + cleanup();
You must not call the cleanup from the test. Cleanup function is called from the test library.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 1 line
testcases/kernel/syscalls/fcntl/Makefile +1
-0+ _exit(0);
Again why _exit() ?
testcases/kernel/syscalls/fcntl/Makefile
Quoted 1 line
testcases/kernel/syscalls/fcntl/Makefile +1
-0+ }
Don't do this.
You must not reexecute the testcase and put file flags into random places. If you need execve() a test helper, you have to write simple binary helper. See testcases/kernel/syscalls/creat/creat07_child.c
testcases/kernel/syscalls/fcntl/Makefile
Quoted 3 lines
testcases/kernel/syscalls/fcntl/Makefile +3
-0+ /* + * If tmp/tst_lock_execve does not exist, initialize it. + */
Please no useless comments like this one.
Quoted 4 lines
testcases/kernel/syscalls/fcntl/Makefile +4
-0+ SAFE_OPEN(flag_fname, O_RDWR|O_CREAT, 0755); + fd = SAFE_OPEN(fname, O_RDWR|O_CREAT, 0755); + struct flock64 flck = { + .l_type = F_WRLCK,
testcases/kernel/syscalls/fcntl/Makefile
Quoted 10 lines
testcases/kernel/syscalls/fcntl/Makefile +10
-0+ .l_whence = SEEK_SET, + .l_start = 0, + .l_len = 1, + }; + SAFE_FCNTL(fd, F_SETLK, &flck); + + /* + * Creat thread and keep it running after placing posix + * write lock. + */
Please no useless comments.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 2 lines
testcases/kernel/syscalls/fcntl/Makefile +2
-0+ pthread_t th; + SAFE_PTHREAD_CREATE(&th, NULL, thread_fn, (void *)&fd);
^
Here as well, we have to cast the fd to intptr_t first.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 1 line
testcases/kernel/syscalls/fcntl/Makefile +1
-0+ sleep(1);
Please no random sleeps() for thread synchronization, either use real thread synchronization primitives or LTP checkpoint synchronization library which is based on futexes.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 3 lines
testcases/kernel/syscalls/fcntl/Makefile +3
-0+ /* + * Clear CLOEXEC + */
Please no useless comments.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 7 lines
testcases/kernel/syscalls/fcntl/Makefile +7
-0+ int flags=SAFE_FCNTL(fd, F_GETFD); + flags &= ~FD_CLOEXEC; + SAFE_FCNTL(fd, F_SETFD, flags); + + /* + * Get full path name of running programm then execve. + */
Please no useless comments.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 3 lines
testcases/kernel/syscalls/fcntl/Makefile +3
-0+ char prog_name[PATH_MAX]; + ret = readlink("/proc/self/exe", prog_name, PATH_MAX); + char * const newargv[] = { prog_name, NULL, NULL };
This is far to complicated for no reason. The path to LTP test binaries is in $PATH so you can execute any LTP binary just by it's name. Absolute path is absolutely unneeded.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 5 lines
testcases/kernel/syscalls/fcntl/Makefile +5
-0+ tst_res(TINFO, "execve %s with %s locked", prog_name, fname); + execve(prog_name, newargv, NULL); + /* + * Failure info for debug + */
Please no useless comments.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 1 line
testcases/kernel/syscalls/fcntl/Makefile +1
-0+ perror("execve ");
No perror() please. Any error messages should be printed via tst_res() or tst_brk().
Quoted 19 lines
testcases/kernel/syscalls/fcntl/Makefile +13
-0+} + +static void cleanup(void) +{ + SAFE_UNLINK(flag_fname); +} + +static struct tst_test test = { + .needs_tmpdir = 1, + .forks_child = 1, + .test_all = test01, + .cleanup = cleanup, +};Signature · 5 lines
1.8.3.1 -- Mailing list info: https://lists.linux.it/listinfo/ltp
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 fcntl37: test posix lock across execve
Hi!
Quoted 6 lines
testcases/kernel/syscalls/fcntl/fcntl37.c line 112
Quoted 2 lines
testcases/kernel/syscalls/fcntl/fcntl37.c new file +2
-0+pthread_t th; +SAFE_PTHREAD_CREATE (&th,NULL , thread_fn, (void *)&fd);^
Here as well, we have to cast the fd tointptr_tfirst.
Scratch that, you are passing the fd by pointer not by value, which is questionable practice for a value on the function stack.
What is usually done when you need to pass integers to threads as paramters is to cast the integer to a type with the same width as a pointer then casting it to a void*, which is (void*)(intptr_t)fd in this case.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 fcntl37: test posix lock across execve
In reply to Cyril HrubisHi,
Thanks for reviewing!
Quoted 9 lines · Cyril Hrubis
Hi!
Quoted 8 lines
testcases/kernel/syscalls/fcntl/Makefile +0
-0@@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread fcntl36: LDLIBS += -lpthread fcntl36_64: LDLIBS += -lpthread
Quoted 4 lines
testcases/kernel/syscalls/fcntl/Makefile fcntl34_64: LDLIBS += -lpthread · line 31
Quoted 2 lines
testcases/kernel/syscalls/fcntl/Makefile +2
-0+fcntl37: LDLIBS += -lpthread +fcntl37_64: LDLIBS += -lpthreadThis is wrong, we should use CFLAGS += -pthread instead, also the rest
^ This is not working.
gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -DTST_USE_NEWER64_SYSCALL=1 -pthread -D_FORTIFY_SOURCE=2 -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl/../utils -D_GNU_SOURCE -I../../../../include -I../../../../include -I../../../../include/old/ -D_FILE_OFFSET_BITS=64 -c -o fcntl37_64.o fcntl37.c
gcc -L../../../../lib fcntl37_64.o -lltp -o fcntl37_64
../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_create':/home/xzhou/ltp/lib/safe_pthread.c:30: undefined reference to `pthread_create'
../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_join':/home/xzhou/ltp/lib/safe_pthread.c:46: undefined reference to `pthread_join'
collect2: error: ld returned 1 exit status
Quoted 90 lines
of the
Makefileshould be fixed as well.Quoted 2 lines
testcases/kernel/syscalls/fcntl/Makefile +0
-0include $(top_srcdir)/include/mk/testcases.mk include $(abs_srcdir)/../utils/newer_64.mk...
testcases/kernel/syscalls/fcntl/Makefile
Quoted 6 lines
testcases/kernel/syscalls/fcntl/Makefile +6
-0+#include "lapi/fcntl.h" +#include "tst_safe_pthread.h" +#include "tst_test.h" + +static const char fname[] = "tst_lock_execve"; +static const char flag_fname[] = "/tmp/execved";^
You must not create files in random places. Once you set the .needs_tmpdir in the test structure the test is executed with unique temporary directory as working directory so all you need to do for temporary files is to work with relative filenames.testcases/kernel/syscalls/fcntl/Makefile
Quoted 5 lines
testcases/kernel/syscalls/fcntl/Makefile +5
-0+static void cleanup(void); + +static void *thread_fn(void *arg) +{ + int fd = *(int *)arg;^
This is broken, you have to useintptr_tQuoted 14 lines
testcases/kernel/syscalls/fcntl/Makefile +14
-0+ tst_res(TINFO, "Thread running. fd %d", fd); + /* Just need to be alive when execve. */ + sleep(5); + SAFE_CLOSE(fd); + return NULL; +} + +static void checklock(int fd) +{ + pid_t pid = SAFE_FORK(); + if (pid == 0) { + struct flock flck = { + .l_type = F_WRLCK, + .l_whence = SEEK_SET,testcases/kernel/syscalls/fcntl/Makefile
Quoted 10 lines
testcases/kernel/syscalls/fcntl/Makefile +10
-0+ .l_start = 0, + .l_len = 1, + }; + SAFE_FCNTL(fd, F_GETLK, &flck); + if (flck.l_type == F_UNLCK) + tst_res(TFAIL, "Record lock gets lost after execve"); + else + tst_res(TPASS, "Record lock survives execve"); + SAFE_CLOSE(fd); + _exit(0);Why
_exit()?testcases/kernel/syscalls/fcntl/Makefile
Quoted 2 lines
testcases/kernel/syscalls/fcntl/Makefile +2
-0+ } + waitpid(pid, NULL, 0);
SAFE_WAITPID()Quoted 1 line
+}
Also having the check in a separate function and calling it several times makes it kind of hard for debugging. if one of the assertions fails the error messages would be the same. Maybe we should pass a string with describes assertion type to the function and use it int the
tst_res()messages.testcases/kernel/syscalls/fcntl/Makefile
Quoted 12 lines
testcases/kernel/syscalls/fcntl/Makefile +12
-0+static void test01(void) +{ + int fd, ret; + struct stat stat_buf; + + /* + * If tmp/tst_lock_execve exists, execve to run checklock. + */ + ret = stat(flag_fname, &stat_buf); + if (ret == 0) { + checklock(fd); + cleanup();You must not call the cleanup from the test. Cleanup function is called from the test library.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 1 line
testcases/kernel/syscalls/fcntl/Makefile +1
-0+ _exit(0);Again why
_exit()?
Quoted 7 lines
testcases/kernel/syscalls/fcntl/Makefile
Quoted 1 line
testcases/kernel/syscalls/fcntl/Makefile +1
-0+ }Don't do this.
You must not reexecute the testcase and put file flags into random places. If you need
execve()a test helper, you have to write simple binary helper. Seetestcases/kernel/syscalls/creat/creat07_child.c
Thanks for the pointer. If I can define TST_NO_DEFAULT_MAIN and have main() function in this testcase, file flag is not needed. Passing args would do the trick, as Daniel's original reproducer.
Thanks,
Xiong
Quoted 100 lines
testcases/kernel/syscalls/fcntl/Makefile
Quoted 4 lines
testcases/kernel/syscalls/fcntl/Makefile +3
-0+ /* + * If tmp/tst_lock_execve does not exist, initialize it. + */Please no useless comments like this one.
Quoted 4 lines
testcases/kernel/syscalls/fcntl/Makefile +4
-0+ SAFE_OPEN(flag_fname, O_RDWR|O_CREAT, 0755); + fd = SAFE_OPEN(fname, O_RDWR|O_CREAT, 0755); + struct flock64 flck = { + .l_type = F_WRLCK,testcases/kernel/syscalls/fcntl/Makefile
Quoted 10 lines
testcases/kernel/syscalls/fcntl/Makefile +10
-0+ .l_whence = SEEK_SET, + .l_start = 0, + .l_len = 1, + }; + SAFE_FCNTL(fd, F_SETLK, &flck); + + /* + * Creat thread and keep it running after placing posix + * write lock. + */Please no useless comments.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 2 lines
testcases/kernel/syscalls/fcntl/Makefile +2
-0+ pthread_t th; + SAFE_PTHREAD_CREATE(&th, NULL, thread_fn, (void *)&fd);^
Here as well, we have to cast the fd tointptr_tfirst.testcases/kernel/syscalls/fcntl/Makefile
Quoted 1 line
testcases/kernel/syscalls/fcntl/Makefile +1
-0+ sleep(1);Please no random
sleeps()for thread synchronization, either use real thread synchronization primitives or LTP checkpoint synchronization library which is based on futexes.testcases/kernel/syscalls/fcntl/Makefile
Quoted 3 lines
testcases/kernel/syscalls/fcntl/Makefile +3
-0+ /* + * Clear CLOEXEC + */Please no useless comments.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 7 lines
testcases/kernel/syscalls/fcntl/Makefile +7
-0+ int flags=SAFE_FCNTL(fd, F_GETFD); + flags &= ~FD_CLOEXEC; + SAFE_FCNTL(fd, F_SETFD, flags); + + /* + * Get full path name of running programm then execve. + */Please no useless comments.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 3 lines
testcases/kernel/syscalls/fcntl/Makefile +3
-0+ char prog_name[PATH_MAX]; + ret = readlink("/proc/self/exe", prog_name, PATH_MAX); + char * const newargv[] = { prog_name, NULL, NULL };This is far to complicated for no reason. The path to LTP test binaries is in $PATH so you can execute any LTP binary just by it's name. Absolute path is absolutely unneeded.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 5 lines
testcases/kernel/syscalls/fcntl/Makefile +5
-0+ tst_res(TINFO, "execve %s with %s locked", prog_name, fname); + execve(prog_name, newargv, NULL); + /* + * Failure info for debug + */Please no useless comments.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 1 line
testcases/kernel/syscalls/fcntl/Makefile +1
-0+ perror("execve ");No
perror()please. Any error messages should be printed viatst_res()ortst_brk().Quoted 19 lines
testcases/kernel/syscalls/fcntl/Makefile +13
-0+} + +static void cleanup(void) +{ + SAFE_UNLINK(flag_fname); +} + +static struct tst_test test = { + .needs_tmpdir = 1, + .forks_child = 1, + .test_all = test01, + .cleanup = cleanup, +};Signature · 5 lines
1.8.3.1 -- Mailing list info: https://lists.linux.it/listinfo/ltpSignature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 fcntl37: test posix lock across execve
Hi!
Quoted 6 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+fcntl37: LDLIBS += -lpthread +fcntl37_64: LDLIBS += -lpthreadThis is wrong, we should use CFLAGS += -pthread instead, also the rest
^ This is not working.
Quoted 7 lines
gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -DTST_USE_NEWER64_SYSCALL=1 -pthread -D_FORTIFY_SOURCE=2 -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl/../utils -D_GNU_SOURCE -I../../../../include -I../../../../include -I../../../../include/old/ -D_FILE_OFFSET_BITS=64 -c -o fcntl37_64.o
fcntl37.c
gcc -L../../../../lib fcntl37_64.o -lltp -ofcntl37_64
../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_create':/home/xzhou/ltp/lib/safe_pthread.c:30: undefined reference to `pthread_create'
../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_join':/home/xzhou/ltp/lib/safe_pthread.c:46: undefined reference to `pthread_join'
collect2: error: ld returned 1 exit status
Hmm, looks like something is wrong with the newer_64.mk. It picks up a linker rule that does not include CFLAGS. Following patch fixes that, but I'm not 100% sure that it's correct, I will look further into that.
testcases/kernel/syscalls/utils/newer_64.mk +3 -1
@@ -49,7 +49,9 @@ HAS_NEWER_64 := 0 endif %_64: CFLAGS += -D$(DEF_64)=1 -# XXX (garrcoop): End section of code in question.. %_64.o: %.c $(COMPILE.c) $(OUTPUT_OPTION) $< + +%_64: %_64.o + $(LINK.c) $(OUTPUT_OPTION) $< $(LDLIBS)
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/1 fcntl37: test posix lock across execve
Quoted 24 lines · Cyril Hrubis
Hi!
Quoted 6 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+fcntl37: LDLIBS += -lpthread +fcntl37_64: LDLIBS += -lpthreadThis is wrong, we should use CFLAGS += -pthread instead, also the rest
^ This is not working.
Quoted 7 lines
gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -DTST_USE_NEWER64_SYSCALL=1 -pthread -D_FORTIFY_SOURCE=2 -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl/../utils -D_GNU_SOURCE -I../../../../include -I../../../../include -I../../../../include/old/ -D_FILE_OFFSET_BITS=64 -c -o fcntl37_64.o
fcntl37.c
gcc -L../../../../lib fcntl37_64.o -lltp -ofcntl37_64
../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_create':/home/xzhou/ltp/lib/safe_pthread.c:30: undefined reference to `pthread_create'
../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_join':/home/xzhou/ltp/lib/safe_pthread.c:46: undefined reference to `pthread_join'
collect2: error: ld returned 1 exit statusHmm, looks like something is wrong with the newer_64.mk. It picks up a linker rule that does not include CFLAGS. Following patch fixes that, but I'm not 100% sure that it's correct, I will look further into that.
testcases/kernel/syscalls/utils/newer_64.mk +0
-0
testcases/kernel/syscalls/utils/newer_64.mk HAS_NEWER_64 := 0 · line 57
Quoted 10 lines
testcases/kernel/syscalls/utils/newer_64.mk +3
-1@@ -49,7 +49,9 @@ HAS_NEWER_64 := 0 endif %_64: CFLAGS += -D$(DEF_64)=1 -# XXX (garrcoop): End section of code in question.. %_64.o: %.c $(COMPILE.c) $(OUTPUT_OPTION) $< + +%_64: %_64.o + $(LINK.c) $(OUTPUT_OPTION) $< $(LDLIBS)
This patch works fine.
Thanks,
Xiong
Quoted 5 lines
Signature · 3 lines
-- Cyril Hrubis [email protected]
PATCH 1/1 fcntl37: test posix lock across execve
In reply to Cyril HrubisQuoted 9 lines · Cyril Hrubis
Hi!
Quoted 8 lines
testcases/kernel/syscalls/fcntl/Makefile +0
-0@@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread fcntl36: LDLIBS += -lpthread fcntl36_64: LDLIBS += -lpthread
...snip......
Quoted 7 lines
testcases/kernel/syscalls/fcntl/Makefile
Quoted 3 lines
testcases/kernel/syscalls/fcntl/Makefile +3
-0+ char prog_name[PATH_MAX]; + ret = readlink("/proc/self/exe", prog_name, PATH_MAX); + char * const newargv[] = { prog_name, NULL, NULL };This is far to complicated for no reason. The path to LTP test binaries is in $PATH so you can execute any LTP binary just by it's name. Absolute path is absolutely unneeded.
According to man 3 execve and my tests, i see absolute path is needed.
The perror after execve reports "No such file or directory".
Thanks,
Xiong
PATCH 1/1 fcntl37: test posix lock across execve
In reply to Cyril HrubisQuoted 103 lines · Cyril Hrubis
Hi!
testcases/kernel/syscalls/fcntl/Makefile fcntl34_64: LDLIBS += -lpthread · line 31
Quoted 10 lines
testcases/kernel/syscalls/fcntl/Makefile +2
-0@@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread fcntl36: LDLIBS += -lpthread fcntl36_64: LDLIBS += -lpthread +fcntl37: LDLIBS += -lpthread +fcntl37_64: LDLIBS += -lpthreadThis is wrong, we should use CFLAGS += -pthread instead, also the rest of the
Makefileshould be fixed as well.Quoted 2 lines
testcases/kernel/syscalls/fcntl/Makefile +0
-0include $(top_srcdir)/include/mk/testcases.mk include $(abs_srcdir)/../utils/newer_64.mk...
testcases/kernel/syscalls/fcntl/Makefile
Quoted 6 lines
testcases/kernel/syscalls/fcntl/Makefile +6
-0+#include "lapi/fcntl.h" +#include "tst_safe_pthread.h" +#include "tst_test.h" + +static const char fname[] = "tst_lock_execve"; +static const char flag_fname[] = "/tmp/execved";^
You must not create files in random places. Once you set the .needs_tmpdir in the test structure the test is executed with unique temporary directory as working directory so all you need to do for temporary files is to work with relative filenames.testcases/kernel/syscalls/fcntl/Makefile
Quoted 5 lines
testcases/kernel/syscalls/fcntl/Makefile +5
-0+static void cleanup(void); + +static void *thread_fn(void *arg) +{ + int fd = *(int *)arg;^
This is broken, you have to useintptr_tQuoted 14 lines
testcases/kernel/syscalls/fcntl/Makefile +14
-0+ tst_res(TINFO, "Thread running. fd %d", fd); + /* Just need to be alive when execve. */ + sleep(5); + SAFE_CLOSE(fd); + return NULL; +} + +static void checklock(int fd) +{ + pid_t pid = SAFE_FORK(); + if (pid == 0) { + struct flock flck = { + .l_type = F_WRLCK, + .l_whence = SEEK_SET,testcases/kernel/syscalls/fcntl/Makefile
Quoted 10 lines
testcases/kernel/syscalls/fcntl/Makefile +10
-0+ .l_start = 0, + .l_len = 1, + }; + SAFE_FCNTL(fd, F_GETLK, &flck); + if (flck.l_type == F_UNLCK) + tst_res(TFAIL, "Record lock gets lost after execve"); + else + tst_res(TPASS, "Record lock survives execve"); + SAFE_CLOSE(fd); + _exit(0);Why
_exit()?testcases/kernel/syscalls/fcntl/Makefile
Quoted 2 lines
testcases/kernel/syscalls/fcntl/Makefile +2
-0+ } + waitpid(pid, NULL, 0);
SAFE_WAITPID()Quoted 1 line
+}
Also having the check in a separate function and calling it several times makes it kind of hard for debugging. if one of the assertions fails the error messages would be the same. Maybe we should pass a string with describes assertion type to the function and use it int the
tst_res()messages.testcases/kernel/syscalls/fcntl/Makefile
Quoted 12 lines
testcases/kernel/syscalls/fcntl/Makefile +12
-0+static void test01(void) +{ + int fd, ret; + struct stat stat_buf; + + /* + * If tmp/tst_lock_execve exists, execve to run checklock. + */ + ret = stat(flag_fname, &stat_buf); + if (ret == 0) { + checklock(fd); + cleanup();You must not call the cleanup from the test. Cleanup function is called from the test library.
testcases/kernel/syscalls/fcntl/Makefile
Quoted 1 line
testcases/kernel/syscalls/fcntl/Makefile +1
-0+ _exit(0);Again why
_exit()?
Quoted 7 lines
testcases/kernel/syscalls/fcntl/Makefile
Quoted 1 line
testcases/kernel/syscalls/fcntl/Makefile +1
-0+ }Don't do this.
You must not reexecute the testcase and put file flags into random places. If you need
execve()a test helper, you have to write simple binary helper. Seetestcases/kernel/syscalls/creat/creat07_child.c
Finally get this helper working but does not reproduce the original bug we are trying to cover. I guess the testcase re-exec is necessary here.
If this rule is unshakeable, I'll try upstreaming it to other test suite. Rule is rule, I respect rules.
THanks,
Xiong
PATCH 1/1 fcntl37: test posix lock across execve
Hi!
Quoted 3 lines
Finally get this helper working but does not reproduce the original bug we are trying to cover. I guess the testcase re-exec is necessary here.
Can't we have the helper to re-exec itself?
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 1/1 fcntl37: test posix lock across execve
In reply to Xiong Zhou---
v2:
s/LDLIBS/CFLAGS/ in Makefile.
No fork() and no another function when checking the lock. TST_NO_DEFAULT_MAIN to pass fd across execve. Update author info. It's from Daniel originally. I copied
from fcntl36.c for v1 patch and forgot to update it. Sorry for missing this.
testcases/kernel/syscalls/fcntl/Makefile | 11 ++-- testcases/kernel/syscalls/fcntl/fcntl37.c | 101 ++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 4 deletions(-) create mode 100644 testcases/kernel/syscalls/fcntl/fcntl37.c
testcases/kernel/syscalls/fcntl/Makefile +7 -4
@@ -21,11 +21,14 @@ top_srcdir ?= ../../../.. fcntl33: LDLIBS+=-lrt fcntl33_64: LDLIBS+=-lrt -fcntl34: LDLIBS += -lpthread -fcntl34_64: LDLIBS += -lpthread +fcntl34: CFLAGS += -pthread +fcntl34_64: CFLAGS += -pthread -fcntl36: LDLIBS += -lpthread -fcntl36_64: LDLIBS += -lpthread +fcntl36: CFLAGS += -pthread +fcntl36_64: CFLAGS += -pthread + +fcntl37: CFLAGS += -pthread +fcntl37_64: CFLAGS += -pthread include $(top_srcdir)/include/mk/testcases.mk include $(abs_srcdir)/../utils/newer_64.mk
testcases/kernel/syscalls/fcntl/fcntl37.c new file +101 -0
101 lines not on this page · raw message
Signature · 1 line
1.8.3.1
PATCH v2 1/1 fcntl37: test posix lock across execve
Hi!
Quoted 10 lines
testcases/kernel/syscalls/fcntl/Makefile +0
-2@@ -21,11 +21,14 @@ top_srcdir ?= ../../../.. fcntl33: LDLIBS+=-lrt fcntl33_64: LDLIBS+=-lrt -fcntl34: LDLIBS += -lpthread -fcntl34_64: LDLIBS += -lpthread
testcases/kernel/syscalls/fcntl/Makefile top_srcdir ?= ../../../.. · line 31
Quoted 10 lines
testcases/kernel/syscalls/fcntl/Makefile +7
-2+fcntl34: CFLAGS += -pthread +fcntl34_64: CFLAGS += -pthread -fcntl36: LDLIBS += -lpthread -fcntl36_64: LDLIBS += -lpthread +fcntl36: CFLAGS += -pthread +fcntl36_64: CFLAGS += -pthread + +fcntl37: CFLAGS += -pthread +fcntl37_64: CFLAGS += -pthread
Unrelated changes (i.e. something that does not belong to the test being added to LTP) must be put into separate patch.
Quoted 38 lines
testcases/kernel/syscalls/fcntl/Makefile +0
-0include $(top_srcdir)/include/mk/testcases.mk include $(abs_srcdir)/../utils/newer_64.mktestcases/kernel/syscalls/fcntl/fcntl37.c new file +30
-0@@ -0,0 +1,101 @@ +/* +* Copyright (c) 2018 Red Hat Inc. All Rights Reserved. +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License as +* published by the Free Software Foundation; either version 2 of +* the License, or (at your option) any later version. +* +* This program is distributed in the hope that it would be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program. If not, see <http://www.gnu.org/licenses/>. +* +* Author: "Daniel P. Berrang??" <[email protected]> +* +* This is testing for +* +* "Record locks are not inherited by a child created via fork(2), +* but are preserved across an execve(2)." +* +* from fcntl(2) man page. +* +*/ + +#include <sys/types.h> +#include <sys/stat.h> +#include <sys/wait.h>
testcases/kernel/syscalls/fcntl/fcntl37.c line 40
Quoted 10 lines
testcases/kernel/syscalls/fcntl/fcntl37.c new file +10
-0+#include <stdio.h> +#include <stdlib.h> +#include <errno.h> +#include <unistd.h> +#include <fcntl.h> +#include <pthread.h> +#include <sched.h> +#include <limits.h> + +#define TST_NO_DEFAULT_MAIN
Uh, please don't do this.
The main test process has to use the test library default main.
The helper binary, that is executed by execve(), will use this so that the test library is not initilized twice. And the main test process will fork before execve() because it has to stay alive. Just like in the test I've pointed out previously.
Just have a look at the creat07.c and creat07_child.c.
testcases/kernel/syscalls/fcntl/fcntl37.c line 50
Quoted 9 lines
testcases/kernel/syscalls/fcntl/fcntl37.c new file +9
-0+#include "lapi/fcntl.h" +#include "tst_safe_pthread.h" +#include "tst_test.h" + +static const char fname[] ="tst_lock_execve" ; + +static void *thread_fn (void *arg) +{ +int fd = *(intptr_t *)arg;
^
This is still passed by a pointer not by value.
Quoted 29 lines
testcases/kernel/syscalls/fcntl/fcntl37.c new file +29
-0+tst_res (TINFO ,"Thread running. fd %d" , fd); +while (1 )sleep (1 ); +return NULL ; +} + +int main (int argc,char *argv[]) +{ +int fd, ret =0 ; + +if (argc ==2 ) { + fd =atoi (argv[1 ]); +struct flock flck = { + .l_type =F_WRLCK , + .l_whence =SEEK_SET , + .l_start =0 , + .l_len =1 , + }; +SAFE_FCNTL (fd,F_GETLK , &flck); +if (flck.l_type ==F_UNLCK ) +tst_res (TFAIL ,"Record lock gets lost after execve" ); +else +tst_res (TPASS ,"Record lock survives execve" ); +SAFE_CLOSE (fd); +SAFE_UNLINK (fname); +exit (0 ); + } + + fd =SAFE_OPEN (fname,O_RDWR |O_CREAT ,0755 ); +struct flock64 flck = {
testcases/kernel/syscalls/fcntl/fcntl37.c line 89
Quoted 10 lines
testcases/kernel/syscalls/fcntl/fcntl37.c new file +10
-0+ .l_type = F_WRLCK, + .l_whence = SEEK_SET, + .l_start = 0, + .l_len = 1, + }; +SAFE_FCNTL (fd,F_SETLK , &flck); + +pthread_t th; +SAFE_PTHREAD_CREATE (&th,NULL , thread_fn, (void *)&fd); +sleep (1 );
No sleep(1) in test in order to synchronize threads/processes.
testcases/kernel/syscalls/fcntl/fcntl37.c line 96
Quoted 6 lines
testcases/kernel/syscalls/fcntl/fcntl37.c new file +6
-0+int flags=SAFE_FCNTL (fd,F_GETFD ); + flags &= ~FD_CLOEXEC ; +SAFE_FCNTL (fd,F_SETFD , flags); + +char fdstr[10 ]; +snprintf (fdstr,10 ,"%d" , fd);
^ sizeof(fdstr)
Quoted 5 lines
testcases/kernel/syscalls/fcntl/fcntl37.c new file +5
-0+char *const newargv[] = { argv[0 ], fdstr,NULL }; +tst_res (TINFO ,"execve %s with %s locked" , argv[0 ], fname); + ret =execve (argv[0 ], newargv,NULL ); +return ret; +}
Signature · 2 lines
Cyril Hrubis [email protected]