WiFi UDP misuses new/delete and crashes

[davepl]

Board

All (eg: HeltecWifiKit32)

Device Description

All

Hardware Configuration

All with WiFi

Version

latest development Release Candidate (RC-X)

IDE Name

VSCode

Operating System

FreeRTOS

Flash frequency

40

PSRAM enabled

no

Upload speed

115200

Description

WiFiUdp::parsePacket makes extensive use of new/delete while checking the return value. It crashes in low memory conditions because new doesn't return null, it throws an exception, which is not caught. They could just specify __nothrow but they don't. Or it could be replaced with malloc/free.

I've fixed it privately, which has corrected this crash for me at least, and would like to fix it in the original.

int WiFiUDP::parsePacket(){
  if(rx_buffer)
    return 0;
  struct sockaddr_in si_other;
  int slen = sizeof(si_other) , len;
  char * buf = new char[1460];
  if(!buf){
    return 0;
  }
  if ((len = recvfrom(udp_server, buf, 1460, MSG_DONTWAIT, (struct sockaddr *) &si_other, (socklen_t *)&slen)) == -1){
    delete[] buf;
    if(errno == EWOULDBLOCK){
      return 0;
    }
    log_e("could not receive data: %d", errno);
    return 0;
  }
  remote_ip = IPAddress(si_other.sin_addr.s_addr);
  remote_port = ntohs(si_other.sin_port);
  if (len > 0) {
    rx_buffer = new cbuf(len);
    rx_buffer->write(buf, len);
  }
  delete[] buf;
  return len;
}

Here’s an example from the log:

  #0  0x40084e09:0x3ffea2e0 in panic_abort at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/esp_system/panic.c:402
  #1  0x4008f1f1:0x3ffea300 in esp_system_abort at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/esp_system/esp_system.c:128
  #2  0x40094c71:0x3ffea320 in abort at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/newlib/abort.c:46
  #3  0x400d89af:0x3ffea3a0 in TerminateHandler() at src/main.cpp:471
  #4  0x40165fab:0x3ffea3d0 in __cxxabiv1::__terminate(void (*)()) at /builds/idf/crosstool-NG/.build/HOST-x86_64-apple-darwin12/xtensa-esp32-elf/src/gcc/libstdc++-v3/libsupc++/[eh_terminate.cc:47](http://eh_terminate.cc:47/)
  #5  0x40166012:0x3ffea3f0 in std::terminate() at /builds/idf/crosstool-NG/.build/HOST-x86_64-apple-darwin12/xtensa-esp32-elf/src/gcc/libstdc++-v3/libsupc++/[eh_terminate.cc:57](http://eh_terminate.cc:57/)
  #6  0x40166e27:0x3ffea410 in __cxa_throw at /builds/idf/crosstool-NG/.build/HOST-x86_64-apple-darwin12/xtensa-esp32-elf/src/gcc/libstdc++-v3/libsupc++/[eh_throw.cc:95](http://eh_throw.cc:95/)
  #7  0x401668ea:0x3ffea430 in operator new(unsigned int) at /builds/idf/crosstool-NG/.build/HOST-x86_64-apple-darwin12/xtensa-esp32-elf/src/gcc/libstdc++-v3/libsupc++/[new_op.cc:54](http://new_op.cc:54/)
  #8  0x40166e81:0x3ffea450 in operator new[](unsigned int) at /builds/idf/crosstool-NG/.build/HOST-x86_64-apple-darwin12/xtensa-esp32-elf/src/gcc/libstdc++-v3/libsupc++/[new_opv.cc:32](http://new_opv.cc:32/)
  #9  0x400e2ba1:0x3ffea470 in WiFiUDP::parsePacket() at /Users/dave/.platformio/packages/framework-arduinoespressif32/libraries/WiFi/src/WiFiUdp.cpp:210
  #10 0x400d7c96:0x3ffea4c0 in NTPTimeClient::UpdateClockFromWeb(WiFiUDP*) at include/ntptimeclient.h:109
  #11 0x40082357:0x3ffea590 in NetworkHandlingLoopEntry(void*) at src/main.cpp:392

Sketch

Should be obvious from inspection

Debug Message

