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

espconn_secure_set_size leaks discussion #1457

Closed
djphoenix opened this issue Aug 14, 2016 · 10 comments
Closed

espconn_secure_set_size leaks discussion #1457

djphoenix opened this issue Aug 14, 2016 · 10 comments

Comments

@djphoenix
Copy link
Contributor

As I mentioned at MQTT PR, sequential calls of espconn_secure_set_size may cause some leaks.

I tested this behavior on SDKs 1.5.4.1 and 2.0.0, and for both of them situation is very unstable (from normal pass to fatal error).

Fix proposal

  1. Choose default "secure size" like 4096 (maybe more or less) and put it in user_config.h for keep it user-configurable.
  2. Grep all invocations of espconn_secure_set_size in modules and remove them.
  3. Add one invocation right near usercode initialization.
  4. ...
  5. Remove entire espconn layer and move to mbedtls/lwip (WIP)
@devsaurus
Copy link
Member

I second Yury's proposal. It'll yield leaner code and apparently avoids spurious leaks.

@djphoenix
Copy link
Contributor Author

I prefer use size of power-of-two (4096 e.g.), but, how I see, there is 5120 in most places. @devsaurus @marcelstoer @pjsg who can describe this magic number?

$ grep -r 'espconn_secure_set_size' .
./app/http/httpclient.c:            espconn_secure_set_size( ESPCONN_CLIENT, 5120 ); /* set SSL buffer size */
./app/modules/mqtt.c:  espconn_secure_set_size(ESPCONN_CLIENT, 4096);
./app/modules/net.c:      espconn_secure_set_size(ESPCONN_CLIENT, 5120); /* set SSL buffer size */

@devsaurus
Copy link
Member

who can describe this magic number?

Espressif most likely, it's part of their TCP SSL client example. That's the only reference I know of.

@djphoenix
Copy link
Contributor Author

OK, so let it be 5K. Will I make a test PR for this, or there will be further discussion?

@devsaurus
Copy link
Member

There appear to be no objections, so feel free to do the PR. We'll check then for any regressions.

@NicolSpies
Copy link

This might be applicable, I have been testing against the latest build for the last day with the mqtt-fix ( #1445 ) without any problems. I am not using MQTT secure.

@NicolSpies
Copy link

I am changing my previous comment of no problems as per #1406. From my observations the pattern that is emerging seems to indicate MQTT becoming unresponsive / instability when GPIO code is used and when a MQTT message is received.

@jmattsson
Copy link
Member

👍

For point 5, see in-progress #1379

@NicolSpies
Copy link

👍

@NicolSpies
Copy link

@marcelstoer @djphoenix I am not reopening this, only adding the cause of the problem experienced.

I implemented the MQTT with auto-reconnect and used the persistent session option (clean session false) in order to retain subscriptions to eliminate re-subscribing when auto-reconnecting.

This worked for a while until the offline and auto-reconnect stopped firing and the MQTT became unresponsive every time after about 2 minutes resulting in the lwt to fire. This has the effect that a fully operational MQTT session only survives about 2 minutes.

Fix: When a clean session (non persistent) connection is used, everything stays stable as expected.

I am sure this piece of my puzzle could be useful to someone.

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

No branches or pull requests

5 participants