WiFi*Secure: fix for multiple clients / servers

[d-a-v]

@earlephilhower
This is a PR on top of #3001.

This PR allows to make simultaneous secure servers working.
It should allow simultaneous secure clients working too though this is not tried (I do not know whether it is/was working).
It uses std::map to associate axtls file descriptor and ClientContext.

I was hoping to make a PR on earlephilhower's master but github does not work that way...
So I provide the patch (edit: removed, see below) here so it can be tested/included in the original PR

@igrr this is also a WiFiClientSecure fix about axtls's fd that I could only quickly test with Earle's WiFiServerSecure.

[earlephilhower]

Wow, great! I tried something like this initially, but ran out of memory for the new SSL_CTX's in my own application so didn't bother with it. Add this and a SSL MQTT connection and there's little space (maybe negative space!) left on the poor ESP8266 to work with.

Is there any way to make this configurable either at runtime or compile time to choose between a shared context (for memory constrained apps) and a unique (separate from client) one per server?

Actually, I think this may be the reason the client only has a single shared context...there's just not enough memory to sustain multiple SSL connections in many cases.

[d-a-v]

edit: canceled useless second commit
The initial commit is all about assigning a unique "axtls file descriptor" to every new connection.
SSL-Contexts were and still are shared.

[d-a-v]

I did try the example HTTPSRequest in which I make 2 concurrent connections and surround r/w accesses in for() loops :

WiFiClientSecure clients[n];

for (int i = 0; i < n; i++)
  client[i].print()

It does work with the first patch above, it does not without (n=2).
So I will remove the second patch which is useless (I hope I git-can without a reverting commit - edit: done)

[earlephilhower]

Cool, thanks for clarifying. Unique FDs should be no issue, memory-wise. And you probably fixed my problem where SSL outgoing connections (MQTT) were dying on incoming (HTTPS) requests!

[d-a-v]

did you actually verify this ?

[earlephilhower]

I was doing a server rebuild this weekend and not able to run any code. I'll try to test it out tonight or tomorrow, and update this tracker.

[earlephilhower]

Sorry for the delay in getting back to you. I ended up with 2 drives in a RAID6 array giving up the ghost and had to get all that sorted before I found time for this.

I've hand-added your patch, and in doing so found one logic issue: You need to check for _fdcounter over/underflow to zero or negative (negative FDs in UNIX are used to indicate error conditions, not sure if AXTLS has same concept or not, but it's safe to just disallow generating them too) and reset it to 1, or else you'll have a failure on wraparound. It may take many months to happen, but when it does it'll be a pain to figure out. It's a simple check on pre-increment.

That said, I'm seeing AXTLS crashes when I have a SSL server and a SSL client both trying to do work at the same time (i.e MQTT report while doing a web page serve). I'll try and dig into it more, but I think there's something else subtle we need to worry about, especially if the certificates/keys for client and server connections are not the same. When I stop work on the MQTT SSL client during SSL server responses, though, it's fine and dandy without any issues.

There's also a chance I made a mistake while hand-merging the diffs. If you have a second, can you check and see if I missed something here? I'll re-eyeball things, but it's always better to have a second set check this stuff out...

earle@server:~/.arduino15/packages/esp8266/hardware/esp8266/2.3.0/libraries/ESP8266WiFi/src$ diff --unified WiFiClientSecure.cpp.bak  WiFiClientSecure.cpp
--- WiFiClientSecure.cpp.bak	2017-06-10 10:48:27.822378888 -0700
+++ WiFiClientSecure.cpp	2017-06-10 11:07:58.940065682 -0700
@@ -39,6 +39,7 @@
 #include "lwip/netif.h"
 #include "include/ClientContext.h"
 #include "c_types.h"
+#include <map>
 
 #ifdef DEBUG_ESP_SSL
 #define DEBUG_SSL
@@ -71,6 +72,7 @@
             _ssl_server_ctx = ssl_ctx_new(SSL_SERVER_VERIFY_LATER | SSL_DEBUG_OPTS | SSL_CONNECT_IN_PARTS | SSL_READ_BLOCKING, 0);
         }
         ++_ssl_ctx_refcnt;
+        _fd = 0; // Not yet used
     }
 
     ~SSLContext() {
@@ -85,7 +87,9 @@
             ssl_ctx_free(_ssl_server_ctx);
         }
 
-        s_io_ctx = nullptr;
+        if (_fd) {
+            s_io_ctx.erase(_fd);
+        }
     }
 
     void ref() {
@@ -100,8 +104,13 @@
 
     void connectServer(ClientContext *ctx) {
 if (_ssl) { Serial.print("_ssl not numm:"); Serial.println((int)_ssl);}
-        s_io_ctx = ctx;
-	_ssl = ssl_server_new(_ssl_server_ctx, (int)this);
+        _fd = ++_fdcounter;
+        if (_fdcounter < 1) { // Handle wraparound of increment
+            _fd = 1;
+            _fdcounter=1;
+        }
+        s_io_ctx[_fd] = ctx;
+	_ssl = ssl_server_new(_ssl_server_ctx, _fd);
         _isServer = true;
 
 	int timeout_ms = 5000;
@@ -119,9 +128,14 @@
     }
 
     void connect(ClientContext* ctx, const char* hostName, uint32_t timeout_ms) {
-if (_ssl) { Serial.print("_ssl not numm:"); Serial.println((int)_ssl);}
-        s_io_ctx = ctx;
-        _ssl = ssl_client_new(_ssl_ctx, 0, nullptr, 0, hostName);
+if (_ssl) { Serial.print("_ssl not null:"); Serial.println((int)_ssl);}
+        _fd = ++_fdcounter;
+        if (_fdcounter < 1) { // Handle wraparound of increment
+            _fd = 1;
+            _fdcounter=1;
+        }
+        s_io_ctx[_fd] = ctx;
+        _ssl = ssl_client_new(_ssl_ctx, _fd, nullptr, 0, hostName);
         uint32_t t = millis();
 
         while (millis() - t < timeout_ms && ssl_handshake_status(_ssl) != SSL_OK) {
@@ -136,7 +150,7 @@
     }
 
     void stop() {
-        s_io_ctx = nullptr;
+        // s_io_ctx = nullptr;
     }
 
     bool connected() {
@@ -207,7 +221,7 @@
     }
 
     static ClientContext* getIOContext(int fd) {
-        return s_io_ctx;
+        return s_io_ctx[fd];
     }
 
     int loadX509Cert(const uint8_t *cert, int len) {
@@ -254,18 +268,21 @@
     static SSL_CTX* _ssl_ctx;
     static SSL_CTX* _ssl_server_ctx; // 3/11
     static int _ssl_ctx_refcnt;
+    static int _fdcounter; // Increased every constructor call
+    int _fd; // AXTLS file descriptor
     SSL* _ssl = nullptr;
     int _refcnt = 0;
     const uint8_t* _read_ptr = nullptr;
     size_t _available = 0;
-    static ClientContext* s_io_ctx;
+    static std::map<int, ClientContext*> s_io_ctx;
 };
 
 SSL_CTX* SSLContext::_ssl_ctx = nullptr;
 SSL_CTX* SSLContext::_ssl_server_ctx = nullptr;
 int SSLContext::_ssl_ctx_refcnt = 0;
-
-ClientContext* SSLContext::s_io_ctx = nullptr;
+int SSLContext::_fdcounter = 0;
+std::map<int, ClientContext*> SSLContext::s_io_ctx;
+//ClientContext* SSLContext::s_io_ctx = nullptr;
 
 
 WiFiClientSecure::WiFiClientSecure() {

[earlephilhower]

I did some digging and this was caused by a use-after-free issue in the logic when stop() is called.

I also figured out a simple way to avoid the whole issue of fdcounter...sizeof(int)==sizeof(SSLContext*)...so just send in (this) and have a class variable (not-static) for the clientcontext. This allows you to zero it out after a stop() is called but before the class is destroyed, avoiding the hangs I was having.

I'll clean up my code and update my pull request with the minor change. It's maybe 3 lines to implement, and has been working all morning with my testing psychoplug w/SSL MQTT and HTTPS web interface.

[d-a-v]

I like very much your idea to replace the fdcounter mapping by the context address.
I'll very soon try your PR and close this one.

[d-a-v]

@earlephilhower your recent update works for me. Happily closing here.