vintlabs / fauxmoESP

Add voice control of your ESP32 and ESP8266 devices using Amazon Alexa
MIT License
379 stars 70 forks source link

dangling pointers when writing to TCP on ESP8266 #160

Closed bertmelis closed 3 years ago

bertmelis commented 3 years ago

ESPAsyncTCP doesn't add the flag to copy the data when writing to TCP whereas ESP32 does. This causes dangling pointers. Easiest solution is to add ASYNC_WRITE_FLAG_COPY to the client->write(...) statements.

example:

client->write(headers, strlen(headers), ASYNC_WRITE_FLAG_COPY);
client->write(body, strlen(body), ASYNC_WRITE_FLAG_COPY);

API for ESP8266: https://github.com/me-no-dev/ESPAsyncTCP/blob/15476867dcbab906c0f1d47a7f63cdde223abeab/src/ESPAsyncTCP.h#L225 API for ESP32: https://github.com/me-no-dev/AsyncTCP/blob/ca8ac5f919d02bea07b474531981ddbfd64de97c/src/AsyncTCP.h#L82

mcspr commented 3 years ago

I hate to bring this up again for the exact same reason, but... it is still worth mentioning: https://git.savannah.nongnu.org/cgit/lwip.git/tree/src/core/tcp_out.c?h=STABLE-2_1_2_RELEASE#n422 https://github.com/d-a-v/esp82xx-nonos-linklayer/blob/7421258237b7c8f61629226961af498a0a6e0096/glue-lwip/arduino/lwipopts.h#L1665 tcp_write will helpfully modify the apiflags set to include the COPY flag before jumping to the actual sending

e.g. doing something like this:

void loop() {
    auto* ptr = new char[128];
    std::fill(ptr, ptr + 128, 'a');
    client->write(ptr, 128);
    std::fill(ptr, ptr + 128, 'x');
    Serial.println(ptr);
    delay(1000);

should still send 128 'a's

esp32 seems to include the same flag, but I am not sure of the differences between socket and raw apis in this regard (although, iirc both use the same underlying tcp_write. And I hope I don't completely misunderstand the issue here) https://github.com/espressif/esp-idf/blob/6e776946d01ec0d081d09000c36d23ec1d318c06/components/lwip/port/esp32/include/lwipopts.h#L449

It is still probably a nice practice to have explicit COPY flag, so if someone copies the code with a different lwip config there are no surprising changes to behaviour.

bertmelis commented 3 years ago

IS this the same in V1 of LWIP?

bertmelis commented 3 years ago

Nevermind, got it.

Strangely, nobody ever directed me to this very source file.

mcspr commented 3 years ago

btw I thought I was answering in the https://github.com/marvinroger/async-mqtt-client/issues/219 ... will follow up there in the future :oops: This was mentioned originally here - https://github.com/philbowles/PangolinMQTT/issues/2 - since the author experienced the issue with the SMT32 port's config, but did not clearly state this in the associated documentation and code comments :/

lwipv1 should not be considered for anything atm, upstream removed support completely so only v2 remains for both platforms.

bertmelis commented 3 years ago

Got it.

You're welcome :wink: