From: Nicholas Nethercote Date: Thu, 23 Jul 2009 04:30:06 +0000 (+0000) Subject: Handle the memory written by aio_read() properly -- mark the memory buffer X-Git-Tag: svn/VALGRIND_3_5_0~309 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=9cecb57a28fce6b95505279399ab6b25425d9001;p=thirdparty%2Fvalgrind.git Handle the memory written by aio_read() properly -- mark the memory buffer as written once aio_return() is successfully called. Also check the addressability of the buffer for both aio_read() and aio_write(). Also check the file descriptor for aio_read() and aio_write(). And add a test for this. There's one corner case of the test that doesn't work as expected and is currently commented out. But aio_*() certainly works better than it used to. All this is for bug 197227. git-svn-id: svn://svn.valgrind.org/valgrind/trunk@10539 --- diff --git a/coregrind/m_syswrap/priv_syswrap-generic.h b/coregrind/m_syswrap/priv_syswrap-generic.h index 1d66d64021..201c4466f6 100644 --- a/coregrind/m_syswrap/priv_syswrap-generic.h +++ b/coregrind/m_syswrap/priv_syswrap-generic.h @@ -50,7 +50,7 @@ extern Bool ML_(client_signal_OK)(Int sigNo); // Return true if we're allowed to use or create this fd. extern -Bool ML_(fd_allowed)(Int fd, const Char *syscallname, ThreadId tid, Bool soft); +Bool ML_(fd_allowed)(Int fd, const Char *syscallname, ThreadId tid, Bool isNewFD); extern void ML_(record_fd_open_named) (ThreadId tid, Int fd); extern void ML_(record_fd_open_nameless) (ThreadId tid, Int fd); diff --git a/coregrind/m_syswrap/syswrap-darwin.c b/coregrind/m_syswrap/syswrap-darwin.c index 4f1b85692a..29fb0dd37d 100644 --- a/coregrind/m_syswrap/syswrap-darwin.c +++ b/coregrind/m_syswrap/syswrap-darwin.c @@ -49,6 +49,7 @@ #include "pub_core_machine.h" // VG_(get_SP) #include "pub_core_mallocfree.h" #include "pub_core_options.h" +#include "pub_core_oset.h" #include "pub_core_scheduler.h" #include "pub_core_sigframe.h" // For VG_(sigframe_destroy)() #include "pub_core_signals.h" @@ -3439,13 +3440,48 @@ PRE(sigsuspend) PRE_REG_READ1(int, "sigsuspend", int, sigmask); } +/* --------------------------------------------------------------------- + aio_* + ------------------------------------------------------------------ */ + +// We must record the aiocbp for each aio_read() in a table so that when +// aio_return() is called we can mark the memory written asynchronously by +// aio_read() as having been written. We don't have to do this for +// aio_write(). See bug 197227 for more details. +static OSet* aiocbp_table = NULL; +static Bool aio_init_done = False; + +static void aio_init(void) +{ + aiocbp_table = VG_(OSetWord_Create)(VG_(malloc), "syswrap.aio", VG_(free)); + aio_init_done = True; +} + +static Bool was_a_successful_aio_read = False; PRE(aio_return) { + struct vki_aiocb* aiocbp = (struct vki_aiocb*)ARG1; // This assumes that the kernel looks at the struct pointer, but not the // contents of the struct. PRINT( "aio_return ( %#lx )", ARG1 ); PRE_REG_READ1(long, "aio_return", struct vki_aiocb*, aiocbp); + + if (!aio_init_done) aio_init(); + was_a_successful_aio_read = VG_(OSetWord_Remove)(aiocbp_table, (UWord)aiocbp); +} +POST(aio_return) +{ + // If we found the aiocbp in our own table it must have been an aio_read(), + // so mark the buffer as written. If we didn't find it, it must have been + // an aio_write() or a bogus aio_return() (eg. a second one on the same + // aiocbp). Either way, the buffer won't have been written so we don't + // have to mark the buffer as written. + if (was_a_successful_aio_read) { + struct vki_aiocb* aiocbp = (struct vki_aiocb*)ARG1; + POST_MEM_WRITE((Addr)aiocbp->aio_buf, aiocbp->aio_nbytes); + was_a_successful_aio_read = False; + } } PRE(aio_suspend) @@ -3472,18 +3508,55 @@ PRE(aio_error) PRE(aio_read) { + struct vki_aiocb* aiocbp = (struct vki_aiocb*)ARG1; + PRINT( "aio_read ( %#lx )", ARG1 ); PRE_REG_READ1(long, "aio_read", struct vki_aiocb*, aiocbp); PRE_MEM_READ( "aio_read(aiocbp)", ARG1, sizeof(struct vki_aiocb)); + + if (ML_(safe_to_deref)(aiocbp, sizeof(struct vki_aiocb))) { + if (ML_(fd_allowed)(aiocbp->aio_fildes, "aio_read", tid, /*isNewFd*/False)) { + PRE_MEM_WRITE("aio_read(aiocbp->aio_buf)", + (Addr)aiocbp->aio_buf, aiocbp->aio_nbytes); + } else { + SET_STATUS_Failure( VKI_EBADF ); + } + } else { + SET_STATUS_Failure( VKI_EINVAL ); + } +} +POST(aio_read) +{ + // We have to record the fact that there is an asynchronous read request + // pending. When a successful aio_return() occurs for this aiocb, then we + // will mark the memory as having been defined. + struct vki_aiocb* aiocbp = (struct vki_aiocb*)ARG1; + if (!aio_init_done) aio_init(); + // aiocbp shouldn't already be in the table -- if it was a dup, the kernel + // should have caused the aio_read() to fail and we shouldn't have reached + // here. + VG_(OSetWord_Insert)(aiocbp_table, (UWord)aiocbp); } PRE(aio_write) { + struct vki_aiocb* aiocbp = (struct vki_aiocb*)ARG1; + PRINT( "aio_write ( %#lx )", ARG1 ); PRE_REG_READ1(long, "aio_write", struct vki_aiocb*, aiocbp); PRE_MEM_READ( "aio_write(aiocbp)", ARG1, sizeof(struct vki_aiocb)); -} + if (ML_(safe_to_deref)(aiocbp, sizeof(struct vki_aiocb))) { + if (ML_(fd_allowed)(aiocbp->aio_fildes, "aio_write", tid, /*isNewFd*/False)) { + PRE_MEM_READ("aio_write(aiocbp->aio_buf)", + (Addr)aiocbp->aio_buf, aiocbp->aio_nbytes); + } else { + SET_STATUS_Failure( VKI_EBADF ); + } + } else { + SET_STATUS_Failure( VKI_EINVAL ); + } +} /* --------------------------------------------------------------------- mach_msg: formatted messages @@ -7430,11 +7503,11 @@ const SyscallTableEntry ML_(syscall_table)[] = { // _____(__NR_settid_with_pid), // _____(__NR___pthread_cond_timedwait), // _____(__NR_aio_fsync), - MACX_(__NR_aio_return, aio_return), + MACXY(__NR_aio_return, aio_return), MACX_(__NR_aio_suspend, aio_suspend), // _____(__NR_aio_cancel), MACX_(__NR_aio_error, aio_error), - MACX_(__NR_aio_read, aio_read), + MACXY(__NR_aio_read, aio_read), MACX_(__NR_aio_write, aio_write), // _____(__NR_lio_listio), // 320 // _____(__NR___pthread_cond_wait), diff --git a/memcheck/tests/darwin/Makefile.am b/memcheck/tests/darwin/Makefile.am index 1027300ec0..9ef89645ba 100644 --- a/memcheck/tests/darwin/Makefile.am +++ b/memcheck/tests/darwin/Makefile.am @@ -6,6 +6,7 @@ dist_noinst_SCRIPTS = filter_stderr noinst_HEADERS = scalar.h EXTRA_DIST = \ + aio.stderr.exp aio.vgtest \ env.stderr.exp env.vgtest \ pth-supp.stderr.exp pth-supp.vgtest \ scalar.stderr.exp scalar.vgtest \ @@ -14,6 +15,7 @@ EXTRA_DIST = \ scalar_vfork.stderr.exp scalar_vfork.vgtest check_PROGRAMS = \ + aio \ env \ pth-supp \ scalar \ diff --git a/memcheck/tests/darwin/aio.c b/memcheck/tests/darwin/aio.c new file mode 100644 index 0000000000..e0dceef8f7 --- /dev/null +++ b/memcheck/tests/darwin/aio.c @@ -0,0 +1,89 @@ +#include +#include +#include +#include +#include + +int x; + +int main(void) +{ + #define LEN 10 + char buf[LEN]; + + struct aiocb a; + struct sigevent s; + + // Not sure if the sigevent is even looked at by aio_*... just zero it. + memset(&s, 0, sizeof(struct sigevent)); + + a.aio_fildes = -1; + a.aio_offset = 0; + a.aio_buf = NULL; + a.aio_nbytes = LEN; + a.aio_reqprio = 0; + a.aio_sigevent = s; + a.aio_lio_opcode = 0; // ignored + + //------------------------------------------------------------------------ + // The cases where aiocbp itself points to bogus memory is handled in + // memcheck/tests/darwin/scalar.c, so we don't check that here. + + //------------------------------------------------------------------------ + // XXX: This causes an unexpected undef value error later, at the XXX mark. + // Not sure why, it shouldn't. + // assert( aio_return(&a) < 0); // (aiocbp hasn't been inited) + + //------------------------------------------------------------------------ + assert( aio_read(&a) < 0); // invalid fd + + //------------------------------------------------------------------------ + a.aio_fildes = open("aio.c", O_RDONLY); + assert(a.aio_fildes >= 0); + + assert( aio_read(&a) < 0); // unaddressable aio_buf + + //------------------------------------------------------------------------ + a.aio_buf = buf; + + assert( aio_read(&a) == 0 ); + + assert( aio_read(&a) < 0 ); // (don't crash on the repeated &a) + + while (0 != aio_error(&a)) { }; + + if (buf[0] == buf[9]) x++; // undefined -- aio_return() not called yet + + assert( aio_return(&a) > 0 ); // XXX: (undefined value error here) + + if (buf[0] == buf[9]) x++; + + assert( aio_return(&a) < 0 ); // (repeated aio_return(); fails because + // Valgrind can't find &a in the table) + + //------------------------------------------------------------------------ + a.aio_buf = 0; + a.aio_fildes = creat("mytmpfile", S_IRUSR|S_IWUSR); + assert(a.aio_fildes >= 0); + + assert( aio_write(&a) < 0); // unaddressable aio_buf + + //------------------------------------------------------------------------ + a.aio_buf = buf; + + assert( aio_write(&a) == 0 ); + + assert( aio_write(&a) < 0 ); // (don't crash on the repeated &a) + + while (0 != aio_error(&a)) { }; + + assert( aio_return(&a) > 0 ); + + assert( aio_return(&a) < 0 ); // (repeated aio_return(); fails because + // Valgrind can't find &a in the table) + + return x; +}; + + + diff --git a/memcheck/tests/darwin/aio.stderr.exp b/memcheck/tests/darwin/aio.stderr.exp new file mode 100644 index 0000000000..e4c0d4a490 --- /dev/null +++ b/memcheck/tests/darwin/aio.stderr.exp @@ -0,0 +1,19 @@ + +Warning: invalid file descriptor -1 in syscall aio_read() +Syscall param aio_read(aiocbp->aio_buf) points to unaddressable byte(s) + ... + Address 0x........ is not stack'd, malloc'd or (recently) free'd + +Conditional jump or move depends on uninitialised value(s) + at 0x........: main (aio.c:55) + +Syscall param aio_write(aiocbp->aio_buf) points to unaddressable byte(s) + ... + Address 0x........ is not stack'd, malloc'd or (recently) free'd + +ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0) +malloc/free: in use at exit: ... bytes in ... blocks. +malloc/free: ... allocs, ... frees, ... bytes allocated. +For a detailed leak analysis, rerun with: --leak-check=yes +For counts of detected errors, rerun with: -v +Use --track-origins=yes to see where uninitialised values come from diff --git a/memcheck/tests/darwin/aio.vgtest b/memcheck/tests/darwin/aio.vgtest new file mode 100644 index 0000000000..020a3ad26f --- /dev/null +++ b/memcheck/tests/darwin/aio.vgtest @@ -0,0 +1,3 @@ +prog: aio +vgopts: --leak-check=no +stderr_filter: ../filter_allocs