date: 2002/11/09 10:42:01; author: hno; state: Exp; lines: +46 -20

Bugzilla #451: Occasional corruption of objects

This patch fixes a cbdata boundary violation in the aufs storeio
implementation wich caused data corruption on aborted cache hits.

diff --git a/src/fs/aufs/aiops.cc b/src/fs/aufs/aiops.cc
index d47c14ce159a4c09033d50adfec1506dfa6635d6..7929d6803b755b9fea3567016cc95c81c743b497 100644 (file)
--- a/src/fs/aufs/aiops.cc
+++ b/src/fs/aufs/aiops.cc
@@ -1,5 +1,5 @@
 /*
- * $Id: aiops.cc,v 1.17 2002/11/09 09:35:26 hno Exp $
+ * $Id: aiops.cc,v 1.18 2002/11/10 02:29:58 hno Exp $
  *
  * DEBUG: section 43    AIOPS
  * AUTHOR: Stewart Forster <slf@connect.com.au>
@@ -172,7 +172,7 @@ squidaio_get_pool(int size)
     return p;
 }
 
-static void *
+void *
 squidaio_xmalloc(int size)
 {
     void *p;
@@ -198,7 +198,7 @@ squidaio_xstrdup(const char *str)
     return p;
 }
 
-static void
+void
 squidaio_xfree(void *p, int size)
 {
     MemPool *pool;
@@ -503,9 +503,6 @@ squidaio_cleanup_request(squidaio_request_t * requestp)
        squidaio_xstrfree(requestp->path);
        break;
     case _AIO_OP_READ:
-       if (!cancelled && requestp->ret > 0)
-           xmemcpy(requestp->bufferp, requestp->tmpbufp, requestp->ret);
-       squidaio_xfree(requestp->tmpbufp, requestp->buflen);
        break;
     case _AIO_OP_WRITE:
        squidaio_xfree(requestp->tmpbufp, requestp->buflen);
@@ -578,7 +575,6 @@ squidaio_read(int fd, char *bufp, int bufs, off_t offset, int whence, squidaio_r
     requestp = (squidaio_request_t *)memPoolAlloc(squidaio_request_pool);
     requestp->fd = fd;
     requestp->bufferp = bufp;
-    requestp->tmpbufp = (char *) squidaio_xmalloc(bufs);
     requestp->buflen = bufs;
     requestp->offset = offset;
     requestp->whence = whence;
@@ -596,7 +592,7 @@ static void
 squidaio_do_read(squidaio_request_t * requestp)
 {
     lseek(requestp->fd, requestp->offset, requestp->whence);
-    requestp->ret = read(requestp->fd, requestp->tmpbufp, requestp->buflen);
+    requestp->ret = read(requestp->fd, requestp->bufferp, requestp->buflen);
     requestp->err = errno;
 }
 

diff --git a/src/fs/aufs/async_io.cc b/src/fs/aufs/async_io.cc
index 144db3591f08b16073778450cd8e194009d46864..8704362e2fad5963bf09ace7b337fe38f85e1e1c 100644 (file)
--- a/src/fs/aufs/async_io.cc
+++ b/src/fs/aufs/async_io.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: async_io.cc,v 1.16 2002/10/13 20:35:24 robertc Exp $
+ * $Id: async_io.cc,v 1.17 2002/11/10 02:29:58 hno Exp $
  *
  * DEBUG: section 32    Asynchronous Disk I/O
  * AUTHOR: Pete Bentley <pete@demon.net>
@@ -54,26 +54,27 @@ typedef struct squidaio_ctrl_t {
     AIOCB *done_handler;
     void *done_handler_data;
     squidaio_result_t result;
+    int len;
     char *bufp;
     FREE *free_func;
     dlink_node node;
 } squidaio_ctrl_t;
 
 static struct {
-     int open_start;
-     int open_finish;
-     int close_start;
-     int close_finish;
-     int cancel;
-     int write_start;
-     int write_finish;
-     int read_start;
-     int read_finish;
-     int stat_start;
-     int stat_finish;
-     int unlink_start;
-     int unlink_finish;
-     int check_callback;
+    int open_start;
+    int open_finish;
+    int close_start;
+    int close_finish;
+    int cancel;
+    int write_start;
+    int write_finish;
+    int read_start;
+    int read_finish;
+    int stat_start;
+    int stat_finish;
+    int unlink_start;
+    int unlink_finish;
+    int check_callback;
 } squidaio_counts;
 
 typedef struct squidaio_unlinkq_t {
@@ -153,29 +154,35 @@ aioClose(int fd)
 void
 aioCancel(int fd)
 {
-    squidaio_ctrl_t *curr;
+    squidaio_ctrl_t *ctrlp;
     dlink_node *m, *next;
 
     assert(initialised);
     squidaio_counts.cancel++;
     for (m = used_list.head; m; m = next) {
        next = m->next;
-       curr = (squidaio_ctrl_t *)m->data;
-       if (curr->fd != fd)
+       ctrlp = (squidaio_ctrl_t *)m->data;
+       if (ctrlp->fd != fd)
            continue;
 
-       squidaio_cancel(&curr->result);
+       squidaio_cancel(&ctrlp->result);
 
-       if (curr->done_handler) {
-           AIOCB *callback = curr->done_handler;
+       if (ctrlp->done_handler) {
+           AIOCB *callback = ctrlp->done_handler;
            void *cbdata;
-           curr->done_handler = NULL;
+           ctrlp->done_handler = NULL;
            debug(32, 2) ("this be aioCancel\n");
-           if (cbdataReferenceValidDone(curr->done_handler_data, &cbdata))
-               callback(fd, cbdata, -2, -2);
+           if (cbdataReferenceValidDone(ctrlp->done_handler_data, &cbdata))
+               callback(fd, cbdata, NULL, -2, -2);
+           /* free data if requested to aioWrite() */
+           if (ctrlp->free_func)
+               ctrlp->free_func(ctrlp->bufp);
+           /* free temporary read buffer */
+           if (ctrlp->operation == _AIO_READ)
+               squidaio_xfree(ctrlp->bufp, ctrlp->len);
        }
        dlinkDelete(m, &used_list);
