Correct ssl recv #1
Conversation
Update fork with master
Syncing with me-no-dev repo
Syncing with upstream
…rver::_poll(). And other missed edit for errorTracker around ASYNC_TCP_SSL_ENABLED. This should resolve @kasedy comment me-no-dev#115 (comment) and @mcspr. Tested ASYNC_TCP_SSL_ENABLED using marvinroger/async-mqtt-client/ .. examples/FullyFeaturedSSL. Ran test against test.mosquitto.org's server. Thanks to @mcspr for suggesting. Updated tcp_ssl_read() to check for fd_data being freed by callback functions. I observed this with asyncmqttclient example. When finger print did not match during fd_data->on_handshake callback, the mqtt library did a close(true) which rippled down to an tcp_ssl_free(). Improvements in debug printing to handle debug print from tcp.axtls.c.
| #if defined(DEBUG_ESP_PORT) && !defined(DEBUG_GENERIC) | ||
| #define DEBUG_GENERIC( module, format, ... ) \ | ||
| do { \ | ||
| struct _DEBUG_TIME_STAMP st = debugTimeStamp(); \ | ||
| DEBUG_ESP_PORT.printf( DEBUG_TIME_STAMP_FMT module " " format, st.whole, st.dec, ##__VA_ARGS__ ); \ | ||
| DEBUG_ESP_PORT_PRINTF( (DEBUG_TIME_STAMP_FMT module " " format), st.whole, st.dec, ##__VA_ARGS__ ); \ |
There was a problem hiding this comment.
Why these changes relating to logging?
I'd prefer them to be filed in a separate PR to make reviewing easier.
There was a problem hiding this comment.
Some changes were needed to generalize the debug print macro a little more so I could get them to work from tcp_axtls.c.
| // fd_data may have been freed in callback | ||
| fd_data = tcp_ssl_get(tcp); | ||
| if(NULL == fd_data) | ||
| return SSL_CLOSE_NOTIFY; |
There was a problem hiding this comment.
Can you comment on where the actual fix is? I see a lot of changed lines, but I'd like to know where the fixed happened so that I can review this accurately.
There was a problem hiding this comment.
The fix is at line 418 through 420 and 404 through 407.
The problem occurs when the client does a delete on _client from a callback and fd_data becomes invalid.
|
I'm sorry, I cannot comment on any of the changes made, since I did not make them. This is a pending PR of the base repository by @mhightower83. Also @jeroenst did work on this. Merging these changes into ESPAsyncTCP for esphome helped solving my issues described earlier. Shame on me, I have not worked through the code of these libraries. I'm just thankfully using them. Feel free to close this PR and pull from the original repo as soon as the changes are merged. |
|
Hi everyone, super nice work! , I just tested this and it works great. I understand the question of separating logging related changes and the actual fix. But I think merging them together makes sense in this case... |
|
While this is waiting to be merged, can someone tell me where (i can figure out the how) to manually replace these files with the modified versions in my local installation (HASS.io AddOn or pip setup)? |
I can't comment on HASS.io but on CLI: When you want to compile & upload your project, the compilation usually fails due these MQTT SSL errors. At this time platformio already created folders within your project directory storing the dependencies and also the platformio.ini file for your project. Within these (hidden) folders there should be the ESPAsyncTCP library. Look for the corresponding files and replace them. Aftwerwards compile and upload. I used platformio directly instead of esphome for this step. |
fixes esphome compilation issue when ssl fingerprint is enabled
as discussed here: esphome/issues#786