From 9dce0764af1abe62c851bc457bdaa63f272980be Mon Sep 17 00:00:00 2001 From: Max Prokhorov Date: Fri, 16 Dec 2022 14:12:58 +0300 Subject: [PATCH] Don't return `true` with `WiFiClientSecureBearSSL::connected()` when really disconnected (#8330) * Don't return `true` with WiFiClientSecureBearSSL::connected() when disconnected Apply the same condition as with normal WiFiClient - we are not connected when it's not possible to both write and read. Implement separate methods for actual connection status and the internal ssl engine status and update methods that were previously using available() for this purpose Update examples to check available() when the intent is to only read the data and not interact with the client in any other way. Also, use connect() as a way to notify errors, no need to check things twice --- .../BearSSL_CertStore/BearSSL_CertStore.ino | 5 +- .../BearSSL_MaxFragmentLength.ino | 6 +- .../BearSSL_Sessions/BearSSL_Sessions.ino | 5 +- .../BearSSL_Validation/BearSSL_Validation.ino | 5 +- .../examples/HTTPSRequest/HTTPSRequest.ino | 2 +- .../src/WiFiClientSecureBearSSL.cpp | 61 +++++++++++++------ .../ESP8266WiFi/src/WiFiClientSecureBearSSL.h | 6 ++ 7 files changed, 59 insertions(+), 31 deletions(-) diff --git a/libraries/ESP8266WiFi/examples/BearSSL_CertStore/BearSSL_CertStore.ino b/libraries/ESP8266WiFi/examples/BearSSL_CertStore/BearSSL_CertStore.ino index fd543933b4..f96a4ccf83 100644 --- a/libraries/ESP8266WiFi/examples/BearSSL_CertStore/BearSSL_CertStore.ino +++ b/libraries/ESP8266WiFi/examples/BearSSL_CertStore/BearSSL_CertStore.ino @@ -75,8 +75,7 @@ void fetchURL(BearSSL::WiFiClientSecure *client, const char *host, const uint16_ if (!path) { path = "/"; } Serial.printf("Trying: %s:443...", host); - client->connect(host, port); - if (!client->connected()) { + if (!client->connect(host, port)) { Serial.printf("*** Can't connect. ***\n-------\n"); return; } @@ -88,7 +87,7 @@ void fetchURL(BearSSL::WiFiClientSecure *client, const char *host, const uint16_ client->write("\r\nUser-Agent: ESP8266\r\n"); client->write("\r\n"); uint32_t to = millis() + 5000; - if (client->connected()) { + while (client->available()) { do { char tmp[32]; memset(tmp, 0, 32); diff --git a/libraries/ESP8266WiFi/examples/BearSSL_MaxFragmentLength/BearSSL_MaxFragmentLength.ino b/libraries/ESP8266WiFi/examples/BearSSL_MaxFragmentLength/BearSSL_MaxFragmentLength.ino index 03b619f932..70e7f789e4 100644 --- a/libraries/ESP8266WiFi/examples/BearSSL_MaxFragmentLength/BearSSL_MaxFragmentLength.ino +++ b/libraries/ESP8266WiFi/examples/BearSSL_MaxFragmentLength/BearSSL_MaxFragmentLength.ino @@ -44,8 +44,7 @@ int fetchNoMaxFragmentLength() { BearSSL::WiFiClientSecure client; client.setInsecure(); - client.connect("tls.mbed.org", 443); - if (client.connected()) { + if (client.connect("tls.mbed.org", 443)) { Serial.printf("Memory used: %d\n", ret - ESP.getFreeHeap()); ret -= ESP.getFreeHeap(); fetch(&client); @@ -81,8 +80,7 @@ int fetchMaxFragmentLength() { Serial.printf("\nConnecting to https://tls.mbed.org\n"); Serial.printf("MFLN supported: %s\n", mfln ? "yes" : "no"); if (mfln) { client.setBufferSizes(512, 512); } - client.connect("tls.mbed.org", 443); - if (client.connected()) { + if (client.connect("tls.mbed.org", 443)) { Serial.printf("MFLN status: %s\n", client.getMFLNStatus() ? "true" : "false"); Serial.printf("Memory used: %d\n", ret - ESP.getFreeHeap()); ret -= ESP.getFreeHeap(); diff --git a/libraries/ESP8266WiFi/examples/BearSSL_Sessions/BearSSL_Sessions.ino b/libraries/ESP8266WiFi/examples/BearSSL_Sessions/BearSSL_Sessions.ino index be1ad6e4b7..cf227403f3 100644 --- a/libraries/ESP8266WiFi/examples/BearSSL_Sessions/BearSSL_Sessions.ino +++ b/libraries/ESP8266WiFi/examples/BearSSL_Sessions/BearSSL_Sessions.ino @@ -63,8 +63,7 @@ void fetchURL(BearSSL::WiFiClientSecure *client, const char *host, const uint16_ if (!path) { path = "/"; } Serial.printf("Trying: %s:443...", host); - client->connect(host, port); - if (!client->connected()) { + if (!client->connect(host, port)) { Serial.printf("*** Can't connect. ***\n-------\n"); return; } @@ -76,7 +75,7 @@ void fetchURL(BearSSL::WiFiClientSecure *client, const char *host, const uint16_ client->write("\r\nUser-Agent: ESP8266\r\n"); client->write("\r\n"); uint32_t to = millis() + 5000; - if (client->connected()) { + while (client->available()) { do { char tmp[32]; memset(tmp, 0, 32); diff --git a/libraries/ESP8266WiFi/examples/BearSSL_Validation/BearSSL_Validation.ino b/libraries/ESP8266WiFi/examples/BearSSL_Validation/BearSSL_Validation.ino index f1bc55e411..cc76e5d24e 100644 --- a/libraries/ESP8266WiFi/examples/BearSSL_Validation/BearSSL_Validation.ino +++ b/libraries/ESP8266WiFi/examples/BearSSL_Validation/BearSSL_Validation.ino @@ -49,8 +49,7 @@ void fetchURL(BearSSL::WiFiClientSecure *client, const char *host, const uint16_ ESP.resetFreeContStack(); uint32_t freeStackStart = ESP.getFreeContStack(); Serial.printf("Trying: %s:443...", host); - client->connect(host, port); - if (!client->connected()) { + if (!client->connect(host, port)) { Serial.printf("*** Can't connect. ***\n-------\n"); return; } @@ -62,7 +61,7 @@ void fetchURL(BearSSL::WiFiClientSecure *client, const char *host, const uint16_ client->write("\r\nUser-Agent: ESP8266\r\n"); client->write("\r\n"); uint32_t to = millis() + 5000; - if (client->connected()) { + while (client->available()) { do { char tmp[32]; memset(tmp, 0, 32); diff --git a/libraries/ESP8266WiFi/examples/HTTPSRequest/HTTPSRequest.ino b/libraries/ESP8266WiFi/examples/HTTPSRequest/HTTPSRequest.ino index 33327cde4f..696d513b52 100644 --- a/libraries/ESP8266WiFi/examples/HTTPSRequest/HTTPSRequest.ino +++ b/libraries/ESP8266WiFi/examples/HTTPSRequest/HTTPSRequest.ino @@ -77,7 +77,7 @@ void setup() { client.print(String("GET ") + url + " HTTP/1.1\r\n" + "Host: " + github_host + "\r\n" + "User-Agent: BuildFailureDetectorESP8266\r\n" + "Connection: close\r\n\r\n"); Serial.println("Request sent"); - while (client.connected()) { + while (client.available()) { String line = client.readStringUntil('\n'); if (line == "\r") { Serial.println("Headers received"); diff --git a/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp b/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp index d97e6be7cb..53b606f8c3 100644 --- a/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp +++ b/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp @@ -250,19 +250,32 @@ void WiFiClientSecureCtx::_freeSSL() { } bool WiFiClientSecureCtx::_clientConnected() { - return (_client && _client->state() == ESTABLISHED); + if (!_client || (_client->state() == CLOSED)) { + return false; + } + + return _client->state() == ESTABLISHED; +} + +bool WiFiClientSecureCtx::_engineConnected() { + return _clientConnected() && _handshake_done && _eng && (br_ssl_engine_current_state(_eng) != BR_SSL_CLOSED); } uint8_t WiFiClientSecureCtx::connected() { - if (available() || (_clientConnected() && _handshake_done && (br_ssl_engine_current_state(_eng) != BR_SSL_CLOSED))) { + if (!_engineConnected()) { + return false; + } + + if (_pollRecvBuffer() > 0) { return true; } - return false; + + return _engineConnected(); } int WiFiClientSecureCtx::availableForWrite () { - // code taken from ::_write() - if (!connected() || !_handshake_done) { + // Can't write things when there's no connection or br_ssl engine is closed + if (!_engineConnected()) { return 0; } // Get BearSSL to a state where we can send @@ -284,7 +297,7 @@ int WiFiClientSecureCtx::availableForWrite () { size_t WiFiClientSecureCtx::_write(const uint8_t *buf, size_t size, bool pmem) { size_t sent_bytes = 0; - if (!connected() || !size || !_handshake_done) { + if (!size || !_engineConnected()) { return 0; } @@ -331,10 +344,11 @@ size_t WiFiClientSecureCtx::write_P(PGM_P buf, size_t size) { } size_t WiFiClientSecureCtx::write(Stream& stream) { - if (!connected() || !_handshake_done) { - DEBUG_BSSL("write: Connect/handshake not completed yet\n"); + if (!_engineConnected()) { + DEBUG_BSSL("write: no br_ssl engine to work with\n"); return 0; } + return stream.sendAll(this); } @@ -343,12 +357,20 @@ int WiFiClientSecureCtx::read(uint8_t *buf, size_t size) { return -1; } - int avail = available(); - bool conn = connected(); - if (!avail && conn) { - return 0; // We're still connected, but nothing to read + // will either check the internal buffer, or try to wait for some data + // *may* attempt to write some pending ::write() data b/c of _run_until + int avail = _pollRecvBuffer(); + + // internal buffer might still be available for some time + bool engine = _engineConnected(); + + // we're still connected, but nothing to read + if (!avail && engine) { + return 0; } - if (!avail && !conn) { + + // or, available failed to assign the internal buffer and we are already disconnected + if (!avail && !engine) { DEBUG_BSSL("read: Not connected, none left available\n"); return -1; } @@ -363,10 +385,11 @@ int WiFiClientSecureCtx::read(uint8_t *buf, size_t size) { return to_copy; } - if (!conn) { + if (!engine) { DEBUG_BSSL("read: Not connected\n"); return -1; } + return 0; // If we're connected, no error but no read. } @@ -395,7 +418,7 @@ int WiFiClientSecureCtx::read() { return -1; } -int WiFiClientSecureCtx::available() { +int WiFiClientSecureCtx::_pollRecvBuffer() { if (_recvapp_buf) { return _recvapp_len; // Anything from last call? } @@ -416,8 +439,12 @@ int WiFiClientSecureCtx::available() { return 0; } +int WiFiClientSecureCtx::available() { + return _pollRecvBuffer(); +} + int WiFiClientSecureCtx::peek() { - if (!ctx_present() || !available()) { + if (!ctx_present() || (0 == _pollRecvBuffer())) { DEBUG_BSSL("peek: Not connected, none left available\n"); return -1; } @@ -436,7 +463,7 @@ size_t WiFiClientSecureCtx::peekBytes(uint8_t *buffer, size_t length) { } _startMillis = millis(); - while ((available() < (int) length) && ((millis() - _startMillis) < 5000)) { + while ((_pollRecvBuffer() < (int) length) && ((millis() - _startMillis) < 5000)) { yield(); } diff --git a/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h b/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h index cc08f10353..d78dad2972 100644 --- a/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h +++ b/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h @@ -195,7 +195,13 @@ class WiFiClientSecureCtx : public WiFiClient { unsigned char *_recvapp_buf; size_t _recvapp_len; + int _pollRecvBuffer(); // If there's a buffer with some pending data, return it's length + // If there's no buffer, poll the engine and store any received data there and return the length + // (which also may change the internal state, e.g. make us disconnected) + bool _clientConnected(); // Is the underlying socket alive? + bool _engineConnected(); // Are both socket and the bearssl engine alive? + std::shared_ptr _alloc_iobuf(size_t sz); void _freeSSL(); int _run_until(unsigned target, bool blocking = true);