Content-Type "multipart/form-data" in POST request occasionally crashes WiFiWebServer

[bizprof]

Describe the bug

Very random bug that I can't even seem to reproduce at the moment. Was using XMLHttpRequest to submit form data from client to server via POST. Form data is stored in a FormData object. As plain vanilla as it gets, no extra headers etc. But request did not get parsed correctly by the server when (by default) Content-Type was set to "multipart/form-data". When switching to Content-Type "application/x-www-form-urlencoded" the POST worked as expected.

Steps to Reproduce

The relevant code snippets are for the client (taken from https://developer.mozilla.org/en-US/docs/Learn/Forms/Sending_forms_through_JavaScript): //////////////////////////////////////////////////////

Click Me!

//////////////////////////////////////////////////////

The server used was WiFiWebServer, called from SAMD_WiFiNINA.ino in the WiFiManager_NINA_Lite examples. Server connected to local WiFi, both WIFI and WIFININA debug levels set to 4.

Expected behavior

Expect the submitted test data (value pair "test" and "ok") to show up in the debug log, like it does when using "application/x-www-form-urlencoded", see log below:

[WIFI] handleClient: New Client [WIFI] method: POST [WIFI] url: / [WIFI] search:
[WIFI] headerName: Host [WIFI] headerValue: 192.168.0.29 [WIFI] headerName: User-Agent [WIFI] headerValue: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:88.0) Gecko/20100101 Firefox/88.0 [WIFI] headerName: Accept [WIFI] headerValue: / [WIFI] headerName: Accept-Language [WIFI] headerValue: en-GB,en;q=0.5 [WIFI] headerName: Accept-Encoding [WIFI] headerValue: gzip, deflate [WIFI] headerName: Content-Type [WIFI] headerValue: application/x-www-form-urlencoded [WIFI] headerName: Content-Length [WIFI] headerValue: 175 [WIFI] headerName: Origin [WIFI] headerValue: null [WIFI] headerName: Connection [WIFI] headerValue: keep-alive [WIFI] headerName: Sec-GPC [WIFI] headerValue: 1 [WIFI] headerName: Pragma [WIFI] headerValue: no-cache [WIFI] headerName: Cache-Control [WIFI] headerValue: no-cache [WIFI] args: -----------------------------276130166930317460881212043603 Content-Disposition: form-data; name="test"

ok -----------------------------276130166930317460881212043603--

[WIFI] args count: 2 [WIFI] args: -----------------------------276130166930317460881212043603 Content-Disposition: form-data; name="test"

ok

Actual behavior

See log below. The request did not go past the parser, but got stuck somewhere in Parsing-impl.h.
Server then became unresponsive, and with it the whole board.

Debug log

[WIFI] handleClient: New Client [WIFI] method: POST [WIFI] url: / [WIFI] search:
[WIFI] headerName: Host [WIFI] headerValue: 192.168.0.29 [WIFI] headerName: User-Agent [WIFI] headerValue: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:88.0) Gecko/20100101 Firefox/88.0 [WIFI] headerName: Accept [WIFI] headerValue: / [WIFI] headerName: Accept-Language [WIFI] headerValue: en-GB,en;q=0.5 [WIFI] headerName: Accept-Encoding [WIFI] headerValue: gzip, deflate [WIFI] headerName: Content-Type [WIFI] headerValue: multipart/form-data; boundary=---------------------------180992477013271867871486342273 [WIFI] headerName: Content-Length [WIFI] headerValue: 174 [WIFI] headerName: Origin [WIFI] headerValue: null [WIFI] headerName: Connection [WIFI] headerValue: keep-alive [WIFI] headerName: Sec-GPC [WIFI] headerValue: 1 [WIFI] args:
[WIFI] args count: 0 [WIFI] args:
[WIFI] args count: 0 [WIFI] Parse Form: Boundary: ---------------------------180992477013271867871486342273 [WIFI] Length: 174 ...and this is where we become stuck!

Other observations

I did look at the HTTP request details (Request payload section) in the Firefox Network debugger.

In the --...---xx... Boundary line, after the last number, a space and two double quotes showed up ( ""), which looked odd and out of place. Too bad, I can't reproduce this at the moment and provide a matching screen shot. And it does not show in the log above, either. At an earlier time, the last message on the console would be "Error: line: ...", and it would only then go unresponsive. That led me to look at Parsing-impl.h, where this message must be generated in method WiFiWebServer::_parseForm.

Maybe the extra quotes are mistaken by the parser for the start of a key/data value? I'm not sure, and I'm sorry I can't even reproduce it at the moment at all. Hope any of this makes some sense to you.

Information

• Arduino IDE version: 1.8.13 on Windows 10
• Board: Arduino NANO 33 IoT with latest firmware

[khoih-prog]

Hi,

Wow, so great to get your bug report, again with the same impressive details. Will look at it very soon.

Thanks and Regards,

[khoih-prog]

Hi,

The MPUs' software and library are not designed to be complex with all the features as the normal PC ones (lack of power, memory, etc.) . Sometimes, the memory limitation (heap / stack overflow) can cause hard-to-duplicate issues, especially if we send too much more data than the MPU can handle.

Currently, the multipart/form-data header type is not fully tested / supported yet, but the application/x-www-form-urlencoded has already been supported. Hopefully we can find out where the issue is and fix it.

