syscalls: nullwrite [RFC] #217

carenas · 2017-10-01T14:16:03Z

check for a special case of write, when a NULL buffer is used with a size of 0 and which result in a 0 response, instead of -1 (EFAULT)

metan-ucw · 2017-10-02T12:31:27Z

Looking at the manual page, one of the errors listed is EFAULT, so as far as I can tell write() is allowed to fail with EFAULT with NULL buffer and zero size. If we want to check for no-op write() we should pass a valid buffer pointer as well.

Also these two testcases should be cleaned up and converted to the new test library before we start adding new functionality.

See:
https://github.com/linux-test-project/ltp/wiki/C-Test-Case-Tutorial
https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines

Or have a look at access() testcases that have been already converted.

carenas · 2017-10-05T08:09:36Z

my intention was to check for the specific case where both NULL and a size of 0 are being used, and that was found recently to fail in qemu (linux-user) at least when both host and guest were 64bit and which would therefore seem to be an exception for the tests that were modified.

agree though that both, -1 (EFAULT) and 0 are documented as valid, but hadn't been able to find an scenario where and EFAULT is generated with NULL, and while the code in linux is eventually arch specific (looking for access for the buffer), it would seem that at least for NULL; 0 would be the expected response for all the paths I'd read (I don't have access to all those architectures though to test, and qemu is obviously not an option, unless full system emulation is used)

interestingly enough, I'd found the same to be true in BSD and macOS, which could explain why some code might be intentionally using that and not expecting a failure, and even a comment in uclibc that says that a NULL buffer is valid and ignored (so it would never generate a fault), but hadn't been able to find any specifics and from the history of the documentation, it would seem it was something that was found to work and therefore clarified upon to be intentionally an expected response.

FWIW write(fd, (void *)-1, 0) returns always -1 (EFAULT) as it will be expected of less obvious invalid buffers, including NULL.

metan-ucw · 2017-10-05T09:27:25Z

If there is a code in the wild that makes use of the NULL and 0 combination it certainly makes sense to test it, however, in order to make the intentions clear, can you please include this information in the commit message?

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>

also correct typo: read() -> write() Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>

removes uclinux compatibility as a sideeffect of new test library not supporting MAP_PRIVATE_EXCEPT_UCLINUX or FORK_OR_VFORK Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>

As documented, calling write with 0 as nsize is valid and should return 0. Passing NULL as a buffer is also valid, and when combined with a size of 0 has been shown historically to result in a returned value of 0. SysV and BSD make a point to always return 0, regardless of the buffer and therefore some software expects a call like write(fd, NULL, 0) to never fail, so test for that combination as an exception to this test that looks for an EFAULT. The fact that this test was disabled for uclinux and a comment on their implementation seems to suggest that this exception was known before and was therefore clarified as suported later in the documentation. Signed-off-by: Carlo Marcelo Arenas Belón <carlo@gmail.com>

metan-ucw · 2017-10-25T13:02:00Z

testcases/kernel/syscalls/write/write04.c

-	/* global setup */
-	setup();
+	if (stat(fifo, &buf) != 0)
+		tst_res(TBROK | TERRNO, "stat() failed, errno: %d", errno);



We do have the SAFE_FOO() macros for most of the syscalls like the stat(), mknod(), etc. can you please make use of them: https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#224-safe-macros ?

yes; I intentionally kept the old code to make sure I didn't introduce accidentally a bug by misusing them, or change the logic (indeed, I also kept code that is buggy, so the commit will be also bug compatible).

do you want me to add commits to this branch that do all those additional changes?, close this and create a new one? or just push --force on top of this with the next RC?

For rewriting old buggy testcases I'm fine a single patch per test rewrite. The incremental approach does not make IMHO much sense there, so I don't tend to overcomplicate the git log, but I'm fine with incremental approach as well. So choose what suits you best.

metan-ucw · 2017-10-25T13:02:45Z

testcases/kernel/syscalls/write/write04.c


-		(void)memset((void *)wbuf, 'A', 17 * PIPE_SIZE_TEST);
+	(void)memset((void *)wbuf, 'A', 17 * PIPE_SIZE_TEST);



Please just get rid of all the (void) cast for the return values, these are not useful for anything.

metan-ucw · 2017-10-25T13:04:52Z

testcases/kernel/syscalls/write/write04.c

+	(void)alarm(10);	/* set alarm for 10 seconds */
+	rfd = open(fifo, O_RDONLY | O_NONBLOCK);
+	(void)alarm(0);
+	if (rfd < 0) {


And also get rid of all the alarm() calls and the alarm handler and the sigsetjmp() mess, the new test library handles the test timeout itself, no need to add hacking timeouts like this one.

metan-ucw · 2017-10-25T13:05:57Z

testcases/kernel/syscalls/write/write04.c

@@ -208,32 +182,24 @@ void alarm_handler(void)
 * setup()
 *	performs all ONE TIME setup for this test
 */
-void setup(void)


And also please get rid of useless comments like this one that are explaining the obvious.

metan-ucw · 2017-10-25T13:13:18Z

testcases/kernel/syscalls/pwrite/pwrite02.c

+	.setup = setup,
+	.test = verify_pwrite,
+	.tcnt = ARRAY_SIZE(testfunc),
+};


Well we usually tend to use more data driven approach for testcases that check for the invalid syscall inputs, just have a look at syscalls/read/read02.c or do 'git grep struct tcase' but if you do not want to spend more time on this the code is clean enough to be accepted as well.

metan-ucw · 2017-10-25T13:16:10Z