Skip to content

Webserver double freeing _currentHeaders on destruction #4350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mongozmaki opened this issue Feb 11, 2018 · 4 comments
Closed

Webserver double freeing _currentHeaders on destruction #4350

mongozmaki opened this issue Feb 11, 2018 · 4 comments

Comments

@mongozmaki
Copy link
Contributor

mongozmaki commented Feb 11, 2018

Destructor of ESP8266WebServer double-frees _currentHeaders.
The problem is, that the destructor frees _currentHeaders and sets _headerKeysCount=0 and than calls close(), which checks if(!_headerKeysCount) (=true) and calls collectHeaders(0, 0) which frees _currentHeaders again (because it is not set to nullptr).
A fix would be to call close() in the beginning of the destructor.
A more extensive fix would be to call collectHeaders(0, 0) in begin() and not in close().

Code fixed:

ESP8266WebServer::~ESP8266WebServer() {
  //Close here
  close();
  if (_currentHeaders) {
    delete[]_currentHeaders;
  }
  _headerKeysCount = 0;
  RequestHandler* handler = _firstHandler;
  while (handler) {
    RequestHandler* next = handler->next();
    delete handler;
    handler = next;
  }
  //Not here
  //close();
}

Sketch

#include <Arduino.h>

void setup() {
Serial.begin(115200);
  {
    Serial.println("Creating server");
    ESP8266WebServer server;
    const char * headerkeys[] = { "If-None-Match" };
    Serial.println("Setting headers");
    //Crashes also without calling collectHeaders
    server.collectHeaders(headerkeys, 1);
    Serial.println("Begin server");
    server.begin();
    Serial.println("About to delete server");
  } //Deleted here
  Serial.println("Server deleted");
}

void loop() {
}

Debug Messages

Creating server
Setting headers
Begin server
About to delete server
*** CRASH ***
@d-a-v
Copy link
Collaborator

d-a-v commented Feb 13, 2018

That would make a good pull-request.

@earlephilhower
Copy link
Collaborator

@mongozmaki Would you like to submit a PR for this?

It will also fix the same problem on the SecureServer, which uses this class' destructor.

earlephilhower added a commit to earlephilhower/Arduino that referenced this issue Feb 14, 2018
In issue esp8266#4350, @mongozmaki found that the web server was accessing a
deleted variable in the destructor.  Implement his suggested change
and move the close() before any freeing.  Could also have simply
NULL'd out the _currentHeaders member after freeing as well.

Fixes issue esp8266#4350
@mongozmaki
Copy link
Contributor Author

Yes, I tried creating a PR but didn't work for some reason. If I find a minor issue again, I will do that.
Thanks for the fix!

jp112sdl added a commit to jp112sdl/WemosD1_HomeMatic_WiFiSensor that referenced this issue Feb 14, 2018
earlephilhower added a commit that referenced this issue Feb 14, 2018
In issue #4350, @mongozmaki found that the web server was accessing a
deleted variable in the destructor.  Implement his suggested change
and move the close() before any freeing.  Could also have simply
NULL'd out the _currentHeaders member after freeing as well.

Fixes issue #4350
@earlephilhower
Copy link
Collaborator

No problem, @mongozmaki . I've referenced you in the change note, and appreciate the detailed error, repro instructions, and fix you did. Nothing for me to do, really. :)

Next one, when you push to your private forked repo, you can go to its github website and there should be a "submit pull request" button right at the top of the page (for about 5 mins after a push) which should get you going.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants