WiFiClientSecure: don't trash unread decrypted data when writing #4024
Conversation
There was a problem hiding this comment.
Instead of queue and all the C-style data handling, wouldn't it be better to use std C++ containers? E.g.: a std::list of std::vector or something along those lines. It would reduce code complexity. The user-facing interface can be wrapped hide the container syntax.
|
Vector doesn't fit: when you try to append a new block of data, it will try to allocate contiguous region to fit all existing data and new data. With fragmented heap, this might not be possible. List-of-buffers implemented here does not have this limitation. (Not to say I'm totally happy with the existing code — which is the reason this is marked as WIP). |
|
std::list might be okay though, if I can make the code size hit less noticable. |
|
Does this mean that BearSSL replacement is no longer on the table? It's separate RX and TX buffers (of separately configurable sizes) would avoid contortions like this, no? |
|
@earlephilhower It doesn't meant that. This workaround is intended for axTLS only. I assume that if/when BearSSL is adopted, WiFiClientSecure will need a more or less full rewrite. |
igrr
force-pushed
the
bugfix/wificlientsecure_full_duplex
branch
from
d5dbaa6
to
4dbab22
Compare
|
@devyte Please take a look, i have replaced all C-style buffer handling with std::list + std::unique_ptr. Turns out I have already pulled in std::list from ESP8266WiFiGeneric.cpp, so code size overhead from this change is just a couple hundred bytes, compared to STAILQ. |
igrr
changed the title
|
I was doing just that. Looks much better! I'll add specific comments to code change lines. |
| typedef struct BufferItem | ||
| { | ||
| BufferItem(const uint8_t* data_, size_t size_) : size(size_) { | ||
| data.reset(new uint8_t[size]); |
There was a problem hiding this comment.
I suggest removing this reset() in the body and directly constructing:
BufferItem(const uint8_t* data_, size_t size_) :
size(size_),
data(new uint8_t[size_]) {
| } | ||
| return result; | ||
| } | ||
|
|
||
| int write(const uint8_t* src, size_t size) | ||
| { | ||
| if (!_available) { |
There was a problem hiding this comment.
Should this check !_available or !hasData()?
There was a problem hiding this comment.
No need to check for hasData, as far as i can tell — we only need to make sure that the fragment buffer is empty.
| */ | ||
| return _writeBufferAdd(src, size); | ||
| } | ||
|
|
||
| int peek() | ||
| { | ||
| if (!_available) { |
There was a problem hiding this comment.
Should this check !available or !hasData()?
There was a problem hiding this comment.
I don't think so. The logic is:
- Is there some incoming plaintext data? If not, we'll try to get any received data from the TCP stack, and decrypt it.
- If after step 1 there is some plaintext data, return the first byte of it.
| data.reset(new uint8_t[size]); | ||
| if (data.get() != nullptr) { | ||
| memcpy(data.get(), data_, size); | ||
| } |
There was a problem hiding this comment.
Maybe an else here with a DEBUGV msg?
|
|
||
| _writeBuffers.emplace_back(data, size); | ||
| if (_writeBuffers.back().data.get() == nullptr) { | ||
| _writeBuffers.pop_back(); |
There was a problem hiding this comment.
Mayeb add a DEBUGV msg here?
There was a problem hiding this comment.
Added a DEBUGV into BufferItem constructor, where the memory allocation failure is first detected.
|
|
||
| int _writeBuffersSend() | ||
| { | ||
| while (!_writeBuffers.empty()) { |
There was a problem hiding this comment.
Is this a lengthy operation? Are there timing concerns due to the exection time of this loop?
There was a problem hiding this comment.
Depends on the amount of data queued for transmission. In most cases, this will be zero. For protocols like MQTT this may send a few packets. There will be yields in ClientContext, so we don't need to worry about starving the WiFi stack here.
| auto& first = _writeBuffers.front(); | ||
| int rc = _write(first.data.get(), first.size); | ||
| _writeBuffers.pop_front(); | ||
| if (rc < 0) { |
There was a problem hiding this comment.
Maybe a DEBUGV msg here for the failure and for the buffers getting dropped (data drop)?
There was a problem hiding this comment.
Added DEBUGV for ssl_write failure into _write and a message about buffers being dropped here.
When application requests to write data, check if there is any unread decrypted data left. If there is, don't write immediately, but save the data to be written. When all decrypted data has been consumed by the application, send out the saved outgoing data. Fixes #2256.
igrr
force-pushed
the
bugfix/wificlientsecure_full_duplex
branch
from
4dbab22
to
02bc538
Compare
|
@igrr Thank you. Looks stable in our scenario |
When application requests to write data, check if there is any unread decrypted data left. If there is, don't write immediately, but save the data to be written. When all decrypted data has been consumed by the application, send out the saved outgoing data.
Also don't use
available()in the test forconnected()to reduce the chance that plaintext buffer gets filled with incoming data when the application does not actually need it, and does not need to know its exact size.Fixes #2256.