Skip to contentSkip to conversation

readdir: rewrite readdir02

PATCH New Li Wang 2 participants 6 messages Dec 20, 2018 – Feb 15, 2019

lore
Share & tools
lore

Li Wang

author
#0 lore raw

PATCH 1/2 readdir: rewrite readdir02

---
 testcases/kernel/syscalls/readdir/readdir02.c | 132 ++++++++------------------
 1 file changed, 42 insertions(+), 90 deletions(-)
testcases/kernel/syscalls/readdir/readdir02.c +42 -90
index 3f3151f6a..441c4b431 100644
@@ -1,20 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (C) Bull S.A. 2001
- * Copyright (c) International Business Machines  Corp., 2001
- *
- * 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 will 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, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ * Copyright (c) IBM Corp., 2001
+ * Copyright (c) Jacky Malcles. 2002
+ * Copyright (c) Robbie Williamson. 2003
+ * Copyright (c) Linux Test Project. 2018
  */
 
 /*
@@ -23,24 +13,17 @@
  *
  * ALGORITHM
  *      loop if that option was specified
- *      call readdir() with an invalid file descriptor
+ *      call readdir() with an invalid directory stream descriptor
  *      check the errno value
  *        issue a PASS message if we get EBADF - errno 9
  *      otherwise, the tests fails
  *        issue a FAIL message
- *        call cleanup
  *
  * NOTE
  *	The POSIX standard says:
  *	  The readdir() function may fail if:
  *	  [EBADF] The dirp argument does not refer to an open directory stream.
  *	  (Note that readdir() is not _required_ to fail in this case.)
- *
- * HISTORY
- *      04/2002 - Written by Jacky Malcles
- *
- *      06/2003 - Added code to catch SIGSEGV and return TCONF.
- *		Robbie Williamson<[email protected]>
  */
 
 #include <sys/types.h>
@@ -52,91 +35,60 @@
 #include <string.h>
 #include <signal.h>
 
-#include "test.h"
+#include "tst_test.h"
 