Parsing-impl.h#L717-L721

      if (headerName.equalsIgnoreCase("Content-Type"))
      {
        using namespace mime;

        if (headerValue.startsWith(mimeTable[txt].mimeType))
        {
          isForm = false;
        }
        else if (headerValue.startsWith("application/x-www-form-urlencoded"))
        {
          isForm = false;
          isEncoded = true;
        }
        else if (headerValue.startsWith("multipart/"))
        {
          boundaryStr = headerValue.substring(headerValue.indexOf('=') + 1);
          // KH
          boundaryStr.replace("\"", "");
          //
          isForm = true;
        }
      }

It's possible we're stuck in an infinite loop (???) here in Parsing-impl.h _parseForm().

  do
  {
    line = client.readStringUntil('\r');        <============ the culprit possibly is readStringUntil() of Stream library
    ++retry;
  } while (line.length() == 0 && retry < 3);

---

For example, the sample code of Stream lib

String Stream::readStringUntil(char terminator, size_t max)
{
  String ret;
  int c = timedRead();
  while (c >= 0 && c != terminator)
  {
    ret += (char)c;
    c = timedRead();
  }

  return ret;
}

It's helpful if you can provide the full source code so that anybody can easily rerun and test. If you have time, please also try to duplicate and nail the issue down. I currently have no idea yet.

[bizprof]

Hi Khoi, Thanks for looking into this. I have today encountered the issue again, and did some digging. Plastered Parsing-impl.h with a million WS_LOGDEBUGs, basically. Turns out, the problem is caused by this very innocent looking statement:

if (_postArgs)
delete[] _postArgs;   ----> **we don't get past this point!**

This is supposed to call the destructor for _postArgs, an array of "RequestArgument" structures, each with 2 strings (key and value) inside.

As soon as I comment out the delete[] _postArgs; statement, the script works fine and correctly parses all parameters.

The delete[] must be there for good reason, though. It then dawned on me that _postArgs is actually just a pointer to the array of structures, so if (_postArgs) would always be true, even when _postArgs points to nothing. Changing the if statement to if (_postArgs != nullptr) seems to do the trick. So, I think the issue is caused by trying to delete _postArgs when it hasn't been instantiated yet. Which happens right after processing the POST headers, when no key/value pairs have yet been processed.

I will run with my mod for a while and see whether that fixes the issue for good. Have changed all the other delete[] sections, too, as they are similar.

[khoih-prog]

Wow, such great findings after a lot of works :+1: . I'll have to spend time to look why it's there. Please have more tests to be sure it's the only culprit, or there are more. When you're sure it's working reliably, please make a PR.

[bizprof]

Will do. Looks like I need to change a bit more, explicitely assigning nullptr to newly instatiated object pointers, and also after deleting them...

[khoih-prog]

explicitely assigning nullptr to newly instatiated object pointers, and also after deleting them...

This is a must, but sometimes we forget to do it correctly. It takes your time and devotion to find out. Thanks.

[bizprof]

Just submitted a PR. That should take care of this issue. In any case, adding = nullptr to the pointer instantiation in WifiWebServer won't do any harm for the other variables either. No changes needed in Parsing-impl!

[khoih-prog]

Already merged the PR.

Can you also help check the remaining code using delete, delete [], or new and modify using nullptr and nullptr check as well before we can publish new release.

Parsing-impl.h:    delete[] _currentArgs;
Parsing-impl.h:    delete[] _currentArgs;
Parsing-impl.h:      delete[] _postArgs;
Parsing-impl.h:      delete[] _currentArgs;
Parsing-impl.h:      delete[] _postArgs;
Parsing-impl.h:    if (_currentArgs) delete[] _currentArgs;
Parsing-impl.h:      delete[] postArgs;
WiFiWebServer-impl.h:    delete[]_currentHeaders;
WiFiWebServer-impl.h:    delete handler;
WiFiWebServer-impl.h:        delete[] toencode;
WiFiWebServer-impl.h:        delete[] toencode;
WiFiWebServer-impl.h:        delete[] encoded;
WiFiWebServer-impl.h:      delete[] toencode;
WiFiWebServer-impl.h:      delete[] encoded;
WiFiWebServer-impl.h:    delete [] buffer;
WiFiWebServer-impl.h:    delete[]_currentHeaders;

Parsing-impl.h:      buf = newBuf;
Parsing-impl.h:    dataLength += newLength;
Parsing-impl.h:  _currentArgs = new RequestArgument[_currentArgCount + 1];
Parsing-impl.h:    _currentArgs = new RequestArgument[1];
Parsing-impl.h:  _currentArgs = new RequestArgument[_currentArgCount + 1];
Parsing-impl.h:    _postArgs = new RequestArgument[WEBSERVER_MAX_POST_ARGS];
Parsing-impl.h:              _currentUpload = new HTTPUpload();
Parsing-impl.h:    _currentArgs = new RequestArgument[_postArgsLen];
Parsing-impl.h:    RequestArgument* postArgs = new RequestArgument[32];
Parsing-impl.h:    _currentArgs = new RequestArgument[postArgsLen];
utility/RingBuffer.cpp:  ringBuf = new char[size + 1];
WiFiWebServer-impl.h:      char *toencode = new char[toencodeLen + 1];
WiFiWebServer-impl.h:      char *encoded = new char[base64_encode_expected_len(toencodeLen) + 1];
WiFiWebServer-impl.h:  uint8_t* buffer = new uint8_t[SENDCONTENT_P_BUFFER_SZ];
WiFiWebServer-impl.h:  _currentHeaders = new RequestArgument[_headerKeysCount];

Regards,