From: Frank Lahm Date: Thu, 23 Aug 2012 10:20:00 +0000 (+0200) Subject: Fix data corruption bug X-Git-Url: https://arthur.barton.de/cgi-bin/gitweb.cgi?p=netatalk.git;a=commitdiff_plain;h=8e5e83dac34cf886996821a51dee5c971e7c51f2 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 c64082b7..e3b577b0 100644 --- 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: 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 diff --git a/etc/afpd/afp_dsi.c b/etc/afpd/afp_dsi.c index a841860d..3e5cd47c 100644 --- 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, - (char *)&dsi->commands, dsi->cmdlen, + dsi->commands, dsi->cmdlen, (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, - (char *)&dsi->commands, dsi->cmdlen, + dsi->commands, dsi->cmdlen, (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 cd1580fa..9c3ed7d2 100644 --- 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; - 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++; @@ -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: - dsi_writeinit(obj->dsi, rbuf, *rbuflen); - dsi_writeflush(obj->dsi); + dsi_writeinit(dsi, rcvbuf, rcvbuflen); + dsi_writeflush(dsi); if (err != AFP_OK) { *rbuflen = 0; diff --git a/include/atalk/dsi.h b/include/atalk/dsi.h index bd8cc8b4..0e776c31 100644 --- 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 */ - 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 */ }; -#define DSI_CMDSIZ 8192 #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; - 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 */ diff --git a/libatalk/dsi/dsi_opensess.c b/libatalk/dsi/dsi_opensess.c index d01e64c4..a2dcb4c9 100644 --- a/libatalk/dsi/dsi_opensess.c +++ b/libatalk/dsi/dsi_opensess.c @@ -16,25 +16,12 @@ #include #include -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; - 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"); diff --git a/libatalk/dsi/dsi_stream.c b/libatalk/dsi/dsi_stream.c index 15429774..711a037b 100644 --- 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->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; diff --git a/libatalk/dsi/dsi_tcp.c b/libatalk/dsi/dsi_tcp.c index 2c16323e..e06e87d9 100644 --- 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); } +/*! + * 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) @@ -136,6 +156,8 @@ static int dsi_tcp_open(DSI *dsi) } #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. */ @@ -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->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) { diff --git a/libatalk/dsi/dsi_write.c b/libatalk/dsi/dsi_write.c index 13a72347..1cf25e64 100644 --- 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 */ - 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) { - len = MIN(sizeof(dsi->commands) - header, dsi->datasize); + len = MIN(dsi->server_quantum - header, dsi->datasize); /* 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;