Skip to content

Commit 2043623

Browse files
earlephilhowerearlephilhower
authored
Fix POST form parser edge cases (#2182)
From espressif/arduino-esp32#9167
1 parent a3dba8b commit 2043623

File tree

1 file changed

+60
-125
lines changed

1 file changed

+60
-125
lines changed

‎libraries/WebServer/src/Parsing.cpp‎

Lines changed: 60 additions & 125 deletions
Original file line numberDiff line numberDiff line change
@@ -358,49 +358,20 @@ void HTTPServer::_uploadWriteByte(uint8_t b) {
358358

359359
int HTTPServer::_uploadReadByte(WiFiClient * client) {
360360
int res = client->read();
361-
if (res < 0) {
362-
// keep trying until you either read a valid byte or timeout
363-
unsigned long startMillis = millis();
364-
unsigned long timeoutIntervalMillis = client->getTimeout();
365-
bool timedOut = false;
366-
for (;;) {
367-
if (!client->connected()) {
368-
return -1;
369-
}
370-
// loosely modeled after blinkWithoutDelay pattern
371-
while (!timedOut && !client->available() && client->connected()) {
372-
delay(2);
373-
timedOut = millis() - startMillis >= timeoutIntervalMillis;
374-
}
375361

376-
res = client->read();
377-
if (res >= 0) {
378-
return res; // exit on a valid read
379-
}
380-
// NOTE: it is possible to get here and have all of the following
381-
// assertions hold true
382-
//
383-
// -- client->available() > 0
384-
// -- client->connected == true
385-
// -- res == -1
386-
//
387-
// a simple retry strategy overcomes this which is to say the
388-
// assertion is not permanent, but the reason that this works
389-
// is elusive, and possibly indicative of a more subtle underlying
390-
// issue
391-
392-
timedOut = millis() - startMillis >= timeoutIntervalMillis;
393-
if (timedOut) {
394-
return res; // exit on a timeout
395-
}
362+
if (res < 0) {
363+
while (!client->available() && client->connected()) {
364+
delay(2);
396365
}
366+
367+
res = client->read();
397368
}
398369

399370
return res;
400371
}
401372

402373
bool HTTPServer::_parseForm(WiFiClient * client, String boundary, uint32_t len) {
403-
(void) len;
374+
(void)len;
404375
log_v("Parse Form: Boundary: %s Length: %d", boundary.c_str(), len);
405376
String line;
406377
int retry = 0;
@@ -448,10 +419,12 @@ bool HTTPServer::_parseForm(WiFiClient * client, String boundary, uint32_t len)
448419
argType = FPSTR(mimeTable[txt].mimeType);
449420
line = client->readStringUntil('\r');
450421
client->readStringUntil('\n');
451-
if (line.length() > 12 && line.substring(0, 12).equalsIgnoreCase(FPSTR(Content_Type))) {
452-
argType = line.substring(line.indexOf(':') + 2);
453-
//skip next line
454-
client->readStringUntil('\r');
422+
while (line.length() > 0) {
423+
if (line.length() > 12 && line.substring(0, 12).equalsIgnoreCase(FPSTR(Content_Type))) {
424+
argType = line.substring(line.indexOf(':') + 2);
425+
}
426+
//skip over any other headers
427+
line = client->readStringUntil('\r');
455428
client->readStringUntil('\n');
456429
}
457430
log_v("PostArg Type: %s", argType.c_str());
@@ -469,7 +442,7 @@ bool HTTPServer::_parseForm(WiFiClient * client, String boundary, uint32_t len)
469442
}
470443
log_v("PostArg Value: %s", argValue.c_str());
471444

472-
RequestArgument& arg = _postArgs[_postArgsLen++];
445+
RequestArgument &arg = _postArgs[_postArgsLen++];
473446
arg.key = argName;
474447
arg.value = argValue;
475448

@@ -493,98 +466,60 @@ bool HTTPServer::_parseForm(WiFiClient * client, String boundary, uint32_t len)
493466
_currentHandler->upload(*this, _currentUri, *_currentUpload);
494467
}
495468
_currentUpload->status = UPLOAD_FILE_WRITE;
496-
int argByte = _uploadReadByte(client);
497-
readfile:
498469

499-
while (argByte != 0x0D) {
500-
if (argByte < 0) {
470+
int fastBoundaryLen = 4 /* \r\n-- */ + boundary.length() + 1 /* \0 */;
471+
char fastBoundary[fastBoundaryLen];
472+
snprintf(fastBoundary, fastBoundaryLen, "\r\n--%s", boundary.c_str());
473+
int boundaryPtr = 0;
474+
while (true) {
475+
int ret = _uploadReadByte(client);
476+
if (ret < 0) {
477+
// Unexpected, we should have had data available per above
501478
return _parseFormUploadAborted();
502479
}
503-
_uploadWriteByte(argByte);
504-
argByte = _uploadReadByte(client);
505-
}
506-
507-
argByte = _uploadReadByte(client);
508-
if (argByte < 0) {
509-
return _parseFormUploadAborted();
510-
}
511-
if (argByte == 0x0A) {
512-
argByte = _uploadReadByte(client);
513-
if (argByte < 0) {
514-
return _parseFormUploadAborted();
515-
}
516-
if ((char)argByte != '-') {
517-
//continue reading the file
518-
_uploadWriteByte(0x0D);
519-
_uploadWriteByte(0x0A);
520-
goto readfile;
521-
} else {
522-
argByte = _uploadReadByte(client);
523-
if (argByte < 0) {
524-
return _parseFormUploadAborted();
525-
}
526-
if ((char)argByte != '-') {
527-
//continue reading the file
528-
_uploadWriteByte(0x0D);
529-
_uploadWriteByte(0x0A);
530-
_uploadWriteByte((uint8_t)('-'));
531-
goto readfile;
532-
}
533-
}
534-
535-
uint8_t endBuf[boundary.length()];
536-
uint32_t i = 0;
537-
while (i < boundary.length()) {
538-
argByte = _uploadReadByte(client);
539-
if (argByte < 0) {
540-
return _parseFormUploadAborted();
541-
}
542-
if ((char)argByte == 0x0D) {
543-
_uploadWriteByte(0x0D);
544-
_uploadWriteByte(0x0A);
545-
_uploadWriteByte((uint8_t)('-'));
546-
_uploadWriteByte((uint8_t)('-'));
547-
for (uint32_t j = 0; j < i; j++) {
548-
_uploadWriteByte(endBuf[j]);
549-
}
550-
goto readfile;
551-
}
552-
endBuf[i++] = (uint8_t)argByte;
553-
}
554-
555-
if (strstr((const char*)endBuf, boundary.c_str()) != nullptr) {
556-
if (_currentHandler && _currentHandler->canUpload(_currentUri)) {
557-
_currentHandler->upload(*this, _currentUri, *_currentUpload);
558-
}
559-
_currentUpload->totalSize += _currentUpload->currentSize;
560-
_currentUpload->status = UPLOAD_FILE_END;
561-
if (_currentHandler && _currentHandler->canUpload(_currentUri)) {
562-
_currentHandler->upload(*this, _currentUri, *_currentUpload);
563-
}
564-
log_v("End File: %s Type: %s Size: %d", _currentUpload->filename.c_str(), _currentUpload->type.c_str(), _currentUpload->totalSize);
565-
line = client->readStringUntil(0x0D);
566-
client->readStringUntil(0x0A);
567-
if (line == "--") {
568-
log_v("Done Parsing POST");
480+
char in = (char)ret;
481+
if (in == fastBoundary[boundaryPtr]) {
482+
// The input matched the current expected character, advance and possibly exit this file
483+
boundaryPtr++;
484+
if (boundaryPtr == fastBoundaryLen - 1) {
485+
// We read the whole boundary line, we're done here!
569486
break;
570487
}
571-
continue;
572488
} else {
573-
_uploadWriteByte(0x0D);
574-
_uploadWriteByte(0x0A);
575-
_uploadWriteByte((uint8_t)('-'));
576-
_uploadWriteByte((uint8_t)('-'));
577-
for (uint32_t j = 0; j < boundary.length(); j++) {
578-
_uploadWriteByte(endBuf[j]);
489+
// The char doesn't match what we want, so dump whatever matches we had, the read in char, and reset ptr to start
490+
for (int i = 0; i < boundaryPtr; i++) {
491+
_uploadWriteByte(fastBoundary[i]);
492+
}
493+
if (in == fastBoundary[0]) {
494+
// This could be the start of the real end, mark it so and don't emit/skip it
495+
boundaryPtr = 1;
496+
} else {
497+
// Not the 1st char of our pattern, so emit and ignore
498+
_uploadWriteByte(in);
499+
boundaryPtr = 0;
579500
}
580-
argByte = _uploadReadByte(client);
581-
goto readfile;
582501
}
583-
} else {
584-
_uploadWriteByte(0x0D);
585-
goto readfile;
586502
}
587-
break;
503+
// Found the boundary string, finish processing this file upload
504+
if (_currentHandler && _currentHandler->canUpload(_currentUri)) {
505+
_currentHandler->upload(*this, _currentUri, *_currentUpload);
506+
}
507+
_currentUpload->totalSize += _currentUpload->currentSize;
508+
_currentUpload->status = UPLOAD_FILE_END;
509+
if (_currentHandler && _currentHandler->canUpload(_currentUri)) {
510+
_currentHandler->upload(*this, _currentUri, *_currentUpload);
511+
}
512+
log_v("End File: %s Type: %s Size: %d", _currentUpload->filename.c_str(), _currentUpload->type.c_str(), (int)_currentUpload->totalSize);
513+
if (!client->connected()) {
514+
return _parseFormUploadAborted();
515+
}
516+
line = client->readStringUntil('\r');
517+
client->readStringUntil('\n');
518+
if (line == "--") { // extra two dashes mean we reached the end of all form fields
519+
log_v("Done Parsing POST");
520+
break;
521+
}
522+
continue;
588523
}
589524
}
590525
}
@@ -593,7 +528,7 @@ bool HTTPServer::_parseForm(WiFiClient * client, String boundary, uint32_t len)
593528
int iarg;
594529
int totalArgs = ((WEBSERVER_MAX_POST_ARGS - _postArgsLen) < _currentArgCount) ? (WEBSERVER_MAX_POST_ARGS - _postArgsLen) : _currentArgCount;
595530
for (iarg = 0; iarg < totalArgs; iarg++) {
596-
RequestArgument& arg = _postArgs[_postArgsLen++];
531+
RequestArgument &arg = _postArgs[_postArgsLen++];
597532
arg.key = _currentArgs[iarg].key;
598533
arg.value = _currentArgs[iarg].value;
599534
}
@@ -602,7 +537,7 @@ bool HTTPServer::_parseForm(WiFiClient * client, String boundary, uint32_t len)
602537
}
603538
_currentArgs = new RequestArgument[_postArgsLen];
604539
for (iarg = 0; iarg < _postArgsLen; iarg++) {
605-
RequestArgument& arg = _currentArgs[iarg];
540+
RequestArgument &arg = _currentArgs[iarg];
606541
arg.key = _postArgs[iarg].key;
607542
arg.value = _postArgs[iarg].value;
608543
}

0 commit comments

Comments
 (0)