Fix data corruption bug

Received data from the client was written to the read-ahead buffer
from dsi_peek() which caused data corrution.
Fix is: change the DSI command buffer from static 8192 bytes to
an allocated buffer of size DSI quantum and use this buffer in
dsi_write/write_fork. That just requires dsi_writeinit to use
memmove instead of memcpy, because now we use the same buffer.

diff --git a/NEWS b/NEWS
index c64082b787176d5ce1cd549591e0827def2f9672..e3b577b0c10e69908dde252a366269d39f3b42ac 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,8 @@ Changes in 3.0.1
 * FIX: afpd: Fixes open file handle refcounting bug which was reported as
        being unable to play movies off a Netatalk AFP share.
        Bug ID 3559783.
 * FIX: afpd: Fixes open file handle refcounting bug which was reported as
        being unable to play movies off a Netatalk AFP share.
        Bug ID 3559783.

+* FIX: afpd: Fix a possible data corruption when reading from and writing
+       to the server simultaniously under load

 * FIX: Fix possible alignment violations due to bad casts
 * FIX: dbd: Fix logging
 * FIX: apple_dump: Extended Attributes AppleDouble support for *BSD
 * FIX: Fix possible alignment violations due to bad casts
 * FIX: dbd: Fix logging
 * FIX: apple_dump: Extended Attributes AppleDouble support for *BSD

