WiFiManagerParameter does not handle move constructor

[axlan]

Hardware

WiFimanager Branch/Release: Master commit 810f144

Esp8266/Esp32:

Hardware: D1-mini lite

Description

WiFiManagerParameter class does not properly handle the C++ move constructor resulting in a use after free error.

Reproduction Example

#include <vector>

#include <WiFiManager.h> // https://github.com/tzapu/WiFiManager

WiFiManager wm;
std::vector<WiFiManagerParameter> parameters;

void setup() {
    WiFi.mode(WIFI_STA); // explicitly set mode, esp defaults to STA+AP    
    // put your setup code here, to run once:
    Serial.begin(115200);

    // Allocate new space and construct parameter in it
    parameters.emplace_back("id1", "field1", "", 40);
    // Allocate new space, move "id1" and construct parameter after it
    parameters.emplace_back("id2", "field2", "", 40);

    // Add parameters to WiFiManager
    for (auto& param : parameters)
    {
      wm.addParameter(&param);
    }

    wm.setConfigPortalBlocking(false);
    //automatically connect using saved credentials if they exist
    //If connection fails it starts an access point with the specified name
    if(wm.autoConnect("AutoConnectAP")){
        Serial.println("connected...yeey :)");
    }
    else {
        Serial.println("Configportal running");
    }

    wm.startConfigPortal();
}

void loop() {
    wm.process();
}

I added a log message to record the alloc/free's:

new char allocated id1
new char allocated id2
char freed id1
*wm:[2] Added Parameter: id1
*wm:[2] Added Parameter: id2

Solution

What's happening here is that the std::vector is calling the implicit move constructor: https://en.cppreference.com/w/cpp/language/move_constructor

This constructor doesn't correctly handle moving the _value data which is then freed. A similar issue is presumably why the copy constructor is made private.

There are three fixes that I see.

1. Delete the move constructor. My example doesn't build if I add: WiFiManagerParameter(const WiFiManagerParameter&&) = delete;
2. Implement a move (and optionally a copy) constructor. This is fairly straightforward and just needs to correctly propagate the pointer to the new instance and set the old instance to NULL so it isn't freed.
3. Switch to using smart pointers. I took this approach for a fork I made for testing. Here's the code:

class WiFiManagerParameter {
  public:
    /** 
        Create custom parameters that can be added to the WiFiManager setup web page
        @id is used for HTTP queries and must not contain spaces nor other special characters
    */
    WiFiManagerParameter() = default;
    WiFiManagerParameter(const char *custom);
    WiFiManagerParameter(const char *id, const char *label);
    WiFiManagerParameter(const char *id, const char *label, const char *defaultValue, int length);
    WiFiManagerParameter(const char *id, const char *label, const char *defaultValue, int length, const char *custom);
    WiFiManagerParameter(const char *id, const char *label, const char *defaultValue, int length, const char *custom, int labelPlacement);

    const char *getID() const;
    const char *getValue() const;
    const char *getLabel() const;
    const char *getPlaceholder() const; // @deprecated, use getLabel
    int         getValueLength() const;
    int         getLabelPlacement() const;
    virtual const char *getCustomHTML() const;
    void        setValue(const char *defaultValue, int length);

  protected:
    void init(const char *id, const char *label, const char *defaultValue, int length, const char *custom, int labelPlacement);

  private:
    const char *_id = nullptr;
    const char *_label = nullptr;
    std::unique_ptr<char[]> _value;
    int         _length = 1;
    int         _labelPlacement = WFM_LABEL_BEFORE;
  protected:
    const char *_customHTML = "";
    friend class WiFiManager;
};


WiFiManagerParameter::WiFiManagerParameter(const char *custom) {
  _customHTML     = custom;
}

WiFiManagerParameter::WiFiManagerParameter(const char *id, const char *label) {
  init(id, label, "", 0, "", WFM_LABEL_BEFORE);
}

WiFiManagerParameter::WiFiManagerParameter(const char *id, const char *label, const char *defaultValue, int length) {
  init(id, label, defaultValue, length, "", WFM_LABEL_BEFORE);
}

WiFiManagerParameter::WiFiManagerParameter(const char *id, const char *label, const char *defaultValue, int length, const char *custom) {
  init(id, label, defaultValue, length, custom, WFM_LABEL_BEFORE);
}

WiFiManagerParameter::WiFiManagerParameter(const char *id, const char *label, const char *defaultValue, int length, const char *custom, int labelPlacement) {
  init(id, label, defaultValue, length, custom, labelPlacement);
}

void WiFiManagerParameter::init(const char *id, const char *label, const char *defaultValue, int length, const char *custom, int labelPlacement) {
  _id             = id;
  _label          = label;
  _labelPlacement = labelPlacement;
  _customHTML     = custom;
  setValue(defaultValue,length);
}

// WiFiManagerParameter& WiFiManagerParameter::operator=(const WiFiManagerParameter& rhs){
//   Serial.println("copy assignment op called");
//   (*this->_value) = (*rhs._value);
//   return *this;
// }

// @note debug is not available in wmparameter class
void WiFiManagerParameter::setValue(const char *defaultValue, int length) {
  if(!_id){
    // Serial.println("cannot set value of this parameter");
    return;
  }
  
  // if(strlen(defaultValue) > length){
  //   // Serial.println("defaultValue length mismatch");
  //   // return false; //@todo bail 
  // }

  _length = length;
  _value  = std::make_unique<char[]>(_length + 1);
  memset(_value.get(), 0, _length + 1); // explicit null
  
  if (defaultValue != NULL) {
    strncpy(_value.get(), defaultValue, _length);
  }
}
const char* WiFiManagerParameter::getValue() const {
  // Serial.println(printf("Address of _value is %p\n", (void *)_value)); 
  return _value.get();
}
const char* WiFiManagerParameter::getID() const {
  return _id;
}
const char* WiFiManagerParameter::getPlaceholder() const {
  return _label;
}
const char* WiFiManagerParameter::getLabel() const {
  return _label;
}
int WiFiManagerParameter::getValueLength() const {
  return _length;
}
int WiFiManagerParameter::getLabelPlacement() const {
  return _labelPlacement;
}
const char* WiFiManagerParameter::getCustomHTML() const {
  return _customHTML;
}

Note: the copy constructor is implicitly deleted by the inclusion of the unique_ptr.

A simple copy constructor could be implemented with:

WiFiManagerParameter::WiFiManagerParameter(const WiFiManagerParameter& other) {
  Serial.println("copy assignment op called");
  _id             = other._id;
  _label          = other._label;
  _labelPlacement = other._labelPlacement;
  _customHTML     = other._customHTML;
  setValue(other.getValue(), other._length);
}

The only change to the rest of the project was replacing the line:

value.toCharArray(_params[i]->_value, _params[i]->_length+1); // length+1 null terminated
with
value.toCharArray(_params[i]->_value.get(), _params[i]->_length+1); // length+1 null terminated

[tablatronix]

I see no issues switching to smart pointers, I might just need someone else to review this

[tablatronix]

here is the issue for that change to copy operator

#1050

[tablatronix]

FYI this is applied to branch
https://github.com/tzapu/WiFiManager/tree/parameter-smart-ptr

anyone test this?

[tablatronix]

BUMP