| Message ID | 20181220090811.21514-2-liwang@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] readdir: rewrite readdir02 | expand |
Hi! > 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. 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
On Mon, Jan 28, 2019 at 11:19 PM Cyril Hrubis <chrubis@suse.cz> wrote: > > > > 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? > > * 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?
Hi! > > > 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? Yes. > 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. > > > > * 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.
Hi Cyril, Sorry for late reply, since it was China holidays in past two weeks. On Thu, Feb 7, 2019 at 8:51 PM Cyril Hrubis <chrubis@suse.cz> wrote: > > > 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. > > Ok, I got the point, thanks for the explanation. I will help to rewrite a new patch for readdir02 deleting.
diff --git a/testcases/kernel/syscalls/readdir/readdir02.c b/testcases/kernel/syscalls/readdir/readdir02.c index 441c4b431..21d00cb0a 100644 --- a/testcases/kernel/syscalls/readdir/readdir02.c +++ b/testcases/kernel/syscalls/readdir/readdir02.c @@ -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, };
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. Reported-by: Xiong Zhou <xzhou@redhat.com> Signed-off-by: Li Wang <liwang@redhat.com> Cc: Dave Chinner <dchinner@redhat.com> Cc: Scott Mayhew <smayhew@redhat.com> --- testcases/kernel/syscalls/readdir/readdir02.c | 64 +++++++++++---------------- 1 file changed, 25 insertions(+), 39 deletions(-)