search

warmcat / lws-esp32-factory

Libwebsockets ESP32 Factory Application
Other
80 stars 24 forks source link

struct lws_esp32 too small to hold long SSID or Password #25

Closed FredrikFornstad closed 6 years ago

FredrikFornstad commented 6 years ago

SSID and WiFi passwords can be 32 characters long, but in libwebsockets.h row 568 - 571 smaller values have been used (I guess the NULL character needs to be stored as well...)

I have an AP with SSID containing 14 characters and Password containing 31 characters (excl NULL). lws.esp32.password[4][32] <-- This does NOT work lws.esp32.password[4][33] <-- This works

I can not really understand why, since my 31 character password would need 32 if just one NULL is added to the string.

With the original definition of [32], I can see that only the SSID is stored in nvs and nothing (or just NULL) for password. The result is that lws goes into cyclic (about 5 per second) STA-DISCONNECT after configuring the AP in slot 0 and rebooting.

While trouble shooting for this I could also see that the storage for lws.esp32.ssid , lws.esp32.active_ssid and lws.esp32.access_pw also seem to be too small.

Just for reference: Looking at esp-idf and their wifi_types_h I can see that they are using uint8_t password[64] in wifi_sta_config_t , This seems waste of space and I guess it can also be dangerous in memcpy using sizeof... For SSID they use [32] in sta_config and ap_config, but [33] for wifi_ap_record_t , but maybe they store NULL in the wifi_ap_record_t, I have not investigated that.

lws-team commented 6 years ago

OK... in lws I cranked esp32.ssid / password / active_ssid to all be [64], along with a temp array used in the scan protocol. I updated -factory and the -test-server-demos to use the new lws.

Please give it a try.

FredrikFornstad commented 6 years ago

Now connecting to my AP as it should. It also connects after reboot as well without going cyclic needing a erase_flash as it did before.

Not sure if this fix introduced other problems, but the lws-esp32-factory is not as robust as I would have believed. I will try to enter my findings in separate issues so you can deal with them one by one if/when you have time. Please let me know if I start to become a pain and then I will back off a bit... :-)

Overall, I find the whole lws-esp32 app REALLY nice, and if it works as I think it does, it will be perfect for my Little Project of connecting/monitoring a machine with a few sensors (using an odd serial interface) and a few switches. But your code is extensive and I must admit that I get lost in it. It can be me who is not an expert in programming, but some additional high level comments here and there would be of much help. Also, it could maybe have a bit more modular structure (or maybe it is just me who has not understood the structure yet...).

lws-team commented 6 years ago

Not sure if this fix introduced other problems

Doubt it. esp-idf changing all the time "introduces other problems".

lws and this ESP32 stuff is FOSS... I don't have any relationship to Espressif and nobody pays me to look after it or be "tech support". So I would hope to get fixes contributed back.

it could maybe have a bit more modular structure

If you feel that's stopping you sending patches, send patches for that :-)