-       memPoolFree(squidaio_ctrl_pool, curr);
+       memPoolFree(squidaio_ctrl_pool, ctrlp);
     }
 }
 
@@ -208,7 +215,7 @@ aioWrite(int fd, int offset, char *bufp, int len, AIOCB * callback, void *callba
 
 
 void
-aioRead(int fd, int offset, char *bufp, int len, AIOCB * callback, void *callback_data)
+aioRead(int fd, int offset, int len, AIOCB * callback, void *callback_data)
 {
     squidaio_ctrl_t *ctrlp;
     int seekmode;
@@ -220,6 +227,8 @@ aioRead(int fd, int offset, char *bufp, int len, AIOCB * callback, void *callbac
     ctrlp->done_handler = callback;
     ctrlp->done_handler_data = cbdataReference(callback_data);
     ctrlp->operation = _AIO_READ;
+    ctrlp->len = len;
+    ctrlp->bufp = squidaio_xmalloc(len);
     if (offset >= 0)
        seekmode = SEEK_SET;
     else {
@@ -227,7 +236,7 @@ aioRead(int fd, int offset, char *bufp, int len, AIOCB * callback, void *callbac
        offset = 0;
     }
     ctrlp->result.data = ctrlp;
-    squidaio_read(fd, bufp, len, offset, seekmode, &ctrlp->result);
+    squidaio_read(fd, ctrlp->bufp, len, offset, seekmode, &ctrlp->result);
     dlinkAdd(ctrlp, &ctrlp->node, &used_list);
     return;
 }                              /* aioRead */
@@ -296,29 +305,29 @@ aioCheckCallbacks(SwapDir * SD)
        if ((resultp = squidaio_poll_done()) == NULL)
            break;
        ctrlp = (squidaio_ctrl_t *) resultp->data;
-       switch (resultp->result_type){
-           case _AIO_OP_NONE:
-           case _AIO_OP_TRUNCATE:
-           case _AIO_OP_OPENDIR:
-                break;
-           case _AIO_OP_OPEN:
-                ++squidaio_counts.open_finish;
-                break;
-           case _AIO_OP_READ:
-                ++squidaio_counts.read_finish;
-                break;
-           case _AIO_OP_WRITE:
-                ++squidaio_counts.write_finish;
-                break;
-           case _AIO_OP_CLOSE:
-                ++squidaio_counts.close_finish;
-                break;
-           case _AIO_OP_UNLINK:
-                ++squidaio_counts.unlink_finish;
-                break;
-           case _AIO_OP_STAT:
-                ++squidaio_counts.stat_finish;
-                break;
+       switch (resultp->result_type) {
+       case _AIO_OP_NONE:
+       case _AIO_OP_TRUNCATE:
+       case _AIO_OP_OPENDIR:
+           break;
+       case _AIO_OP_OPEN:
+           ++squidaio_counts.open_finish;
+           break;
+       case _AIO_OP_READ:
+           ++squidaio_counts.read_finish;
+           break;
+       case _AIO_OP_WRITE:
+           ++squidaio_counts.write_finish;
+           break;
+       case _AIO_OP_CLOSE:
+           ++squidaio_counts.close_finish;
+           break;
+       case _AIO_OP_UNLINK:
+           ++squidaio_counts.unlink_finish;
+           break;
+       case _AIO_OP_STAT:
+           ++squidaio_counts.stat_finish;
+           break;
        }
        if (ctrlp == NULL)
            continue;           /* XXX Should not happen */
@@ -329,13 +338,23 @@ aioCheckCallbacks(SwapDir * SD)
            ctrlp->done_handler = NULL;
            if (cbdataReferenceValidDone(ctrlp->done_handler_data, &cbdata)) {
                retval = 1;     /* Return that we've actually done some work */
-               callback(ctrlp->fd, cbdata,
+               callback(ctrlp->fd, cbdata, ctrlp->bufp,
                    ctrlp->result.aio_return, ctrlp->result.aio_errno);
+           } else {
+               if (ctrlp->operation == _AIO_OPEN) {
+                   /* The open operation was aborted.. */
+                   int fd = ctrlp->result.aio_return;
+                   if (fd >= 0)
+                       aioClose(fd);
+               }
            }
        }
        /* free data if requested to aioWrite() */
        if (ctrlp->free_func)
            ctrlp->free_func(ctrlp->bufp);
+       /* free temporary read buffer */
+       if (ctrlp->operation == _AIO_READ)
+           squidaio_xfree(ctrlp->bufp, ctrlp->len);
        if (ctrlp->operation == _AIO_CLOSE)
            aioFDWasClosed(ctrlp->fd);
        memPoolFree(squidaio_ctrl_pool, ctrlp);
@@ -348,13 +367,13 @@ aioStats(StoreEntry * sentry)
 {
     storeAppendPrintf(sentry, "ASYNC IO Counters:\n");
     storeAppendPrintf(sentry, "Operation\t# Requests\tNumber serviced\n");
-    storeAppendPrintf(sentry, "open\t%d\t%d\n", squidaio_counts.open_start,squidaio_counts.open_finish);
-    storeAppendPrintf(sentry, "close\t%d\t%d\n", squidaio_counts.close_start,squidaio_counts.close_finish);
+    storeAppendPrintf(sentry, "open\t%d\t%d\n", squidaio_counts.open_start, squidaio_counts.open_finish);
+    storeAppendPrintf(sentry, "close\t%d\t%d\n", squidaio_counts.close_start, squidaio_counts.close_finish);
     storeAppendPrintf(sentry, "cancel\t%d\t-\n", squidaio_counts.cancel);
-    storeAppendPrintf(sentry, "write\t%d\t%d\n", squidaio_counts.write_start,squidaio_counts.write_finish);
-    storeAppendPrintf(sentry, "read\t%d\t%d\n", squidaio_counts.read_start,squidaio_counts.read_finish);
-    storeAppendPrintf(sentry, "stat\t%d\t%d\n", squidaio_counts.stat_start,squidaio_counts.stat_finish);
-    storeAppendPrintf(sentry, "unlink\t%d\t%d\n", squidaio_counts.unlink_start,squidaio_counts.unlink_finish);
+    storeAppendPrintf(sentry, "write\t%d\t%d\n", squidaio_counts.write_start, squidaio_counts.write_finish);
+    storeAppendPrintf(sentry, "read\t%d\t%d\n", squidaio_counts.read_start, squidaio_counts.read_finish);
+    storeAppendPrintf(sentry, "stat\t%d\t%d\n", squidaio_counts.stat_start, squidaio_counts.stat_finish);
+    storeAppendPrintf(sentry, "unlink\t%d\t%d\n", squidaio_counts.unlink_start, squidaio_counts.unlink_finish);
     storeAppendPrintf(sentry, "check_callback\t%d\t-\n", squidaio_counts.check_callback);
     storeAppendPrintf(sentry, "queue\t%d\t-\n", squidaio_get_queue_len());
 }

diff --git a/src/fs/aufs/store_asyncufs.h b/src/fs/aufs/store_asyncufs.h
index 664240a10be1831bfac8ac40a44b88b46fe704fb..f8c61349221ad2cca971435345208274fc81c82a 100644 (file)
--- a/src/fs/aufs/store_asyncufs.h
+++ b/src/fs/aufs/store_asyncufs.h
@@ -48,7 +48,7 @@ struct _squidaio_result_t {
 
 typedef struct _squidaio_result_t squidaio_result_t;
 
-typedef void AIOCB(int fd, void *, int aio_return, int aio_errno);
+typedef void AIOCB(int fd, void *cbdata, const char *buf, int aio_return, int aio_errno);
 
 int squidaio_cancel(squidaio_result_t *);
 int squidaio_open(const char *, int, mode_t, squidaio_result_t *);
@@ -63,6 +63,8 @@ squidaio_result_t *squidaio_poll_done(void);
 int squidaio_operations_pending(void);
 int squidaio_sync(void);
 int squidaio_get_queue_len(void);
+void *squidaio_xmalloc(int size);
+void squidaio_xfree(void *p, int size);
 
 void aioInit(void);
 void aioDone(void);
@@ -70,7 +72,7 @@ void aioCancel(int);
 void aioOpen(const char *, int, mode_t, AIOCB *, void *);
 void aioClose(int);
 void aioWrite(int, int offset, char *, int size, AIOCB *, void *, FREE *);
-void aioRead(int, int offset, char *, int size, AIOCB *, void *);
+void aioRead(int, int offset, int size, AIOCB *, void *);
 void aioStat(char *, struct stat *, AIOCB *, void *);
 void aioUnlink(const char *, AIOCB *, void *);
 void aioTruncate(const char *, off_t length, AIOCB *, void *);
@@ -89,7 +91,7 @@ struct _squidaiostate_t {
        unsigned int read_kicking:1;
        unsigned int inreaddone:1;
     } flags;
-    const char *read_buf;
+    char *read_buf;
     link_list *pending_writes;
     link_list *pending_reads;
 };

diff --git a/src/fs/aufs/store_dir_aufs.cc b/src/fs/aufs/store_dir_aufs.cc
index 0b8809383102b598296b84349056864787a50b5e..e0b89e6c9d60170e122434666ba482bb5ae97f23 100644 (file)
--- a/src/fs/aufs/store_dir_aufs.cc
+++ b/src/fs/aufs/store_dir_aufs.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: store_dir_aufs.cc,v 1.51 2002/10/13 20:35:24 robertc Exp $
+ * $Id: store_dir_aufs.cc,v 1.52 2002/11/10 02:29:58 hno Exp $
  *
  * DEBUG: section 47    Store Directory Routines
  * AUTHOR: Duane Wessels
@@ -145,7 +145,7 @@ storeAufsDirReconfigure(SwapDir * sd, int index, char *path)
 void
 storeAufsDirDump(StoreEntry * entry, SwapDir * s)
 {
-    commonUfsDirDump (entry, s);
+    commonUfsDirDump(entry, s);
     dump_cachedir_options(entry, options, s);
 }
 

diff --git a/src/fs/aufs/store_io_aufs.cc b/src/fs/aufs/store_io_aufs.cc
index 13190d4dc0bef83ca7ed8f651c146b52cc7cb4d3..9177576db84de20156e9b2319f98f2ba0d488cca 100644 (file)
--- a/src/fs/aufs/store_io_aufs.cc
+++ b/src/fs/aufs/store_io_aufs.cc
@@ -20,7 +20,7 @@ static DWCB storeAufsWriteDone;
 #endif
 static void storeAufsIOCallback(storeIOState * sio, int errflag);
 static AIOCB storeAufsOpenDone;
-static int storeAufsSomethingPending(storeIOState *);
+static int storeAufsNeedCompletetion(storeIOState *);
 static int storeAufsKickWriteQueue(storeIOState * sio);
 static CBDUNL storeAufsIOFreeEntry;
 
@@ -141,7 +141,7 @@ storeAufsClose(SwapDir * SD, storeIOState * sio)
     squidaiostate_t *aiostate = (squidaiostate_t *) sio->fsstate;
     debug(79, 3) ("storeAufsClose: dirno %d, fileno %08X, FD %d\n",
        sio->swap_dirn, sio->swap_filen, aiostate->fd);
-    if (storeAufsSomethingPending(sio)) {
+    if (storeAufsNeedCompletetion(sio)) {
        aiostate->flags.close_request = 1;
        return;
     }
@@ -167,7 +167,7 @@ storeAufsRead(SwapDir * SD, storeIOState * sio, char *buf, size_t size, off_t of
        q->size = size;
        q->offset = offset;
        q->callback = callback;
-       q->callback_data = callback_data;
+       q->callback_data = cbdataReference(callback_data);
        linklistPush(&(aiostate->pending_reads), q);
        return;
     }
@@ -179,7 +179,7 @@ storeAufsRead(SwapDir * SD, storeIOState * sio, char *buf, size_t size, off_t of
     sio->offset = offset;
     aiostate->flags.reading = 1;
 #if ASYNC_READ
-    aioRead(aiostate->fd, offset, buf, size, storeAufsReadDone, sio);
+    aioRead(aiostate->fd, offset, size, storeAufsReadDone, sio);
 #else
     file_read(aiostate->fd, buf, size, offset, storeAufsReadDone, sio);
 #endif
@@ -257,17 +257,19 @@ storeAufsKickReadQueue(storeIOState * sio)
 {
     squidaiostate_t *aiostate = (squidaiostate_t *) sio->fsstate;
     struct _queued_read *q = (struct _queued_read *)linklistShift(&(aiostate->pending_reads));
+    void *cbdata;
     if (NULL == q)
        return 0;
     debug(79, 3) ("storeAufsKickReadQueue: reading queued request of %ld bytes\n",
        (long int) q->size);
-    storeAufsRead(INDEXSD(sio->swap_dirn), sio, q->buf, q->size, q->offset, q->callback, q->callback_data);
+    if (cbdataReferenceValidDone(q->callback_data, &cbdata))
+       storeAufsRead(INDEXSD(sio->swap_dirn), sio, q->buf, q->size, q->offset, q->callback, cbdata);
     memPoolFree(aufs_qread_pool, q);
     return 1;
 }
 
 static void
-storeAufsOpenDone(int unused, void *my_data, int fd, int errflag)
+storeAufsOpenDone(int unused, void *my_data, const char *unused2, int fd, int errflag)
 {
     storeIOState *sio = (storeIOState *)my_data;
     squidaiostate_t *aiostate = (squidaiostate_t *) sio->fsstate;
@@ -288,7 +290,7 @@ storeAufsOpenDone(int unused, void *my_data, int fd, int errflag)
     if (FILE_MODE(sio->mode) == O_WRONLY) {
        if (storeAufsKickWriteQueue(sio))
            return;
-    } else if (FILE_MODE(sio->mode) == O_RDONLY) {
+    } else if ((FILE_MODE(sio->mode) == O_RDONLY) && !aiostate->flags.close_request) {
        if (storeAufsKickReadQueue(sio))
            return;
     }
@@ -299,7 +301,7 @@ storeAufsOpenDone(int unused, void *my_data, int fd, int errflag)
 
 #if ASYNC_READ
 static void
-storeAufsReadDone(int fd, void *my_data, int len, int errflag)
+storeAufsReadDone(int fd, void *my_data, const char *buf, int len, int errflag)
 #else
 static void
 storeAufsReadDone(int fd, const char *buf, int len, int errflag, void *my_data)
@@ -310,6 +312,7 @@ storeAufsReadDone(int fd, const char *buf, int len, int errflag, void *my_data)
     STRCB *callback = sio->read.callback;
     void *cbdata;
     ssize_t rlen;
+    int inreaddone = aiostate->flags.inreaddone;       /* Protect from callback loops */
     debug(79, 3) ("storeAufsReadDone: dirno %d, fileno %08X, FD %d, len %d\n",
        sio->swap_dirn, sio->swap_filen, fd, len);
     aiostate->flags.inreaddone = 1;
@@ -334,10 +337,15 @@ storeAufsReadDone(int fd, const char *buf, int len, int errflag, void *my_data)
 #endif
     assert(callback);
     sio->read.callback = NULL;
-    if (cbdataReferenceValidDone(sio->read.callback_data, &cbdata))
+    if (!aiostate->flags.close_request && cbdataReferenceValidDone(sio->read.callback_data, &cbdata)) {
+#if ASYNC_READ
+       if (rlen > 0)
+           memcpy(aiostate->read_buf, buf, rlen);
+#endif
        callback(cbdata, aiostate->read_buf, rlen);
+    }
     aiostate->flags.inreaddone = 0;
-    if (aiostate->flags.close_request)
+    if (aiostate->flags.close_request && !inreaddone)
        storeAufsIOCallback(sio, errflag);
 }
 
@@ -396,38 +404,39 @@ storeAufsIOCallback(storeIOState * sio, int errflag)
     squidaiostate_t *aiostate = (squidaiostate_t *) sio->fsstate;
     int fd = aiostate->fd;
     debug(79, 3) ("storeAufsIOCallback: errflag=%d\n", errflag);
-    debug(79, 3) ("%s:%d\n", __FILE__, __LINE__);
+    debug(79, 9) ("%s:%d\n", __FILE__, __LINE__);
     if (callback) {
        void *cbdata;
        sio->callback = NULL;
        if (cbdataReferenceValidDone(sio->callback_data, &cbdata))
            callback(cbdata, errflag, sio);
     }
-    debug(79, 3) ("%s:%d\n", __FILE__, __LINE__);
+    debug(79, 9) ("%s:%d\n", __FILE__, __LINE__);
     aiostate->fd = -1;
     cbdataFree(sio);
     if (fd < 0)
        return;
-    debug(79, 3) ("%s:%d\n", __FILE__, __LINE__);
+    debug(79, 9) ("%s:%d\n", __FILE__, __LINE__);
     aioClose(fd);
     fd_close(fd);
     store_open_disk_fd--;
-    debug(79, 3) ("%s:%d\n", __FILE__, __LINE__);
+    debug(79, 9) ("%s:%d\n", __FILE__, __LINE__);
 }
 
 
 static int
-storeAufsSomethingPending(storeIOState * sio)
+storeAufsNeedCompletetion(storeIOState * sio)
 {
     squidaiostate_t *aiostate = (squidaiostate_t *) sio->fsstate;
-    if (aiostate->flags.reading)
-       return 1;
+
     if (aiostate->flags.writing)
        return 1;
-    if (aiostate->flags.opening)
+    if (aiostate->flags.opening && FILE_MODE(sio->mode) == O_WRONLY)
        return 1;
     if (aiostate->flags.inreaddone)
        return 1;
+
+    /* Note: Pending read operations are silently cancelled on close */
     return 0;
 }
 
@@ -438,7 +447,24 @@ storeAufsSomethingPending(storeIOState * sio)
  * to bother with that.
  */
 static void
-storeAufsIOFreeEntry(void *sio)
+storeAufsIOFreeEntry(void *siop)
 {
-    memPoolFree(squidaio_state_pool, ((storeIOState *) sio)->fsstate);
+    storeIOState *sio = (storeIOState *) siop;
+    squidaiostate_t *aiostate = (squidaiostate_t *) sio->fsstate;
+    struct _queued_write *qw;
+    struct _queued_read *qr;
+    while ((qw = linklistShift(&aiostate->pending_writes))) {
+       if (qw->free_func)
+           qw->free_func(qw->buf);
+       memPoolFree(aufs_qwrite_pool, qw);
+    }
+    while ((qr = linklistShift(&aiostate->pending_reads))) {
+       cbdataReferenceDone(qr->callback_data);
+       memPoolFree(aufs_qread_pool, qr);
+    }
+    if (sio->read.callback_data)
+       cbdataReferenceDone(sio->read.callback_data);
+    if (sio->callback_data)
+       cbdataReferenceDone(sio->callback_data);
+    memPoolFree(squidaio_state_pool, aiostate);
 }