diff --git a/etc/afpd/afp_dsi.c b/etc/afpd/afp_dsi.c
index a841860d128616f6c9963fd99ece47415e43a34d..3e5cd47c3cbf4de4438bc6e3e0e2585a9be72417 100644 (file)
--- a/etc/afpd/afp_dsi.c
+++ b/etc/afpd/afp_dsi.c
@@ -626,7 +626,7 @@ void afp_over_dsi(AFPObj *obj)
                     LOG(log_debug, logtype_afpd, "<== Start AFP command: %s", AfpNum2name(function));
 
                     err = (*afp_switch[function])(obj,
                     LOG(log_debug, logtype_afpd, "<== Start AFP command: %s", AfpNum2name(function));
 
                     err = (*afp_switch[function])(obj,

-                                                  (char *)&dsi->commands, dsi->cmdlen,
+                                                  dsi->commands, dsi->cmdlen,

                                                   (char *)&dsi->data, &dsi->datalen);
 
                     LOG(log_debug, logtype_afpd, "==> Finished AFP command: %s -> %s",
                                                   (char *)&dsi->data, &dsi->datalen);
 
                     LOG(log_debug, logtype_afpd, "==> Finished AFP command: %s -> %s",


@@ -667,7 +667,7 @@ void afp_over_dsi(AFPObj *obj)
                 LOG(log_debug, logtype_afpd, "<== Start AFP command: %s", AfpNum2name(function));
 
                 err = (*afp_switch[function])(obj,
                 LOG(log_debug, logtype_afpd, "<== Start AFP command: %s", AfpNum2name(function));
 
                 err = (*afp_switch[function])(obj,

-                                              (char *)&dsi->commands, dsi->cmdlen,
+                                              dsi->commands, dsi->cmdlen,

                                               (char *)&dsi->data, &dsi->datalen);
 
                 LOG(log_debug, logtype_afpd, "==> Finished AFP command: %s -> %s",
                                               (char *)&dsi->data, &dsi->datalen);
 
                 LOG(log_debug, logtype_afpd, "==> Finished AFP command: %s -> %s",

diff --git a/etc/afpd/fork.c b/etc/afpd/fork.c
index cd1580fa43005552880432d968571b7fe3b34248..9c3ed7d2f1be64ce11c14b5814850195ca6c7742 100644 (file)
--- a/etc/afpd/fork.c
+++ b/etc/afpd/fork.c
@@ -1090,8 +1090,8 @@ static int write_fork(AFPObj *obj, char *ibuf, size_t ibuflen _U_, char *rbuf, s
     uint16_t        ofrefnum;
     ssize_t         cc;
     DSI             *dsi = obj->dsi;
     uint16_t        ofrefnum;
     ssize_t         cc;
     DSI             *dsi = obj->dsi;

-    char            *rcvbuf = dsi->buffer;
-    size_t          rcvbuflen = dsi->dsireadbuf * dsi->server_quantum;
+    char            *rcvbuf = dsi->commands;
+    size_t          rcvbuflen = dsi->server_quantum;

 
     /* figure out parameters */
     ibuf++;
 
     /* figure out parameters */
     ibuf++;


@@ -1233,8 +1233,8 @@ static int write_fork(AFPObj *obj, char *ibuf, size_t ibuflen _U_, char *rbuf, s
     return( AFP_OK );
 
 afp_write_err:
     return( AFP_OK );
 
 afp_write_err:

-    dsi_writeinit(obj->dsi, rbuf, *rbuflen);
-    dsi_writeflush(obj->dsi);
+    dsi_writeinit(dsi, rcvbuf, rcvbuflen);
+    dsi_writeflush(dsi);

 
     if (err != AFP_OK) {
         *rbuflen = 0;
 
     if (err != AFP_OK) {
         *rbuflen = 0;

diff --git a/include/atalk/dsi.h b/include/atalk/dsi.h
index bd8cc8b4e50ec700970fb05bcefa5f6267810cbe..0e776c31cca07667a43926f5ad96d493163a25ba 100644 (file)
--- a/include/atalk/dsi.h
+++ b/include/atalk/dsi.h
@@ -45,12 +45,14 @@ struct dsi_block {
     uint8_t dsi_flags;       /* packet type: request or reply */
     uint8_t dsi_command;     /* command */
     uint16_t dsi_requestID;  /* request ID */
     uint8_t dsi_flags;       /* packet type: request or reply */
     uint8_t dsi_command;     /* command */
     uint16_t dsi_requestID;  /* request ID */

-    uint32_t dsi_code;       /* error code or data offset */
+    union {
+        uint32_t dsi_code;   /* error code */
+        uint32_t dsi_doff;   /* data offset */
+    };

     uint32_t dsi_len;        /* total data length */
     uint32_t dsi_reserved;   /* reserved field */
 };
 
     uint32_t dsi_len;        /* total data length */
     uint32_t dsi_reserved;   /* reserved field */
 };
 

-#define DSI_CMDSIZ        8192

 #define DSI_DATASIZ       8192
 
 /* child and parent processes might interpret a couple of these
 #define DSI_DATASIZ       8192
 
 /* child and parent processes might interpret a couple of these


@@ -72,7 +74,8 @@ typedef struct DSI {
 
     uint32_t attn_quantum, datasize, server_quantum;
     uint16_t serverID, clientID;
 
     uint32_t attn_quantum, datasize, server_quantum;
     uint16_t serverID, clientID;

-    uint8_t  commands[DSI_CMDSIZ], data[DSI_DATASIZ];
+    uint8_t  *commands; /* DSI recieve buffer */
+    uint8_t  data[DSI_DATASIZ];    /* DSI reply buffer */

     size_t   datalen, cmdlen;
     off_t    read_count, write_count;
     uint32_t flags;             /* DSI flags like DSI_SLEEPING, DSI_DISCONNECTED */
     size_t   datalen, cmdlen;
     off_t    read_count, write_count;
     uint32_t flags;             /* DSI flags like DSI_SLEEPING, DSI_DISCONNECTED */

diff --git a/libatalk/dsi/dsi_opensess.c b/libatalk/dsi/dsi_opensess.c
index d01e64c42fb017c2b6ec94eab1283ff8d7ef22d6..a2dcb4c9b0d6c11177eca4cd0eea24344a63163f 100644 (file)
--- a/libatalk/dsi/dsi_opensess.c
+++ b/libatalk/dsi/dsi_opensess.c
@@ -16,25 +16,12 @@
 #include <atalk/util.h>
 #include <atalk/logger.h>
 
 #include <atalk/util.h>
 #include <atalk/logger.h>
 

-static void dsi_init_buffer(DSI *dsi)
-{
-    /* default is 12 * 300k = 3,6 MB (Apr 2011) */
-    if ((dsi->buffer = malloc(dsi->dsireadbuf * dsi->server_quantum)) == NULL) {
-        LOG(log_error, logtype_dsi, "dsi_init_buffer: OOM");
-        AFP_PANIC("OOM in dsi_init_buffer");
-    }
-    dsi->start = dsi->buffer;
-    dsi->eof = dsi->buffer;
-    dsi->end = dsi->buffer + (dsi->dsireadbuf * dsi->server_quantum);
-}
-

 /* OpenSession. set up the connection */
 void dsi_opensession(DSI *dsi)
 {
   uint32_t i = 0; /* this serves double duty. it must be 4-bytes long */
   int offs;
 
 /* OpenSession. set up the connection */
 void dsi_opensession(DSI *dsi)
 {
   uint32_t i = 0; /* this serves double duty. it must be 4-bytes long */
   int offs;
 

-  dsi_init_buffer(dsi);

   if (setnonblock(dsi->socket, 1) < 0) {
       LOG(log_error, logtype_dsi, "dsi_opensession: setnonblock: %s", strerror(errno));
       AFP_PANIC("setnonblock error");
   if (setnonblock(dsi->socket, 1) < 0) {
       LOG(log_error, logtype_dsi, "dsi_opensession: setnonblock: %s", strerror(errno));
       AFP_PANIC("setnonblock error");

diff --git a/libatalk/dsi/dsi_stream.c b/libatalk/dsi/dsi_stream.c
index 154297748fbd235ce5ffb325d4f7e356d4c67156..711a037b5098925c59c77198bb6350d390d9b5a3 100644 (file)
--- a/libatalk/dsi/dsi_stream.c
+++ b/libatalk/dsi/dsi_stream.c
@@ -596,7 +596,7 @@ int dsi_stream_receive(DSI *dsi)
   dsi->clientID = ntohs(dsi->header.dsi_requestID);
   
   /* make sure we don't over-write our buffers. */
   dsi->clientID = ntohs(dsi->header.dsi_requestID);
   
   /* make sure we don't over-write our buffers. */

-  dsi->cmdlen = MIN(ntohl(dsi->header.dsi_len), DSI_CMDSIZ);
+  dsi->cmdlen = MIN(ntohl(dsi->header.dsi_len), dsi->server_quantum);

   if (dsi_stream_read(dsi, dsi->commands, dsi->cmdlen) != dsi->cmdlen) 
     return 0;
 
   if (dsi_stream_read(dsi, dsi->commands, dsi->cmdlen) != dsi->cmdlen) 
     return 0;
 

diff --git a/libatalk/dsi/dsi_tcp.c b/libatalk/dsi/dsi_tcp.c
index 2c16323e2d607884e60ceff2dc6371b3f0a92a7b..e06e87d92420187948611b9d044995ab158065c6 100644 (file)
--- a/libatalk/dsi/dsi_tcp.c
+++ b/libatalk/dsi/dsi_tcp.c
@@ -83,6 +83,26 @@ static void timeout_handler(int sig _U_)
     exit(EXITERR_CLNT);
 }
 
     exit(EXITERR_CLNT);
 }
 

+/*!
+ * Allocate DSI read buffer and read-ahead buffer
+ */
+static void dsi_init_buffer(DSI *dsi)
+{
+    if ((dsi->commands = malloc(dsi->server_quantum)) == NULL) {
+        LOG(log_error, logtype_dsi, "dsi_init_buffer: OOM");
+        AFP_PANIC("OOM in dsi_init_buffer");
+    }
+
+    /* dsi_peek() read ahead buffer, default is 12 * 300k = 3,6 MB (Apr 2011) */
+    if ((dsi->buffer = malloc(dsi->dsireadbuf * dsi->server_quantum)) == NULL) {
+        LOG(log_error, logtype_dsi, "dsi_init_buffer: OOM");
+        AFP_PANIC("OOM in dsi_init_buffer");
+    }
+    dsi->start = dsi->buffer;
+    dsi->eof = dsi->buffer;
+    dsi->end = dsi->buffer + (dsi->dsireadbuf * dsi->server_quantum);
+}
+

 static struct itimerval itimer;
 /* accept the socket and do a little sanity checking */
 static int dsi_tcp_open(DSI *dsi)
 static struct itimerval itimer;
 /* accept the socket and do a little sanity checking */
 static int dsi_tcp_open(DSI *dsi)


@@ -136,6 +156,8 @@ static int dsi_tcp_open(DSI *dsi)
         }
 #endif
 
         }
 #endif
 

+        dsi_init_buffer(dsi);
+

         /* read in commands. this is similar to dsi_receive except
          * for the fact that we do some sanity checking to prevent
          * delinquent connections from causing mischief. */
         /* read in commands. this is similar to dsi_receive except
          * for the fact that we do some sanity checking to prevent
          * delinquent connections from causing mischief. */


@@ -174,7 +196,7 @@ static int dsi_tcp_open(DSI *dsi)
         dsi->clientID = ntohs(dsi->header.dsi_requestID);
 
         /* make sure we don't over-write our buffers. */
         dsi->clientID = ntohs(dsi->header.dsi_requestID);
 
         /* make sure we don't over-write our buffers. */

-        dsi->cmdlen = min(ntohl(dsi->header.dsi_len), DSI_CMDSIZ);
+        dsi->cmdlen = min(ntohl(dsi->header.dsi_len), dsi->server_quantum);

 
         stored = 0;
         while (stored < dsi->cmdlen) {
 
         stored = 0;
         while (stored < dsi->cmdlen) {

diff --git a/libatalk/dsi/dsi_write.c b/libatalk/dsi/dsi_write.c
index 13a723470153895766530e65235d3a2b00c6070d..1cf25e64c6516ea65e4144f63eafcd625fdb72d2 100644 (file)
--- a/libatalk/dsi/dsi_write.c
+++ b/libatalk/dsi/dsi_write.c
@@ -33,14 +33,14 @@ size_t dsi_writeinit(DSI *dsi, void *buf, const size_t buflen _U_)
 
   /* figure out how much data we have. do a couple checks for 0 
    * data */
 
   /* figure out how much data we have. do a couple checks for 0 
    * data */

-  header = ntohl(dsi->header.dsi_code);
+  header = ntohl(dsi->header.dsi_doff);

   dsi->datasize = header ? ntohl(dsi->header.dsi_len) - header : 0;
 
   if (dsi->datasize > 0) {
   dsi->datasize = header ? ntohl(dsi->header.dsi_len) - header : 0;
 
   if (dsi->datasize > 0) {

-      len = MIN(sizeof(dsi->commands) - header, dsi->datasize);
+      len = MIN(dsi->server_quantum - header, dsi->datasize);

 
       /* write last part of command buffer into buf */
 
       /* write last part of command buffer into buf */

-      memcpy(buf, dsi->commands + header, len);
+      memmove(buf, dsi->commands + header, len);

 
       /* recalculate remaining data */
       dsi->datasize -= len;
 
       /* recalculate remaining data */
       dsi->datasize -= len;