-static void setup(void);
-static void cleanup(void);
-
-char *TCID = "readdir02";
-int TST_TOTAL = 1;
-
-int main(int ac, char **av)
+static void verify_readdir(void)
 {
-	int lc;
 	DIR *test_dir;
 	struct dirent *dptr;
 
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-		tst_count = 0;
-
-		if ((test_dir = opendir(".")) == NULL) {
-			tst_resm(TFAIL, "opendir(\".\") Failed, errno=%d : %s",
+	if ((test_dir = opendir(".")) == NULL) {
+		tst_res(TFAIL, "opendir(\".\") Failed, errno=%d : %s",
+			 errno, strerror(errno));
+	} else {
+		if (closedir(test_dir) < 0) {
+			tst_res(TFAIL,
+				 "closedir(\".\") Failed, errno=%d : %s",
 				 errno, strerror(errno));
 		} else {
-			if (closedir(test_dir) < 0) {
-				tst_resm(TFAIL,
-					 "closedir(\".\") Failed, errno=%d : %s",
+			dptr = readdir(test_dir);
+			switch (errno) {
+			case EBADF:
+				tst_res(TPASS,
+					 "expected failure - errno = %d : %s",
 					 errno, strerror(errno));
-			} else {
-				dptr = readdir(test_dir);
-				switch (errno) {
-				case EBADF:
-					tst_resm(TPASS,
-						 "expected failure - errno = %d : %s",
-						 errno, strerror(errno));
-					break;
-				default:
-					if (dptr != NULL) {
-						tst_brkm(TFAIL, cleanup,
-							 "call failed with an "
-							 "unexpected error - %d : %s",
-							 errno,
-							 strerror(errno));
-					} else {
-						tst_resm(TINFO,
-							 "readdir() is not _required_ to fail, "
-							 "errno = %d  ", errno);
-					}
+				break;
+			default:
+				if (dptr != NULL) {
+					tst_brk(TFAIL,
+						 "call failed with an "
+						 "unexpected error - %d : %s",
+						 errno,
+						 strerror(errno));
+				} else {
+					tst_res(TINFO,
+						 "readdir() is not _required_ to fail, "
+						 "errno = %d  ", errno);
 				}
 			}
-
 		}
-
 	}
-
-	cleanup();
-	tst_exit();
 }
 
-static void sigsegv_handler(int sig)
+static void sighandler(int sig LTP_ATTRIBUTE_UNUSED)
 {
-	tst_resm(TCONF,
-		 "This system's implementation of closedir() will not allow this test to execute properly.");
-	cleanup();
+	tst_res(TCONF,
+		 "This system's implementation of closedir() "
+		 "will not allow this test to execute properly.");
 }
 
 static void setup(void)
 {
-	struct sigaction act;
-
-	tst_sig(NOFORK, DEF_HANDLER, cleanup);
-
-	act.sa_handler = sigsegv_handler;
-	act.sa_flags = 0;
-	sigemptyset(&act.sa_mask);
-	sigaction(SIGSEGV, &act, NULL);
-
-	TEST_PAUSE;
-
-	tst_tmpdir();
+	SAFE_SIGNAL(SIGSEGV, sighandler);
 }
 
-static void cleanup(void)
-{
-	tst_rmdir();
-}
+static struct tst_test test = {
+	.needs_tmpdir = 1,
+	.setup = setup,
+	.test_all = verify_readdir,
+};
Signature · 1 line
2.14.5
author
#1 lore raw

PATCH 2/2 readdir02: use invalid DIR stream descriptor

Issue:
On ppc64le and aarch64, when testing in NFS mountpoint, test process receives SIGSEGV when calling readdir on a DIR which has just been closed by closedir().

Unfortunately, ltp/readdir02.c handles SIGSEGV. This makes it hits SIGSEGV again in its cleanup function. So readdir02 hangs there hitting SEGV endlessly.

That's because a DIR * is NOT a file descriptor. It's memory allocated by opendir() that contains libc internal information about the directory. closedir(test_dir) frees any memory associated with the open directory pointer test_dir.

To then pass the freed dir pointer to readdir() is a use-after-free. It probably won't return EBADF, it will dereference freed memory and whatever happens after that is undefined.

In this patch, I simply modify the test to use an exist FILE * stream to simulate the invalid directory stream descriptor. Then it won't hit the use-after-free issue any more.

Also, the sighandler function has been dropped.

---
 testcases/kernel/syscalls/readdir/readdir02.c | 64 +++++++++++----------------
 1 file changed, 25 insertions(+), 39 deletions(-)
testcases/kernel/syscalls/readdir/readdir02.c +25 -39
index 441c4b431..21d00cb0a 100644
@@ -36,59 +36,45 @@
 #include <signal.h>
 
 #include "tst_test.h"
+#include "tst_safe_stdio.h"
+
+#define TEST_FILE "readdir_file.txt"
 
 static void verify_readdir(void)
 {
+	FILE *fp;
 	DIR *test_dir;
 	struct dirent *dptr;
 
-	if ((test_dir = opendir(".")) == NULL) {
-		tst_res(TFAIL, "opendir(\".\") Failed, errno=%d : %s",
-			 errno, strerror(errno));
-	} else {
-		if (closedir(test_dir) < 0) {
+	fp = SAFE_FOPEN(TEST_FILE, "ab+");
+	/* regard FILE * as an invalid directory stream descriptor */
+	test_dir = (DIR *)fp;
+
+	dptr = readdir(test_dir);
+	switch (errno) {
+	case EBADF:
+		tst_res(TPASS,
+			"expected failure - errno = %d : %s",
+			errno, strerror(errno));
+		break;
+	default:
+		if (dptr != NULL) {
 			tst_res(TFAIL,
-				 "closedir(\".\") Failed, errno=%d : %s",
-				 errno, strerror(errno));
+				"call failed with an "
+				"unexpected error - %d : %s",
+				errno,
+				strerror(errno));
 		} else {
-			dptr = readdir(test_dir);
-			switch (errno) {
-			case EBADF:
-				tst_res(TPASS,
-					 "expected failure - errno = %d : %s",
-					 errno, strerror(errno));
-				break;
-			default:
-				if (dptr != NULL) {
-					tst_brk(TFAIL,
-						 "call failed with an "
-						 "unexpected error - %d : %s",
-						 errno,
-						 strerror(errno));
-				} else {
-					tst_res(TINFO,
-						 "readdir() is not _required_ to fail, "
-						 "errno = %d  ", errno);
-				}
-			}
+			tst_res(TINFO,
+				"readdir() is not _required_ to fail, "
+				"errno = %d ", errno);
 		}
 	}
-}
 
-static void sighandler(int sig LTP_ATTRIBUTE_UNUSED)
-{
-	tst_res(TCONF,
-		 "This system's implementation of closedir() "
-		 "will not allow this test to execute properly.");
-}
-
-static void setup(void)
-{
-	SAFE_SIGNAL(SIGSEGV, sighandler);
+	SAFE_FCLOSE(fp);
 }
 
 static struct tst_test test = {
 	.needs_tmpdir = 1,
-	.setup = setup,
 	.test_all = verify_readdir,
 };
Signature · 1 line
2.14.5
#2 lore raw

PATCH 2/2 readdir02: use invalid DIR stream descriptor

Hi!

Quoted 14 lines

Issue:
On ppc64le and aarch64, when testing in NFS mountpoint, test process receives SIGSEGV when calling readdir on a DIR which has just been closed by closedir().

Unfortunately, ltp/readdir02.c handles SIGSEGV. This makes it hits SIGSEGV again in its cleanup function. So readdir02 hangs there hitting SEGV endlessly.

That's because a DIR * is NOT a file descriptor. It's memory allocated by opendir() that contains libc internal information about the directory. closedir(test_dir) frees any memory associated with the open directory pointer test_dir.

Quoted 7 lines

To then pass the freed dir pointer to readdir() is a use-after-free. It probably won't return EBADF, it will dereference freed memory and whatever happens after that is undefined.

In this patch, I simply modify the test to use an exist FILE * stream to simulate the invalid directory stream descriptor. Then it won't hit the use-after-free issue any more.

Actually I think that the best we can do here is to delete the testcase because:

* Casting FILE* to DIR* is IMHO invoking even worse undefined behavior than the original test that called readdir() on closed DIR*

* We do cover the EBADF for getents() syscalls getents02 test

Signature · 2 lines
Cyril Hrubis
[email protected]
author
#3 lore raw

PATCH 2/2 readdir02: use invalid DIR stream descriptor

Quoted 11 lines · Cyril Hrubis
Quoted 4 lines

In this patch, I simply modify the test to use an exist FILE * stream to simulate the invalid directory stream descriptor. Then it won't hit the use-after-free issue any more.

Actually I think that the best we can do here is to delete the testcase because:

* Casting FILE* to DIR* is IMHO invoking even worse undefined behavior than the original test that called readdir() on closed DIR*

Why say this? Does this CASTING will do something more bad? AFAICT that changing an variable of one data type into another, and the worst harmness is to loss of information in the variable so we'd better avoid that. But in this test we only need a invalid DIR* for readdir() tesst, it does *not* really care about the pointer content I guess?

Quoted 3 lines

* We do cover the EBADF for getents() syscalls getents02 test

I'm sorry, I don't find this testcase in LTP, or did I miss anything?

Signature · 5 lines
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190201/b5f0bd6a/attachment-0001.html>
#4 lore raw

PATCH 2/2 readdir02: use invalid DIR stream descriptor

Hi!

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines

In this patch, I simply modify the test to use an exist FILE * stream to simulate the invalid directory stream descriptor. Then it won't hit the use-after-free issue any more.

Quoted 8 lines
Quoted 6 lines

Actually I think that the best we can do here is to delete the testcase because:

* Casting FILE* to DIR* is IMHO invoking even worse undefined behavior than the original test that called readdir() on closed DIR*

Why say this? Does this CASTING will do something more bad?

Yes.

Quoted 5 lines

AFAICT that changing an variable of one data type into another, and the worst harmness is to loss of information in the variable so we'd better avoid that. But in this test we only need a invalid DIR* for readdir() tesst, it does *not* really care about the pointer content I guess?

Not at all, both FILE and DIR are typedefs to C structures, which are just chunks of memory, by doing this you are basically passing random data to the call because all it does when the C library gets the fd from these strucutres is that it takes bytes from at some offest in the chunk of memory. There are no abstract types, methods or objects in C, just chunks of memory.

Quoted 5 lines
Quoted 3 lines

* We do cover the EBADF for getents() syscalls getents02 test

I'm sorry, I don't find this testcase in LTP, or did I miss anything?

Sorry typo, it's getdents02.

Signature · 2 lines
Cyril Hrubis
[email protected]
author
#5 lore raw

PATCH 2/2 readdir02: use invalid DIR stream descriptor

Hi Cyril,

Sorry for late reply, since it was China holidays in past two weeks.

Quoted 7 lines · Cyril Hrubis
Quoted 6 lines

AFAICT that changing an variable of one data type into another, and the worst harmness is to loss of information in the variable so we'd better avoid that. But in this test we only need a invalid DIR* for readdir() tesst, it does *not* really care about the pointer content I guess?

Quoted 8 lines

Not at all, both FILE and DIR are typedefs to C structures, which are just chunks of memory, by doing this you are basically passing random data to the call because all it does when the C library gets the fd from these strucutres is that it takes bytes from at some offest in the chunk of memory. There are no abstract types, methods or objects in C, just chunks of memory.

Ok, I got the point, thanks for the explanation. I will help to rewrite a new patch for readdir02 deleting.

Signature · 5 lines
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190215/9363425b/attachment.html>