ESP8266WebServer uses >2kb of stack when reinitialized in setup()

[chschu]

Basic Infos

Hardware

Hardware: ESP-12F on NodeMCU clone (Geekcreit Doit)
Core Version: 2.3.0

Description

When reassigning a ESP8266WebServer in the setup() function, e.g. to make it listen only on the AP IP address, a temporary instance is created on the stack. This requires more than 2 kilobytes of stack space, and is pretty likely to overrun the stack boundary at 4 kilobytes in complex setups. This may or may not be detected by cont_check, and lead to strange errors and WDT resets.

The reason for this huge stack allocation is the 2K buffer in HTTPUpload contained in a ESP8266WebServer instance.

Strictly speaking, this is NOT a bug, and the stack is properly released after setup() returns. Still, it is somewhat inconvenient considering the small total stack size. The code below illustrates some workarounds (CASE 2 and 3) using more complex C++ syntax, but it would be nicer to not have to write things like that.

Settings in IDE

Module: NodeMCU 1.0 (ESP-12E Module)
Flash Size: 4MB (3M SPIFFS)
CPU Frequency: 80Mhz
Upload Using: SERIAL

Sketch

#include "ESP8266WiFi.h"
#include "ESP8266WebServer.h"

extern "C" {
#include <cont.h>
  extern cont_t g_cont;
}

// CASE 0: no reinitialization
// CASE 1: reinitialize from an implicit temporary instance created on the stack
// CASE 2: reinitialize from an explicit temporary instance created on the heap
// CASE 3: reinitialize with "placement new" (no temporary instance)
#define CASE 1

#define WIFI_SSID "..."
#define WIFI_PASS "..."

ESP8266WebServer server;

void setup() {
  Serial.begin(115200);
  Serial.println();
  Serial.println();
  Serial.println();

  WiFi.begin(WIFI_SSID, WIFI_PASS);
  Serial.print("Connecting to WiFi.");
  while (WiFi.status() != WL_CONNECTED) {
    delay(1000);
    Serial.print(".");
  }
  Serial.println("OK");

#if CASE == 0
  /* no code */
#elif CASE == 1
  server = ESP8266WebServer(WiFi.softAPIP());
#elif CASE == 2
  ESP8266WebServer *tmp = new ESP8266WebServer(WiFi.softAPIP());
  server = *tmp;
  delete tmp;
#elif CASE == 3
  server.~ESP8266WebServer();
  new (&server) ESP8266WebServer(WiFi.softAPIP());
#endif

  server.begin();

  Serial.printf("unmodified stack   = %4d\n", cont_get_free_stack(&g_cont));
  register uint32_t *sp asm("a1");
  Serial.printf("current free stack = %4d\n", 4 * (sp - g_cont.stack));
}

void loop() {
  // do nothing
  delay(1000);
}

Debug Messages

CASE 0: no reinitialization

unmodified stack   = 3024
current free stack = 4064

CASE 1: reinitialize from an implicit temporary instance created on the stack

unmodified stack   =  736
current free stack = 1776

CASE 2: reinitialize from an explicit temporary instance created on the heap

unmodified stack   = 2992
current free stack = 4032

CASE 3: reinitialize with "placement new" (no temporary instance)

unmodified stack   = 3008
current free stack = 4048

The value of "unmodified stack" varies a bit even for the same CASE, which may be caused by some uninitialized variables. The value of "current free stack" is always the same for one CASE, as one would expect.

[devyte]

@chschu I find this awesome, thank you. I'm an experienced programmer, and I doubt that the stack usage of using a temporary would have occurred to me.
Your finding still holds in latest git. I'm not sure about a solution, though:

• The IP address in the Webserver constructor is passed to the _server member constructor, so changing it dynamically to avoid reconfiguration of the entire object implies constructing the _server dynamically
• The HTTPUpload struct could have the buffer created/destroyed dynamically, e.g.: on construct/destroy
• The HTTPUpload struct could have the buffer changed from a fixed size one to a managed object, like std::vector
• We coud just document the problem as a pitfall and list your possible workarounds.

[devyte]

@igrr what are your thoughts on this? The issue is real, but I'm not even sure whether I should label it as a bug.

[igrr]

It's definitely a bug. Not only this can cause a stack overflow, it also causes excessive RAM usage when the web server is allocated statically, even if file uploads are never used.

I would suggest changing the type of _currentUpload to std::unique_ptr, and allocating it in Parsing.cpp (where _currentUpload.status = UPLOAD_FILE_START; is done currently). Once the request is handled, _currentUpload can be cleared (in ESP8266WebServer::handleClient function).

Note, i would also refactor handleClient to eliminate multiple exit paths, many of which do the same thing:

      _currentClient = WiFiClient();
      _currentStatus = HC_NONE;

(and now they would also be doing _currentUpload.reset();)

@chschu, i can leave the fix to you if you like. Otherwise let me know and i will prepare a PR.

[chschu]

@igrr, I'll provide a PR over the weekend.

[chschu]

@igrr, I just noticed that you probably wanted the whole _currentUpload to be a wrapped inside a std::unique_ptr, while I just did it for buf. Wrapping the whole struct eliminates the nasty upload.buf.get() in the examples. I'll change that.

[d-a-v]

@chschu Since the PR is merged, can this issue be closed ?

[chschu]

@d-a-v Yes of course. Thanks for the nudge.