Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handling of errors - like unstable network - coming via SSL #89

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bligeti
Copy link

@bligeti bligeti commented May 12, 2020

Possible fix for #85
The trigger of this problem can be network disconnections or similar errors coming via SSL.

In HTTPConnection::updateBuffer() closeConnection() is called when error is detected from SSL to clean up but the possibility of already cleaned up session is not handled in HTTPConnection::pendingBufferSize() and in HTTPConnection::loop().

Also in such case WSHandler::onClose() is not called therefore the recorded handlers (like in the websocket example) are not cleaned up.

In case closeConnection() is called due to SSL reported error - like session disconnection due to unstable Wifi -  onClose() should be called to free up the recorded websocket handlers.
From updateBuffer() a session clean up is initiated via closeConnection() if error is detected when reading from SSL but the already removed session is not handled in other parts of the state machine.
@fhessel fhessel added CI: Build Examples When set, the examples will be built for this PR and removed CI: Build Examples When set, the examples will be built for this PR labels Jun 6, 2020
@fhessel fhessel self-requested a review June 6, 2020 23:57
Copy link
Owner

@fhessel fhessel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for providing the PR, I'll try to check it on a board asap.

However, during the code review I noticed this line (which has been there already before the PR):

size_t HTTPSConnection::readBytesToBuffer(byte* buffer, size_t length) {

Using size_t instead of ssize_t means that no underlying SSL error will ever be reported to the caller. That alone will cause trouble when using readBytesToBuffer(). The same goes for passing the return value of recv() in the basic HTTPConnection variant.

In particular, this means that the else branch in HTTPConnection.cpp:195..212 will never be reached with an int readReturnCode being set based on a size_t.

        if (readReturnCode > 0) {
          // ...
        } else if (readReturnCode == 0) {
          // ...
        } else {
          // dead code
        }

So I would be curious if you ever saw that added SSL_error=... message in your logs?

src/HTTPConnection.cpp Outdated Show resolved Hide resolved
src/HTTPSConnection.cpp Outdated Show resolved Hide resolved
src/HTTPConnection.cpp Outdated Show resolved Hide resolved
KaloNK added a commit to KaloNK/esp32_https_server that referenced this pull request Nov 29, 2021
Merge fhessel#89 from fhessel/esp32_https_server
KaloNK added a commit to KaloNK/esp32_https_server that referenced this pull request Nov 29, 2021
* Trigger wsHander onClose from closeConnection()

In case closeConnection() is called due to SSL reported error - like session disconnection due to unstable Wifi -  onClose() should be called to free up the recorded websocket handlers.

* Handle error triggered by closed sessions

From updateBuffer() a session clean up is initiated via closeConnection() if error is detected when reading from SSL but the already removed session is not handled in other parts of the state machine.

* Get detailed SSL_read error

* Update src/HTTPSConnection.cpp

Co-authored-by: Frank Hessel <[email protected]>

* Update src/HTTPConnection.cpp

Co-authored-by: Frank Hessel <[email protected]>

* correct the copy-paste error

Co-authored-by: bligeti <[email protected]>
Co-authored-by: Frank Hessel <[email protected]>
gb-123-git added a commit to gb-123-git/esp32_https_server that referenced this pull request Jan 23, 2022
May help in crash fix for websockets.
gb-123-git added a commit to gb-123-git/esp32_https_server that referenced this pull request Jan 23, 2022
Update as mentioned in issue fhessel#89 for possible crash fix on websockets.
khoih-prog added a commit to khoih-prog/ESP32_HTTPS_Server that referenced this pull request Sep 27, 2022
Merge some upstream PRs

- Handling of errors - like unstable network - coming via SSL fhessel#89
- WIP: Prevent crash on WebSocket request to non-WebSocket node. fhessel#106
- Fix infinite loop when the buffer ends with \r fhessel#123
- Fixed memory leak in the Websocket example fhessel#157
This was referenced Sep 27, 2022
khoih-prog added a commit to khoih-prog/ESP32_HTTPS_Server that referenced this pull request Sep 27, 2022
1. Merge some upstream PRs

- Handling of errors - like unstable network - coming via SSL fhessel#89
- WIP: Prevent crash on WebSocket request to non-WebSocket node. fhessel#106
- Fix infinite loop when the buffer ends with \r fhessel#123
- Fixed memory leak in the Websocket example fhessel#157

2. Update examples and `README.md`
khoih-prog added a commit to khoih-prog/ESP32_HTTPS_Server that referenced this pull request Sep 27, 2022
1. Merge some upstream PRs

- Handling of errors - like unstable network - coming via SSL fhessel#89
- WIP: Prevent crash on WebSocket request to non-WebSocket node. fhessel#106
- Fix infinite loop when the buffer ends with \r fhessel#123
- Fixed memory leak in the Websocket example fhessel#157

2. Update examples and `README.md`
khoih-prog added a commit to khoih-prog/ESP32_HTTPS_Server that referenced this pull request Sep 27, 2022
1. Merge some upstream PRs

- Handling of errors - like unstable network - coming via SSL fhessel#89
- WIP: Prevent crash on WebSocket request to non-WebSocket node. fhessel#106
- Fix infinite loop when the buffer ends with \r fhessel#123
- Fixed memory leak in the Websocket example fhessel#157

2. Update examples and `README.md`
gb-123-git added a commit to gb-123-git/esp32_https_server that referenced this pull request Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants