Skip to content

Calls to stop before connected cause lockups. #13

Closed

Description

@bleckers

Describe the bug
Calling sslClient.stop() when !sslClient.connected() causes buffer overflow, even after previously successful connection (and disconnect).

To Reproduce
EthernetClient mqttEthClient;
Ethernet.init(14);
SSLClient sslClient(mqttEthClient, TAs_HTTPS, (size_t)TAs_NUM_HTTPS, RANDOM_DATA_PIN);
sslClient.stop();

Expected behavior
Calls to stop should exit gracefully or ignore the request if already exited.

Context (please complete the following information):

  • Device Type - ESP32/Teensy4
  • Arduino Core Version - Teensyduino 1.52
  • Relevant Library Versions - PubSubClient
  • SSLClient Version - v1.6.6

Additional context
Discovered while trying to make the library robust to internet connectivity issues. Removing jump_handshake in br_ssl_engine_close prevents this (not a fix).

Activity

  1. prototypicalpro commented on Jun 14, 2020

    @prototypicalpro
    Member

    Is the behavior a crash or a freeze? In my experience BearSSL attempts to close the connection by sending a close_notify (which is usually ignored) and then times out waiting for a response.

  2. prototypicalpro commented on Jun 14, 2020

    @prototypicalpro
    Member

    I think 4707ea4 should fix the close_notify issue, would you give master latest a shot?

  3. bleckers commented on Jun 15, 2020

    @bleckers
    ContributorAuthor

    The behaviour is a crash rather than a freeze. The new commit fixes this with a fresh stop before connect, after a client has connected and disconnected AND if the client is connected.

  4. bleckers commented on Jun 15, 2020

    @bleckers
    ContributorAuthor

    Oh just to add, if you have network connectivity, a connected socket, but temporarily no route to the server and are using the Arduino Ethernet 2.0 library, you will need the following change in the EthernetClient.cpp for this commit to fix these issues (as calls to get_arduino_client().flush() in this configuration will hang indefinitely when there's no route). This is also true for calls to flush in the m_update_engine of SSLClient, and considering the lack of development effort in Ethernet, it might have to be a recommendation in SSLClient (particularly if using PubSubClient because of the regular pings which are susceptible to minor lapses in network connectivity).

    void EthernetClient::flush()
    {
    	unsigned long start = millis();
    	while (sockindex < MAX_SOCK_NUM) {
    		if (millis() - start > _timeout) 
    		{
    			stop();
    			return;
    		}
    		uint8_t stat = Ethernet.socketStatus(sockindex);
    		if (stat != SnSR::ESTABLISHED && stat != SnSR::CLOSE_WAIT) return;
    		if (Ethernet.socketSendAvailable(sockindex) >= W5100.SSIZE) return;
    	}
    }
    

    This has been PRed into Paul's "fork" of the Arduino repo - PaulStoffregen/Ethernet@master...bleckers:patch-1

  5. prototypicalpro commented on Jun 15, 2020

    @prototypicalpro
    Member

    Hmmm yeah, I don't think I have a good way of enforcing a timeout on Client::flush without modifying the client implementation. I will add a note in the README about this, and patch EthernetLarge as soon as your pull request is merged.

  6. added a commit that references this issue on Jun 15, 2020
    f40b9c4
  7. linked a pull request that will close this issueFix #13 #14on Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions