From 636b3fc3785facedc6cdd706da6060a4c834c3b8 Mon Sep 17 00:00:00 2001 From: David Crocker Date: Fri, 21 Apr 2017 15:14:50 +0100 Subject: [PATCH] Bug fixes to 1.19 alpha Fixed buffer overflow in OutputBuffer::cat when the input string is longer than the remaining size in the buffer plus one new buffer Fixed issue with sending output buffers totlaling more then 2Kb to the Ethernet interface on the Duet Ethernet --- src/DuetNG/DuetEthernet/NetworkResponder.cpp | 2 +- src/DuetNG/DuetEthernet/Socket.cpp | 8 +- .../Wiznet/Ethernet/W5500/w5500.cpp | 3 - src/OutputMemory.cpp | 142 ++++++------------ src/OutputMemory.h | 1 + src/Version.h | 4 +- 6 files changed, 55 insertions(+), 105 deletions(-) diff --git a/src/DuetNG/DuetEthernet/NetworkResponder.cpp b/src/DuetNG/DuetEthernet/NetworkResponder.cpp index c67898b..92c8f3e 100644 --- a/src/DuetNG/DuetEthernet/NetworkResponder.cpp +++ b/src/DuetNG/DuetEthernet/NetworkResponder.cpp @@ -78,7 +78,7 @@ void NetworkResponder::SendData() } else { - const size_t sent = skt->Send(reinterpret_cast(outBuf->Data()), bytesLeft); + const size_t sent = skt->Send(reinterpret_cast(outBuf->UnreadData()), bytesLeft); if (sent == 0) { // Check whether the connection has been closed diff --git a/src/DuetNG/DuetEthernet/Socket.cpp b/src/DuetNG/DuetEthernet/Socket.cpp index a4743a4..403d5ea 100644 --- a/src/DuetNG/DuetEthernet/Socket.cpp +++ b/src/DuetNG/DuetEthernet/Socket.cpp @@ -286,16 +286,13 @@ size_t Socket::Send(const uint8_t *data, size_t length) } wiz_send_data_at(socketNum, data, length, wizTxBufferPtr); wizTxBufferLeft -= length; + wizTxBufferPtr += length; if (wizTxBufferLeft == 0) { - // Buffer is full so send it - ExecCommand(socketNum, Sn_CR_SEND); - isSending = true; - sendOutstanding = false; + Send(); } else { - wizTxBufferPtr += length; sendOutstanding = true; } return length; @@ -307,6 +304,7 @@ void Socket::Send() { if (CanSend() && sendOutstanding) { + setSn_TX_WR(socketNum, wizTxBufferPtr); ExecCommand(socketNum, Sn_CR_SEND); isSending = true; sendOutstanding = false; diff --git a/src/DuetNG/DuetEthernet/Wiznet/Ethernet/W5500/w5500.cpp b/src/DuetNG/DuetEthernet/Wiznet/Ethernet/W5500/w5500.cpp index 9c7b415..c106164 100644 --- a/src/DuetNG/DuetEthernet/Wiznet/Ethernet/W5500/w5500.cpp +++ b/src/DuetNG/DuetEthernet/Wiznet/Ethernet/W5500/w5500.cpp @@ -150,9 +150,6 @@ void wiz_send_data_at(uint8_t sn, const uint8_t *wizdata, uint16_t len, uint16_t { const uint32_t addrsel = ((uint32_t)ptr << 8) + (WIZCHIP_TXBUF_BLOCK(sn) << 3); WIZCHIP_WRITE_BUF(addrsel, wizdata, len); - - ptr += len; - setSn_TX_WR(sn,ptr); } } diff --git a/src/OutputMemory.cpp b/src/OutputMemory.cpp index d825eca..139d868 100644 --- a/src/OutputMemory.cpp +++ b/src/OutputMemory.cpp @@ -114,6 +114,7 @@ size_t OutputBuffer::catf(const char *fmt, ...) vsnprintf(formatBuffer, ARRAY_SIZE(formatBuffer), fmt, vargs); va_end(vargs); + formatBuffer[ARRAY_UPB(formatBuffer)] = 0; return cat(formatBuffer); } @@ -148,54 +149,8 @@ size_t OutputBuffer::copy(const char *src, size_t len) last = this; } - // Does the whole string fit into this instance? - if (len > OUTPUT_BUFFER_SIZE) - { - // No - copy what we can't write into a new chain - OutputBuffer *currentBuffer; - size_t bytesCopied = OUTPUT_BUFFER_SIZE; - do { - if (!Allocate(currentBuffer)) - { - // We cannot store the whole string, stop here - break; - } - currentBuffer->references = references; - - // Fill up the next instance - const size_t copyLength = min(OUTPUT_BUFFER_SIZE, len - bytesCopied); - memcpy(currentBuffer->data, src + bytesCopied, copyLength); - currentBuffer->dataLength = copyLength; - bytesCopied += copyLength; - - // Link it to the chain - if (next == nullptr) - { - next = last = currentBuffer; - } - else - { - last->next = currentBuffer; - last = currentBuffer; - } - } while (bytesCopied < len); - - // Update references to the last entry for all items - for(OutputBuffer *item = Next(); item != last; item = item->Next()) - { - item->last = last; - } - - // Then copy the rest into this instance - memcpy(data, src, OUTPUT_BUFFER_SIZE); - dataLength = OUTPUT_BUFFER_SIZE; - return bytesCopied; - } - - // Yes, the whole string fits into this instance. No need to allocate a new item - memcpy(data, src, len); - dataLength = len; - return len; + dataLength = 0; + return cat(src, len); } size_t OutputBuffer::cat(const char c) @@ -235,36 +190,32 @@ size_t OutputBuffer::cat(const char *src) size_t OutputBuffer::cat(const char *src, size_t len) { - // Copy what we can into the last buffer - size_t copyLength = min(len, OUTPUT_BUFFER_SIZE - last->dataLength); - memcpy(last->data + last->dataLength, src, copyLength); - last->dataLength += copyLength; - - // Is there any more data left? - if (len > copyLength) + size_t copied = 0; + while (copied < len) { - // Yes - copy what we couldn't write into a new chain - OutputBuffer *nextBuffer; - if (!Allocate(nextBuffer)) + if (last->dataLength == OUTPUT_BUFFER_SIZE) { - // We cannot store any more data, stop here - return copyLength; + // The last buffer is full + OutputBuffer *nextBuffer; + if (!Allocate(nextBuffer)) + { + // We cannot store any more data, stop here + break; + } + nextBuffer->references = references; + last->next = nextBuffer; + last = nextBuffer->last; + for (OutputBuffer *item = Next(); item != nextBuffer; item = item->Next()) + { + item->last = last; + } } - nextBuffer->references = references; - const size_t bytesCopied = copyLength + nextBuffer->copy(src + copyLength, len - copyLength); - - // Done - now append the new entry to the chain - last->next = nextBuffer; - last = nextBuffer->last; - for(OutputBuffer *item = Next(); item != nextBuffer; item = item->Next()) - { - item->last = last; - } - return bytesCopied; + const size_t copyLength = min(len - copied, OUTPUT_BUFFER_SIZE - last->dataLength); + memcpy(last->data + last->dataLength, src + copied, copyLength); + last->dataLength += copyLength; + copied += copyLength; } - - // No more data had to be written, we could store everything - return len; + return copied; } size_t OutputBuffer::cat(StringRef &str) @@ -281,13 +232,15 @@ size_t OutputBuffer::EncodeString(const char *src, size_t srcLength, bool allowC bytesWritten += cat('"'); } - size_t srcPointer = 1; - char c = *src++; - while (srcPointer <= srcLength && c != 0 && (c >= ' ' || allowControlChars)) + if (srcLength != 0) { - char esc; - switch (c) + size_t srcPointer = 1; + char c = *src++; + while (srcPointer <= srcLength && c != 0 && (c >= ' ' || allowControlChars)) { + char esc; + switch (c) + { case '\r': esc = 'r'; break; @@ -310,20 +263,21 @@ size_t OutputBuffer::EncodeString(const char *src, size_t srcLength, bool allowC default: esc = 0; break; - } + } - if (esc != 0) - { - bytesWritten += cat('\\'); - bytesWritten += cat(esc); - } - else - { - bytesWritten += cat(c); - } + if (esc != 0) + { + bytesWritten += cat('\\'); + bytesWritten += cat(esc); + } + else + { + bytesWritten += cat(c); + } - c = *src++; - srcPointer++; + c = *src++; + srcPointer++; + } } if (encapsulateString) @@ -393,7 +347,7 @@ size_t OutputBuffer::EncodeReply(OutputBuffer *src, bool allowControlChars) // Get the number of bytes left for continuous writing /*static*/ size_t OutputBuffer::GetBytesLeft(const OutputBuffer *writingBuffer) { - size_t freeOutputBuffers = OUTPUT_BUFFER_COUNT - usedOutputBuffers; + const size_t freeOutputBuffers = OUTPUT_BUFFER_COUNT - usedOutputBuffers; if (writingBuffer == nullptr) { // Only return the total number of bytes left @@ -401,11 +355,11 @@ size_t OutputBuffer::EncodeReply(OutputBuffer *src, bool allowControlChars) } // We're doing a possibly long response like a filelist - size_t bytesLeft = OUTPUT_BUFFER_SIZE - writingBuffer->last->DataLength(); + const size_t bytesLeft = OUTPUT_BUFFER_SIZE - writingBuffer->last->DataLength(); if (freeOutputBuffers < RESERVED_OUTPUT_BUFFERS) { - // Keep some space left to encapsulate the respones (e.g. via an HTTP header) + // Keep some space left to encapsulate the responses (e.g. via an HTTP header) return bytesLeft; } diff --git a/src/OutputMemory.h b/src/OutputMemory.h index 3be373c..c90542a 100644 --- a/src/OutputMemory.h +++ b/src/OutputMemory.h @@ -29,6 +29,7 @@ class OutputBuffer void IncreaseReferences(size_t refs); const char *Data() const { return data; } + const char *UnreadData() const { return data + bytesRead; } size_t DataLength() const { return dataLength; } // How many bytes have been written to this instance? size_t Length() const; // How many bytes have been written to the whole chain? diff --git a/src/Version.h b/src/Version.h index 7e112d0..e315fce 100644 --- a/src/Version.h +++ b/src/Version.h @@ -9,11 +9,11 @@ #define SRC_VERSION_H_ #ifndef VERSION -# define VERSION "1.19alpha1b" +# define VERSION "1.19alpha1" #endif #ifndef DATE -# define DATE "2017-04-17" +# define DATE "2017-04-21" #endif #define AUTHORS "reprappro, dc42, chrishamm, t3p3, dnewman"