* [LTP] Access02 and use of system() library function
@ 2017-03-16 14:04 Laurent GONZALEZ
2017-03-20 11:11 ` Cyril Hrubis
0 siblings, 1 reply; 6+ messages in thread
From: Laurent GONZALEZ @ 2017-03-16 14:04 UTC (permalink / raw)
To: ltp
Hi list,
in order to check the actual executable mode of a file, access02 calls system() library function.
The test fails when the return value of system() is not 0. Shouldn't the return value being tested only for its sign instead (and even versus -1 only) ?
The system() function returns -1 in case of error, and the return value of the command otherwise. Even in the case the command returns non-zero value, its execution is positively checked, but access02 fails because the return value is not 0.
This corner case happens when the shell is busybox's hush. Hush uses execvp to execute the test_file3, this leads the kernel to return ENOEXEC (bad exec format). Then execvp tries to execute the shell "/bin/sh" with the name of the file "./test_file3" as argv[0]. Then busybox returns that no such an applet does exist, and terminates with error code 127.
Thanks for comments.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] Access02 and use of system() library function
2017-03-16 14:04 [LTP] Access02 and use of system() library function Laurent GONZALEZ
@ 2017-03-20 11:11 ` Cyril Hrubis
2017-03-20 12:22 ` Laurent GONZALEZ
0 siblings, 1 reply; 6+ messages in thread
From: Cyril Hrubis @ 2017-03-20 11:11 UTC (permalink / raw)
To: ltp
Hi!
> in order to check the actual executable mode of a file, access02 calls system() library function.
>
> The test fails when the return value of system() is not 0. Shouldn't the return value being tested only for its sign instead (and even versus -1 only) ?
>
> The system() function returns -1 in case of error, and the return value of the command otherwise. Even in the case the command returns non-zero value, its execution is positively checked, but access02 fails because the return value is not 0.
>
> This corner case happens when the shell is busybox's hush. Hush uses execvp to execute the test_file3, this leads the kernel to return ENOEXEC (bad exec format). Then execvp tries to execute the shell "/bin/sh" with the name of the file "./test_file3" as argv[0]. Then busybox returns that no such an applet does exist, and terminates with error code 127.
What LTP version is this? The access02 test was rewritten some time ago and
there is no test_file3 since commit:
commit adb3e23d9de5f8ff54d2395112f79f25c36cf598
Author: Guangwen Feng <fenggw-fnst@cn.fujitsu.com>
Date: Wed Jul 20 18:44:39 2016 +0800
syscalls/access02: reconstruct and convert to new API
Which is included in last to stable releases....
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] Access02 and use of system() library function
2017-03-20 11:11 ` Cyril Hrubis
@ 2017-03-20 12:22 ` Laurent GONZALEZ
2017-03-30 14:19 ` Cyril Hrubis
0 siblings, 1 reply; 6+ messages in thread
From: Laurent GONZALEZ @ 2017-03-20 12:22 UTC (permalink / raw)
To: ltp
On 20/03/2017 12:11, Cyril Hrubis wrote:
> Hi!
>> in order to check the actual executable mode of a file, access02 calls system() library function.
>>
>> The test fails when the return value of system() is not 0. Shouldn't the return value being tested only for its sign instead (and even versus -1 only) ?
>>
>> The system() function returns -1 in case of error, and the return value of the command otherwise. Even in the case the command returns non-zero value, its execution is positively checked, but access02 fails because the return value is not 0.
>>
>> This corner case happens when the shell is busybox's hush. Hush uses execvp to execute the test_file3, this leads the kernel to return ENOEXEC (bad exec format). Then execvp tries to execute the shell "/bin/sh" with the name of the file "./test_file3" as argv[0]. Then busybox returns that no such an applet does exist, and terminates with error code 127.
> What LTP version is this? The access02 test was rewritten some time ago and
> there is no test_file3 since commit:
>
> commit adb3e23d9de5f8ff54d2395112f79f25c36cf598
> Author: Guangwen Feng <fenggw-fnst@cn.fujitsu.com>
> Date: Wed Jul 20 18:44:39 2016 +0800
>
> syscalls/access02: reconstruct and convert to new API
>
> Which is included in last to stable releases....
>
You're right, i am using an old version of LTP (20160126).
It does not change my remark anyway, newer test does X_OK test using system() library function. The return value is checked against 0 for success, whereas I suggest that it should check against any positive value, or even against any value but -1.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] Access02 and use of system() library function
2017-03-20 12:22 ` Laurent GONZALEZ
@ 2017-03-30 14:19 ` Cyril Hrubis
2017-03-31 7:58 ` Laurent GONZALEZ
0 siblings, 1 reply; 6+ messages in thread
From: Cyril Hrubis @ 2017-03-30 14:19 UTC (permalink / raw)
To: ltp
Hi!
> > What LTP version is this? The access02 test was rewritten some time ago and
> > there is no test_file3 since commit:
> >
> > commit adb3e23d9de5f8ff54d2395112f79f25c36cf598
> > Author: Guangwen Feng <fenggw-fnst@cn.fujitsu.com>
> > Date: Wed Jul 20 18:44:39 2016 +0800
> >
> > syscalls/access02: reconstruct and convert to new API
> >
> > Which is included in last to stable releases....
> >
> You're right, i am using an old version of LTP (20160126).
>
> It does not change my remark anyway, newer test does X_OK test using
> system() library function. The return value is checked against 0 for
> success, whereas I suggest that it should check against any positive
> value, or even against any value but -1.
Hmm, we can always write a shebang to the file then the file should be
recognized by shell, right?
Does this fixes the issue:
diff --git a/testcases/kernel/syscalls/access/access02.c b/testcases/kernel/syscalls/access/access02.c
index 779e81a..86e0342 100644
--- a/testcases/kernel/syscalls/access/access02.c
+++ b/testcases/kernel/syscalls/access/access02.c
@@ -187,6 +187,7 @@ static void setup(void)
SAFE_TOUCH(FNAME_R, 0444, NULL);
SAFE_TOUCH(FNAME_W, 0222, NULL);
SAFE_TOUCH(FNAME_X, 0555, NULL);
+ SAFE_FILE_PRINTF(FNAME_X, "#!/bin/sh\n");
SAFE_SYMLINK(FNAME_F, SNAME_F);
SAFE_SYMLINK(FNAME_R, SNAME_R);
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [LTP] Access02 and use of system() library function
2017-03-30 14:19 ` Cyril Hrubis
@ 2017-03-31 7:58 ` Laurent GONZALEZ
2017-03-31 11:29 ` Cyril Hrubis
0 siblings, 1 reply; 6+ messages in thread
From: Laurent GONZALEZ @ 2017-03-31 7:58 UTC (permalink / raw)
To: ltp
On 30/03/2017 16:19, Cyril Hrubis wrote:
> Hi!
>>> What LTP version is this? The access02 test was rewritten some time ago and
>>> there is no test_file3 since commit:
>>>
>>> commit adb3e23d9de5f8ff54d2395112f79f25c36cf598
>>> Author: Guangwen Feng <fenggw-fnst@cn.fujitsu.com>
>>> Date: Wed Jul 20 18:44:39 2016 +0800
>>>
>>> syscalls/access02: reconstruct and convert to new API
>>>
>>> Which is included in last to stable releases....
>>>
>> You're right, i am using an old version of LTP (20160126).
>>
>> It does not change my remark anyway, newer test does X_OK test using
>> system() library function. The return value is checked against 0 for
>> success, whereas I suggest that it should check against any positive
>> value, or even against any value but -1.
> Hmm, we can always write a shebang to the file then the file should be
> recognized by shell, right?
>
> Does this fixes the issue:
>
> diff --git a/testcases/kernel/syscalls/access/access02.c b/testcases/kernel/syscalls/access/access02.c
> index 779e81a..86e0342 100644
> --- a/testcases/kernel/syscalls/access/access02.c
> +++ b/testcases/kernel/syscalls/access/access02.c
> @@ -187,6 +187,7 @@ static void setup(void)
> SAFE_TOUCH(FNAME_R, 0444, NULL);
> SAFE_TOUCH(FNAME_W, 0222, NULL);
> SAFE_TOUCH(FNAME_X, 0555, NULL);
> + SAFE_FILE_PRINTF(FNAME_X, "#!/bin/sh\n");
>
> SAFE_SYMLINK(FNAME_F, SNAME_F);
> SAFE_SYMLINK(FNAME_R, SNAME_R);
>
Yes it does, and the test passes.
IMHO returning explicitely 0 (exit 0) will make the test even more robust, because 0 is the value returned by the system() library function call, and expected by the test.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] Access02 and use of system() library function
2017-03-31 7:58 ` Laurent GONZALEZ
@ 2017-03-31 11:29 ` Cyril Hrubis
0 siblings, 0 replies; 6+ messages in thread
From: Cyril Hrubis @ 2017-03-31 11:29 UTC (permalink / raw)
To: ltp
Hi!
> Yes it does, and the test passes.
I've pushed the fix.
> IMHO returning explicitely 0 (exit 0) will make the test even more
> robust, because 0 is the value returned by the system() library
> function call, and expected by the test.
The POSIX specification requires the sh to exit with zero if the input
file is empty or consits solely of blank lines and comments. So I would
say that file with just a shebang is robust enough.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-31 11:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-16 14:04 [LTP] Access02 and use of system() library function Laurent GONZALEZ
2017-03-20 11:11 ` Cyril Hrubis
2017-03-20 12:22 ` Laurent GONZALEZ
2017-03-30 14:19 ` Cyril Hrubis
2017-03-31 7:58 ` Laurent GONZALEZ
2017-03-31 11:29 ` Cyril Hrubis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox