WebServer file upload fails when file ends with "--\r\n".

[everslick]

Board

n.a.

Device Description

n.a.

Hardware Configuration

n.a.

Version

latest master (checkout manually)

IDE Name

n.a.

Operating System

n.a.

Flash frequency

n.a.

PSRAM enabled

yes

Upload speed

n.a.

Description

When the file to upload ends with the string "--\r\n" (e.g. SSL cert files with windows line endings) the WebServer resets the connection before the upload is complete.

I guess the problem is somewhere here: https://github.com/espressif/arduino-esp32/blob/master/libraries/WebServer/src/Parsing.cpp#L353-L524

Sketch

n.a.

Debug Message

n.a.

Other Steps to Reproduce

n.a.

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

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

[me-no-dev]

@everslick since you already have a test case, could you please try and figure out where does it exactly fail in that function. In reality it should not break, but instead figure out that this is not the searched for string and give it to the file processor. Maybe some edge case that we have not thought of

[everslick]

I tracked it down as far as I could. The _parseForm() method is aborted in line https://github.com/espressif/arduino-esp32/blob/master/libraries/WebServer/src/Parsing.cpp#L452
while reading the file.

No idea how to fix this. though. :-/

ESP8266 to the rescue! ;-)

[everslick]

I have opened #9167 for a proposed fix. Please review carefully.

[everslick]

The _uploadReadByte() method should also be replaced with this implementation from ESP8266:

int WebServer::_uploadReadByte(WiFiClient& client){
  int res = client.read();

  if (res < 0) {
    while(!client.available() && client.connected())
      delay(2);

    res = client.read();
  }

  return res;
}

[me-no-dev]

did you try with just replacing that?

[everslick]

did you try with just replacing that?

No, I did not. But, let me explain...

I maintain my own fork of the WebServer library since approx. 4 years, because I need some changes that are not relevant for upstreaming. That fork is compatible with ESP32 and ESP8266 and also had already a different _uploadReadByte() method, which was similar to the one from ESP8266. When I discovered the bug we are discussing here, I immediately replaced my own _uploadReadByte() by the one in ESP32 WebServer, which did not fix anything. Then I filed the issue report. Later I tried replacing _parseForm() with the one from ESP8266 which made the original bug go away and I prepared the PR #9167. After that I made some more tests and found that when uploading big files the connection would be sometimes reset by the server. I then replaced uploadReadByte() again with the one from ESP8266 (see above) and that finally fixed all my issues. It works flawlessly here. That's why I think the actual bug is in _parseForm(), but it needs both functions fixed.

Still, I did not try to understand the bug, nor do I know, if this is a complete solution. Nevertheless the code from ESP8266 looks much saner and is much shorter anyway, so I think it's worth to give it a shot.

Having said that, we can as well wait some days and see if anything comes up here on my side with more testing. Who knows whats still lurking in there. ;-)

[me-no-dev]

the code from ESP8266 looks much saner and is much shorter anyway

regardless, your issue you pointed at _uploadReadByte, so it made sense to ask to change just that. Now we could not know what got it fixed or if we introduced something else. I do not mind porting over the 8266 code, but this is no way to solve issues ;)

[lucasssvaz]

@everslick Could you provide the sketch or example that you used to test this issue ?

[everslick]

I don't have one. And I don't even have Arduino installed. But I believe that the https://github.com/espressif/arduino-esp32/blob/master/libraries/WebServer/examples/FSBrowser/FSBrowser.ino example should exhibit the bug.

What I can say is: I have the code from PR #9167 running the last 3 days with zero issues. I uploaded many files (one with 1.6GB), and I tested excessively with Form data with nearly 100 postArgs. Rock solid for all my use cases with CORE 2.0.14 and 3.0.0 so far.

[lucasssvaz]

I tried to reproduce this issue and wasn't able to trigger it. I tried to upload a text file with a SSL key using windows line endings with the FSBrowser example and I got no issues:

[274878][V][WebServer.cpp:381] handleClient(): New client: client.localIP()=0.0.0.0
[274887][V][Parsing.cpp:122] _parseRequest(): method: POST url: /edit search: 
[274894][V][Parsing.cpp:154] _parseRequest(): headerName: Host
[274900][V][Parsing.cpp:155] _parseRequest(): headerValue: esp32fs.local
[274906][V][Parsing.cpp:154] _parseRequest(): headerName: Connection
[274912][V][Parsing.cpp:155] _parseRequest(): headerValue: keep-alive
[274919][V][Parsing.cpp:154] _parseRequest(): headerName: Content-Length
[274925][V][Parsing.cpp:155] _parseRequest(): headerValue: 1589
[274932][V][Parsing.cpp:154] _parseRequest(): headerName: User-Agent
[274938][V][Parsing.cpp:155] _parseRequest(): headerValue: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36
[274952][V][Parsing.cpp:154] _parseRequest(): headerName: Content-Type
[274959][V][Parsing.cpp:155] _parseRequest(): headerValue: multipart/form-data; boundary=----WebKitFormBoundaryVHENevA9Xe6NHmGP
[274970][V][Parsing.cpp:154] _parseRequest(): headerName: Accept
[274976][V][Parsing.cpp:155] _parseRequest(): headerValue: */*
[274982][V][Parsing.cpp:154] _parseRequest(): headerName: Sec-GPC
[274988][V][Parsing.cpp:155] _parseRequest(): headerValue: 1
[274993][V][Parsing.cpp:154] _parseRequest(): headerName: Accept-Language
[275000][V][Parsing.cpp:155] _parseRequest(): headerValue: pt-BR,pt;q=0.9
[275007][V][Parsing.cpp:154] _parseRequest(): headerName: Origin
[275012][V][Parsing.cpp:155] _parseRequest(): headerValue: http://esp32fs.local
[275020][V][Parsing.cpp:154] _parseRequest(): headerName: Referer
[275026][V][Parsing.cpp:155] _parseRequest(): headerValue: http://esp32fs.local/edit
[275034][V][Parsing.cpp:154] _parseRequest(): headerName: Accept-Encoding
[275040][V][Parsing.cpp:155] _parseRequest(): headerValue: gzip, deflate
[275047][V][Parsing.cpp:254] _parseArguments(): args: 
[275052][V][Parsing.cpp:355] _parseForm(): Parse Form: Boundary: ----WebKitFormBoundaryVHENevA9Xe6NHmGP Length: 1589
[275063][V][Parsing.cpp:389] _parseForm(): PostArg FileName: /test.txt
[275069][V][Parsing.cpp:394] _parseForm(): PostArg Name: data
[275075][V][Parsing.cpp:405] _parseForm(): PostArg Type: text/plain
[275081][V][Parsing.cpp:435] _parseForm(): Start File: /test.txt Type: text/plain
handleFileUpload Name: /test.txt
handleFileUpload Size: 1406
[275278][V][Parsing.cpp:496] _parseForm(): End File: /test.txt Type: text/plain Size: 1406
[275289][V][Parsing.cpp:500] _parseForm(): Done Parsing POST

Do you have any idea of something else that might cause the behavior you are encountering ?

[everslick]

Yes, because even though I tracked every upstream change to the WebServer library very closely, at least that's what I thought, I missed that one specific fix 3 and a half years ago: #4217

I'm sorry for that invalid bugreport.

Nevertheless, I think my PR still has merit, because it reduces code size and improves readability, but that's of course subjective. Feel free to close the PR, if you don't feel like merging it on those grounds alone.

BTW, I believe the WebServer lib could be improved further with not much effort. For example I saw that _postArgs and _postArgsLen are members of WebServer, but do not need to be, because they are merged with _currentArgs at the end of _parseForm() anyway. Also we leak the _postArgs array, each time _parseForm() returns early, because of an error. But anyway, those are different issues and thus, I'm closing this one as invalid! Thanks for looking into it, and again sorry, for the confusion.