#0  0x40084e09:0x3ffea2e0 in panic_abort at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/esp_system/panic.c:402
  #1  0x4008f1f1:0x3ffea300 in esp_system_abort at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/esp_system/esp_system.c:128
  #2  0x40094c71:0x3ffea320 in abort at /Users/ficeto/Desktop/ESP32/ESP32S2/esp-idf-public/components/newlib/abort.c:46
  #3  0x400d89af:0x3ffea3a0 in TerminateHandler() at src/main.cpp:471
  #4  0x40165fab:0x3ffea3d0 in __cxxabiv1::__terminate(void (*)()) at /builds/idf/crosstool-NG/.build/HOST-x86_64-apple-darwin12/xtensa-esp32-elf/src/gcc/libstdc++-v3/libsupc++/eh_terminate.cc:47
  #5  0x40166012:0x3ffea3f0 in std::terminate() at /builds/idf/crosstool-NG/.build/HOST-x86_64-apple-darwin12/xtensa-esp32-elf/src/gcc/libstdc++-v3/libsupc++/eh_terminate.cc:57
  #6  0x40166e27:0x3ffea410 in __cxa_throw at /builds/idf/crosstool-NG/.build/HOST-x86_64-apple-darwin12/xtensa-esp32-elf/src/gcc/libstdc++-v3/libsupc++/eh_throw.cc:95
  #7  0x401668ea:0x3ffea430 in operator new(unsigned int) at /builds/idf/crosstool-NG/.build/HOST-x86_64-apple-darwin12/xtensa-esp32-elf/src/gcc/libstdc++-v3/libsupc++/new_op.cc:54
  #8  0x40166e81:0x3ffea450 in operator new[](unsigned int) at /builds/idf/crosstool-NG/.build/HOST-x86_64-apple-darwin12/xtensa-esp32-elf/src/gcc/libstdc++-v3/libsupc++/new_opv.cc:32
  #9  0x400e2ba1:0x3ffea470 in WiFiUDP::parsePacket() at /Users/dave/.platformio/packages/framework-arduinoespressif32/libraries/WiFi/src/WiFiUdp.cpp:210
  #10 0x400d7c96:0x3ffea4c0 in NTPTimeClient::UpdateClockFromWeb(WiFiUDP*) at include/ntptimeclient.h:109
  #11 0x40082357:0x3ffea590 in NetworkHandlingLoopEntry(void*) at src/main.cpp:392

Other Steps to Reproduce

No response

I have checked existing issues, online documentation and the Troubleshooting Guide

• I confirm I have checked existing issues, online documentation and Troubleshooting guide.

[lbernstone]

It is pretty easy to submit a pull request by editing the file at https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFi/src/WiFiUdp.cpp

[mrengineer7777]

@davepl How did you fix it in your code? I think the correct fix here would be to add "__nothrow" to each "new" call. Also we should check if rx_buffer = new cbuf(len); succeeds. I'm comfortable submitting a PR if want help.

[davepl]

That’s what I did… std::nothrow on the allocations. I’ll put together a PR for someone to review!

…

On Dec 7, 2022, at 7:40 AM, David McCurley ***@***.***> wrote: @davepl <https://github.com/davepl> How did you fix it in your code? I think the correct fix here would be to add "__nothrow" to each "new" call. Also we should check if rx_buffer = new cbuf(len); succeeds. I'm comfortable submitting a PR if want help. — Reply to this email directly, view it on GitHub <#7558 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCFZTXEU4TEHZ24PZ3M3WMCVXVANCNFSM6AAAAAASV4UPKM>. You are receiving this because you were mentioned.

[VojtechBartoska]

Hello @davepl, thanks for your work on the PR. We really appreciate it!

[BOBAH1248]

According to c++ specification, the construction like

    SomeClass *pSc = new SomeClass;
    if ( NULL == pSc )
    {
        // cope with error
    }

include new [] will not work, because new operator in c++ throws a std::bad_alloc exception instead of returning NULL.
But in the libraries we can see everywhere such
https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFi/src/WiFiUdp.cpp#L45-L46
https://github.com/espressif/arduino-esp32/blob/master/libraries/WebServer/src/WebServer.cpp#L148-L154
and so

PS: The library.properties say, that such mistakes should be forwarded to authors

author=Ivan Grokhotkov
maintainer=Ivan Grokhtkov <...@esp8266.com>

author=Hristo Gochkov
maintainer=Hristo Gochkov <...@espressif.com>

[TD-er]

For this you can simply use:

#include <new>

    SomeClass *pSc = new (std::nothrow) SomeClass;

It can also be used on allocating arrays on the heap: https://en.cppreference.com/w/cpp/memory/new/nothrow

The only disadvantage of this (as far as I know) is that MS Intellisense does not recognize this and thus shows you a red underline in VS-code.
I use it in my code a lot and I see no reason why it couldn't be used in these classes.

[BOBAH1248]

Agreed with @TD-er - I already patched it in my instance.
But I use PlatformIO for managing of dependencies and afraid that it may update packages (include framework-arduinoespressif32) and roll back my patches.
As result, I may again get buggy code.

[mrengineer7777]

@davepl @BOBAH1248 @TD-er PR 7847 closes this issue.
The nothrow issue will be tracked in #7849. Please make your comments there. I will be submitting a PR to fix it.

[BOBAH1248]

@mrengineer7777 probably your commit related to ESP8266 repo, but not ESP32
While PR 7847 just replace few new-delete on malloc-free