From befa1bbacb47157ab06f22c2261a35b288be3144 Mon Sep 17 00:00:00 2001 From: Harald Frostel Date: Sat, 17 Mar 2018 10:41:58 +0100 Subject: [PATCH 1/3] Fix random crashing of ClientContext::write(Stream) and write_P(PGM_P buf, size_t size) (#2504) --- .../ESP8266WiFi/src/include/DataSource.h | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/libraries/ESP8266WiFi/src/include/DataSource.h b/libraries/ESP8266WiFi/src/include/DataSource.h index 7f399a0584..2b4980cb71 100644 --- a/libraries/ESP8266WiFi/src/include/DataSource.h +++ b/libraries/ESP8266WiFi/src/include/DataSource.h @@ -31,14 +31,14 @@ class BufferDataSource : public DataSource { const uint8_t* get_buffer(size_t size) override { - (void) size; + (void)size; assert(_pos + size <= _size); return _data + _pos; } void release_buffer(const uint8_t* buffer, size_t size) override { - (void) buffer; + (void)buffer; assert(buffer == _data + _pos); _pos += size; } @@ -66,28 +66,48 @@ class BufferedStreamDataSource : public DataSource { const uint8_t* get_buffer(size_t size) override { assert(_pos + size <= _size); + + //Data that was already read from the stream but not released (e.g. if error occured). Otherwise this should be 0. + const size_t stream_read = _streamPos - _pos; + if (_bufferSize < size) { - _buffer.reset(new uint8_t[size]); + uint8_t *new_buffer = new uint8_t[size]; + //If stream reading is ahead, than some data is already in the old buffer and needs to be copied to new resized buffer + if (_buffer && stream_read > 0) { + memcpy(new_buffer, _buffer.get(), stream_read); + } + _buffer.reset(new_buffer); _bufferSize = size; } - size_t cb = _stream.readBytes(reinterpret_cast(_buffer.get()), size); - assert(cb == size); - (void) cb; + + //If error in tcp_write in ClientContext::_write_some() occured earlier and therefore release_buffer was not called, than there might not even be data needed to be read from the stream + if (size > stream_read) { + //Remaining bytes to read from stream + const size_t stream_rem = size - stream_read; + const size_t cb = _stream.readBytes(reinterpret_cast(_buffer.get() + stream_read), stream_rem); + assert(cb == stream_rem); + (void)cb; + _streamPos += stream_rem; + } return _buffer.get(); + } void release_buffer(const uint8_t* buffer, size_t size) override { - (void) buffer; + (void)buffer; _pos += size; + //Release size needs to be the same as get_buffer(size) + assert(_pos == _streamPos); } protected: - TStream& _stream; + TStream & _stream; std::unique_ptr _buffer; size_t _size; size_t _pos = 0; size_t _bufferSize = 0; + size_t _streamPos = 0; }; class ProgmemStream @@ -104,7 +124,7 @@ class ProgmemStream size_t will_read = (_left < size) ? _left : size; memcpy_P((void*)dst, (PGM_VOID_P)_buf, will_read); _left -= will_read; - _buf += will_read; + _buf += will_read; return will_read; } From d67641bd98e00b4c01fd7f1056c94e8f50f88fcf Mon Sep 17 00:00:00 2001 From: Harald Frostel Date: Sun, 18 Mar 2018 14:23:15 +0100 Subject: [PATCH 2/3] - Allow partial buffer release --- .../ESP8266WiFi/src/include/DataSource.h | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/libraries/ESP8266WiFi/src/include/DataSource.h b/libraries/ESP8266WiFi/src/include/DataSource.h index 2b4980cb71..ca06fc6b64 100644 --- a/libraries/ESP8266WiFi/src/include/DataSource.h +++ b/libraries/ESP8266WiFi/src/include/DataSource.h @@ -70,14 +70,16 @@ class BufferedStreamDataSource : public DataSource { //Data that was already read from the stream but not released (e.g. if error occured). Otherwise this should be 0. const size_t stream_read = _streamPos - _pos; - if (_bufferSize < size) { - uint8_t *new_buffer = new uint8_t[size]; + const size_t min_buffer_size = size > stream_read ? size : stream_read; + + if (_bufferSize < min_buffer_size) { + uint8_t *new_buffer = new uint8_t[min_buffer_size]; //If stream reading is ahead, than some data is already in the old buffer and needs to be copied to new resized buffer if (_buffer && stream_read > 0) { memcpy(new_buffer, _buffer.get(), stream_read); } _buffer.reset(new_buffer); - _bufferSize = size; + _bufferSize = min_buffer_size; } //If error in tcp_write in ClientContext::_write_some() occured earlier and therefore release_buffer was not called, than there might not even be data needed to be read from the stream @@ -95,10 +97,22 @@ class BufferedStreamDataSource : public DataSource { void release_buffer(const uint8_t* buffer, size_t size) override { + if (size == 0) { + return; + } + (void)buffer; - _pos += size; - //Release size needs to be the same as get_buffer(size) - assert(_pos == _streamPos); + _pos += size; + + //Cannot release more than acquired through get_buffer + assert(_pos <= _streamPos); + + if (_pos < _streamPos) { + //Released less than acquired through get_buffer + // Shift unreleased stream data in buffer to begining + assert(_buffer); + memmove(_buffer.get(), _buffer.get() + size, _streamPos - _pos); + } } protected: From 64fae1cc44abdd0cbd8d32d929b3595927597133 Mon Sep 17 00:00:00 2001 From: Harald Frostel Date: Thu, 22 Mar 2018 00:52:44 +0100 Subject: [PATCH 3/3] - Refined comments --- libraries/ESP8266WiFi/src/include/DataSource.h | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/libraries/ESP8266WiFi/src/include/DataSource.h b/libraries/ESP8266WiFi/src/include/DataSource.h index ca06fc6b64..2a0bfed260 100644 --- a/libraries/ESP8266WiFi/src/include/DataSource.h +++ b/libraries/ESP8266WiFi/src/include/DataSource.h @@ -67,11 +67,13 @@ class BufferedStreamDataSource : public DataSource { { assert(_pos + size <= _size); - //Data that was already read from the stream but not released (e.g. if error occured). Otherwise this should be 0. + //Data that was already read from the stream but not released (e.g. if tcp_write error occured). Otherwise this should be 0. const size_t stream_read = _streamPos - _pos; + //Min required buffer size: max(requested size, previous stream data already in buffer) const size_t min_buffer_size = size > stream_read ? size : stream_read; + //Buffer too small? if (_bufferSize < min_buffer_size) { uint8_t *new_buffer = new uint8_t[min_buffer_size]; //If stream reading is ahead, than some data is already in the old buffer and needs to be copied to new resized buffer @@ -82,7 +84,8 @@ class BufferedStreamDataSource : public DataSource { _bufferSize = min_buffer_size; } - //If error in tcp_write in ClientContext::_write_some() occured earlier and therefore release_buffer was not called, than there might not even be data needed to be read from the stream + //Fetch remaining data from stream + //If error in tcp_write in ClientContext::_write_some() occured earlier and therefore release_buffer was not called last time, than the requested stream data is already in the buffer. if (size > stream_read) { //Remaining bytes to read from stream const size_t stream_rem = size - stream_read; @@ -107,9 +110,9 @@ class BufferedStreamDataSource : public DataSource { //Cannot release more than acquired through get_buffer assert(_pos <= _streamPos); + //Release less than requested with get_buffer? if (_pos < _streamPos) { - //Released less than acquired through get_buffer - // Shift unreleased stream data in buffer to begining + // Move unreleased stream data in buffer to front assert(_buffer); memmove(_buffer.get(), _buffer.get() + size, _streamPos - _pos); }