Repository navigation
esp8266: Passing strings that live in flash ROM to socket functions/methods leads to crash #2066
Description
The following lines will both lead to a hard crash on esp8266:
socket.getaddrinfo('', 80)
s.send('micropython') # assuming s is a connected socket
The reason is that they pass strings that live in flash ROM to functions in lwip, and lwip loads data byte-wise (or calls functions that do).
Activity
pfalcon commentedon May 11, 2016
Yes, I wanted to submit this too. To clarify, "strings that live in flash ROM" == string matching those which are pre-packaged in ROM as qstrs, so it's pretty big issue.
The obvious solution is to try to build lwip with -mforce-l32, but actual cases are probably due to usage of functions from bootrom which don't support it still.
dpgeorge commentedon May 11, 2016
Yes, that's one solution. The getaddrinfo example faults in dns_getaddrinfo because it accesses host[0]. But the send example above faults in bootrom (memcpy it seems... very strange).
Other solution is to have a buffer/string accessor function which copies the data to RAM if the source is in flash ROM, and use such an accessor in modlwip.c for all data that is passed to underlying lwip code. But that seems rather hacky (and bad for memory usage).
pfalcon commentedon May 11, 2016
Nothing strange, vendor-patched lwip source just has
#define memcpy os_memcpysomewhere.No, the proper solution is to write memcpy and friends which always read source as uint32_t (efficiently I mean, not like -mforce-l32 does).
Patching lwip is in my queue.
pfalcon commentedon May 13, 2016
As expected, -mforce-l32 alone doesn't fix it, while stopping to use os_memcpy() alone does. So, I set it as such to avoid -mforce-l32 overhead in lwip. Branch sdk-1.5.0-experimental of esp-open-sdk. Please test if it works for you. I tested by uncommenting this line: https://github.com/micropython/micropython/blob/master/esp8266/scripts/webrepl_setup.py#L29
dpgeorge commentedon May 13, 2016
I don't see this branch...
pfalcon commentedon May 13, 2016
Sorry, I meant of open-lwip: https://github.com/pfalcon/esp-open-lwip/commits/sdk-1.5.0-experimental
pfalcon commentedon May 15, 2016
esp-open-sdk:master switched to esp-open-lwip:sdk-1.5.0-experimental.
dpgeorge commentedon May 16, 2016
Ok, I have latest esp-open-sdk with newest esp-open-lwip. I can confirm that
s.send('micropython')now works. Butsocket.getaddrinfo('', 80)still crashes, due to this line: https://github.com/pfalcon/esp-open-lwip/blob/sdk-1.5.0/lwip/core/dns.c#L952pfalcon commentedon May 16, 2016
So, to enable native lwIP debugging, it needs to be built with -mforce-l32, and currently it takes patches both it and upy to switch it back and forth. So, least-resistance solution would be just enable it all-time.
dpgeorge commentedon May 16, 2016
+1
pfalcon commentedon May 22, 2016
Ok, now done (esp-open-sdk master), but it won't do you much good:
This is consistent with Linux usocket, so "" is Python convention and needs to be handled on the level of modsocket.
dpgeorge commentedon May 23, 2016
Thanks for building lwip with -mforce-l32.
Actually, CPython doesn't handle "" empty string to getaddrinfo either. It was just a simple test case that showed the problem with ROM strings.
And there are still problems, but different ones. Try the following (with latest build at 3e03d1b):
(If this doesn't crash for you, try other interned strings until it does.)
It's crashing at 0x4000bf64 which is a boot ROM function.
dpgeorge commentedon May 23, 2016
This is strlen. lwip is using os_strlen (in core/dns.c). I checked that by replacing os_strlen with just strlen that getaddrinfo no longer crashes with ROM strings.
pfalcon commentedon May 26, 2016
There're gazillion of os_strlen in the vendor lwip, few in dns.c alone. I changes all there, but reluctant to touch other files and carry random deltas. Let it wait while vendor patch will be rebased on pristine lwip (and catch any issues manually, yeah). esp-open-sdk updated.
dpgeorge commentedon Jun 1, 2016
I can confirm that with the latest esp-open-sdk this is fixed, at least for all the tests I can run.
So now I'm able to freeze webrepl (along with other core scripts) via #2067.
Merge pull request micropython#2066 from dhalbert/ble-pairing