|
From: <sv...@va...> - 2009-07-23 04:30:15
|
Author: njn
Date: 2009-07-23 05:30:06 +0100 (Thu, 23 Jul 2009)
New Revision: 10539
Log:
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.
Added:
trunk/memcheck/tests/darwin/aio.c
trunk/memcheck/tests/darwin/aio.stderr.exp
trunk/memcheck/tests/darwin/aio.vgtest
Modified:
trunk/coregrind/m_syswrap/priv_syswrap-generic.h
trunk/coregrind/m_syswrap/syswrap-darwin.c
trunk/memcheck/tests/darwin/Makefile.am
Modified: trunk/coregrind/m_syswrap/priv_syswrap-generic.h
===================================================================
--- trunk/coregrind/m_syswrap/priv_syswrap-generic.h 2009-07-23 00:55:46 UTC (rev 10538)
+++ trunk/coregrind/m_syswrap/priv_syswrap-generic.h 2009-07-23 04:30:06 UTC (rev 10539)
@@ -50,7 +50,7 @@
// 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);
Modified: trunk/coregrind/m_syswrap/syswrap-darwin.c
===================================================================
--- trunk/coregrind/m_syswrap/syswrap-darwin.c 2009-07-23 00:55:46 UTC (rev 10538)
+++ trunk/coregrind/m_syswrap/syswrap-darwin.c 2009-07-23 04:30:06 UTC (rev 10539)
@@ -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,14 +3440,49 @@
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,19 +3508,56 @@
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 @@
// _____(__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),
Modified: trunk/memcheck/tests/darwin/Makefile.am
===================================================================
--- trunk/memcheck/tests/darwin/Makefile.am 2009-07-23 00:55:46 UTC (rev 10538)
+++ trunk/memcheck/tests/darwin/Makefile.am 2009-07-23 04:30:06 UTC (rev 10539)
@@ -6,6 +6,7 @@
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 @@
scalar_vfork.stderr.exp scalar_vfork.vgtest
check_PROGRAMS = \
+ aio \
env \
pth-supp \
scalar \
Added: trunk/memcheck/tests/darwin/aio.c
===================================================================
--- trunk/memcheck/tests/darwin/aio.c (rev 0)
+++ trunk/memcheck/tests/darwin/aio.c 2009-07-23 04:30:06 UTC (rev 10539)
@@ -0,0 +1,89 @@
+#include <assert.h>
+#include <aio.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <string.h>
+
+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;
+};
+
+
+
Added: trunk/memcheck/tests/darwin/aio.stderr.exp
===================================================================
--- trunk/memcheck/tests/darwin/aio.stderr.exp (rev 0)
+++ trunk/memcheck/tests/darwin/aio.stderr.exp 2009-07-23 04:30:06 UTC (rev 10539)
@@ -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
Added: trunk/memcheck/tests/darwin/aio.vgtest
===================================================================
--- trunk/memcheck/tests/darwin/aio.vgtest (rev 0)
+++ trunk/memcheck/tests/darwin/aio.vgtest 2009-07-23 04:30:06 UTC (rev 10539)
@@ -0,0 +1,3 @@
+prog: aio
+vgopts: --leak-check=no
+stderr_filter: ../filter_allocs
|