Skip to content

Reduce code duplications in _parseRequest #6951

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+155 −182
Open

Reduce code duplications in _parseRequest #6951

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
233dad0
Reduce code duplications in _parseRequest
dirkmueller Dec 26, 2019
7a3426f
Add deprecated wrapper/forwarder for _collectHeader
dirkmueller Jan 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion libraries/ESP8266WebServer/src/ESP8266WebServer.h
View file
Original file line number Diff line number Diff line change
@@ -204,7 +204,8 @@ class ESP8266WebServerTemplate
void _uploadWriteByte(uint8_t b);
uint8_t _uploadReadByte(ClientType& client);
void _prepareHeader(String& response, int code, const char* content_type, size_t contentLength);
bool _collectHeader(const char* headerName, const char* headerValue);
bool _collectHeader(const char* headerName, const char* headerValue) __attribute__((deprecated));
bool _collectHeader(const String& headerName, const String& headerValue);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a signature change for a non-private method, and therefore breaking. We can wait to merge for v3, or you could add a compatibility method that forwards from one to the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is source compatible (String(const char*) constructor is implicit). I don't see the need to add a forwarder method - it will just work in any case as far as I can see.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're thinking of method call, I'm referring to inheritance. This change can cause a derived class to no longer compile. Trivial example with std::string instead of String, but the principle is the same:

struct Base
{
    void method(const char * arg) {std::cout << "Base arg=" << arg << std::endl;}
};

struct Derived : Base
{
//    void method(const char *arg) 
    void method(const std::string &arg) 
    {
        std::cout << "strlen=" << strlen(arg) << std::endl;
        Base::method(arg);
    }
};


void _streamFileCore(const size_t fileSize, const String & fileName, const String & contentType);

334 changes: 153 additions & 181 deletions libraries/ESP8266WebServer/src/Parsing-impl.h
View file
Original file line number Diff line number Diff line change
@@ -37,7 +37,6 @@
#endif

static const char Content_Type[] PROGMEM = "Content-Type";
static const char filename[] PROGMEM = "filename";

template <typename ServerType>
static bool readBytesWithTimeout(typename ServerType::ClientType& client, size_t maxLength, String& data, int timeout_ms)
@@ -62,216 +61,188 @@ static bool readBytesWithTimeout(typename ServerType::ClientType& client, size_t

template <typename ServerType>
bool ESP8266WebServerTemplate<ServerType>::_parseRequest(ClientType& client) {
// Read the first line of HTTP request
String req = client.readStringUntil('\r');
// Read the first line of HTTP request
String req = client.readStringUntil('\r');
#ifdef DEBUG_ESP_HTTP_SERVER
DEBUG_OUTPUT.print("request: ");
DEBUG_OUTPUT.println(req);
#endif
client.readStringUntil('\n');
//reset header value
for (int i = 0; i < _headerKeysCount; ++i) {
_currentHeaders[i].value =String();
}

// First line of HTTP request looks like "GET /path HTTP/1.1"
// Retrieve the "/path" part by finding the spaces
int addr_start = req.indexOf(' ');
int addr_end = req.indexOf(' ', addr_start + 1);
if (addr_start == -1 || addr_end == -1) {
client.readStringUntil('\n');
//reset header value
for (size_t i = 0; i < _headerKeysCount; ++i) {
_currentHeaders[i].value.clear();
}

// First line of HTTP request looks like "GET /path HTTP/1.1"
// Retrieve the "/path" part by finding the spaces
int addr_start = req.indexOf(' ');
int addr_end = req.indexOf(' ', addr_start + 1);
if (addr_start == -1 || addr_end == -1) {
#ifdef DEBUG_ESP_HTTP_SERVER
DEBUG_OUTPUT.println("Invalid request");
DEBUG_OUTPUT.println("Invalid request");
#endif
return false;
}
return false;
}

String methodStr = req.substring(0, addr_start);
String url = req.substring(addr_start + 1, addr_end);
String versionEnd = req.substring(addr_end + 8);
_currentVersion = atoi(versionEnd.c_str());
String searchStr;
int hasSearch = url.indexOf('?');
if (hasSearch != -1){
searchStr = url.substring(hasSearch + 1);
url = url.substring(0, hasSearch);
}
_currentUri = url;
_chunked = false;

HTTPMethod method = HTTP_GET;
if (methodStr == F("HEAD")) {
method = HTTP_HEAD;
} else if (methodStr == F("POST")) {
method = HTTP_POST;
} else if (methodStr == F("DELETE")) {
method = HTTP_DELETE;
} else if (methodStr == F("OPTIONS")) {
method = HTTP_OPTIONS;
} else if (methodStr == F("PUT")) {
method = HTTP_PUT;
} else if (methodStr == F("PATCH")) {
method = HTTP_PATCH;
}
_currentMethod = method;
String methodStr = req.substring(0, addr_start);
String url = req.substring(addr_start + 1, addr_end);
_currentVersion = req.substring(addr_end + 8).toInt();
String searchStr;
int hasSearch = url.indexOf('?');
if (hasSearch != -1) {
searchStr = url.substring(hasSearch + 1);
url = url.substring(0, hasSearch);
}
_currentUri = url;
_chunked = false;

HTTPMethod method = HTTP_GET;
if (methodStr == F("HEAD")) {
method = HTTP_HEAD;
} else if (methodStr == F("POST")) {
method = HTTP_POST;
} else if (methodStr == F("DELETE")) {
method = HTTP_DELETE;
} else if (methodStr == F("OPTIONS")) {
method = HTTP_OPTIONS;
} else if (methodStr == F("PUT")) {
method = HTTP_PUT;
} else if (methodStr == F("PATCH")) {
method = HTTP_PATCH;
}
_currentMethod = method;

#ifdef DEBUG_ESP_HTTP_SERVER
DEBUG_OUTPUT.print("method: ");
DEBUG_OUTPUT.print(methodStr);
DEBUG_OUTPUT.print(" url: ");
DEBUG_OUTPUT.print(url);
DEBUG_OUTPUT.print(" search: ");
DEBUG_OUTPUT.println(searchStr);
DEBUG_OUTPUT.print("method: ");
DEBUG_OUTPUT.print(methodStr);
DEBUG_OUTPUT.print(" url: ");
DEBUG_OUTPUT.print(url);
DEBUG_OUTPUT.print(" search: ");
DEBUG_OUTPUT.println(searchStr);
#endif

//attach handler
RequestHandlerType* handler;
for (handler = _firstHandler; handler; handler = handler->next()) {
if (handler->canHandle(_currentMethod, _currentUri))
break;
}
_currentHandler = handler;
//attach handler
RequestHandlerType* handler;
for (handler = _firstHandler; handler; handler = handler->next()) {
if (handler->canHandle(_currentMethod, _currentUri))
break;
}
_currentHandler = handler;

String formData;
// below is needed only when POST type request
if (method == HTTP_POST || method == HTTP_PUT || method == HTTP_PATCH || method == HTTP_DELETE){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test that moving this condition still results in correct function as before for all methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not test all methods. I did test GET and POST though in the scope of my application and it worked fine.

Another way to put it is however that this "if condition" didn't actually move. what happened is that the header parsing part that was used in the POST/PUT/PATCH/DELETE code was moved up front, and then the rest of the code remained the same. this allowed to remove the header parsing that was done for GET/HEAD case as it was duplicate now.

Personally I think thats a relatively low risk change because the two header parsing loops were doing mostly the same, except that POST/PUT/PATCH/DELETE was checking for additional headers and parsed them. It now does so in all cases, but since it is extremely unlikely to get the additional header fields send by GET/HEAD requests, it will not matter in practice. and even if we get them, the parsed result is ignored as the if() condition remained the same and the parsed resulting values are not further evaluated.

String boundaryStr;
String headerName;
String headerValue;
String boundaryStr;
bool isForm = false;
bool isEncoded = false;
uint32_t contentLength = 0;
//parse headers
while(1){
req = client.readStringUntil('\r');
client.readStringUntil('\n');
if (req.isEmpty()) break;//no moar headers
int headerDiv = req.indexOf(':');
if (headerDiv == -1){
break;
}
headerName = req.substring(0, headerDiv);
headerValue = req.substring(headerDiv + 1);
headerValue.trim();
_collectHeader(headerName.c_str(),headerValue.c_str());

#ifdef DEBUG_ESP_HTTP_SERVER
DEBUG_OUTPUT.print("headerName: ");
DEBUG_OUTPUT.println(headerName);
DEBUG_OUTPUT.print("headerValue: ");
DEBUG_OUTPUT.println(headerValue);
#endif

if (headerName.equalsIgnoreCase(FPSTR(Content_Type))){
using namespace mime;
if (headerValue.startsWith(FPSTR(mimeTable[txt].mimeType))){
isForm = false;
} else if (headerValue.startsWith(F("application/x-www-form-urlencoded"))){
isForm = false;
isEncoded = true;
} else if (headerValue.startsWith(F("multipart/"))){
boundaryStr = headerValue.substring(headerValue.indexOf('=') + 1);
boundaryStr.replace("\"","");
isForm = true;
}
} else if (headerName.equalsIgnoreCase(F("Content-Length"))){
contentLength = headerValue.toInt();
} else if (headerName.equalsIgnoreCase(F("Host"))){
_hostHeader = headerValue;
}
}
// parse headers
while (true) {
req = client.readStringUntil('\r');
client.readStringUntil('\n');

String plainBuf;
if ( !isForm
&& // read content into plainBuf
( !readBytesWithTimeout<ServerType>(client, contentLength, plainBuf, HTTP_MAX_POST_WAIT)
|| (plainBuf.length() < contentLength)
)
)
{
return false;
}
int headerDiv = req.indexOf(':');
if (headerDiv < 0) {
break;
}
headerName = req.substring(0, headerDiv);
headerValue = req.substring(headerDiv + 1);
headerValue.trim();
_collectHeader(headerName, headerValue);

if (isEncoded) {
// isEncoded => !isForm => plainBuf is not empty
// add plainBuf in search str
if (searchStr.length())
searchStr += '&';
searchStr += plainBuf;
#ifdef DEBUG_ESP_HTTP_SERVER
DEBUG_OUTPUT.print(F("headerName: "));
DEBUG_OUTPUT.println(headerName);
DEBUG_OUTPUT.print(F("headerValue: "));
DEBUG_OUTPUT.println(headerValue);
#endif
if (headerName.equalsIgnoreCase(FPSTR(Content_Type))) {
if (headerValue.startsWith(FPSTR(::mime::mimeTable[::mime::txt].mimeType))) {
isForm = false;
} else if (headerValue.startsWith(F("application/x-www-form-urlencoded"))) {
isForm = false;
isEncoded = true;
} else if (headerValue.startsWith(F("multipart/"))) {
boundaryStr = headerValue.substring(headerValue.indexOf('=') + 1);
boundaryStr.replace("\"", "");
isForm = true;
}
} else if (headerName.equalsIgnoreCase(F("Content-Length"))) {
contentLength = headerValue.toInt();
} else if (headerName.equalsIgnoreCase(F("Host"))) {
_hostHeader = headerValue;
}
}

// parse searchStr for key/value pairs
_parseArguments(searchStr);

if (!isForm) {
if (contentLength) {
// add key=value: plain={body} (post json or other data)
RequestArgument& arg = _currentArgs[_currentArgCount++];
arg.key = F("plain");
arg.value = plainBuf;
_currentArgsHavePlain = 1;
}
} else { // isForm is true
// here: content is not yet read (plainBuf is still empty)
if (!_parseForm(client, boundaryStr, contentLength)) {
return false;
}
}
} else {
String headerName;
String headerValue;
//parse headers
while(1){
req = client.readStringUntil('\r');
client.readStringUntil('\n');
if (req.isEmpty()) break;//no moar headers
int headerDiv = req.indexOf(':');
if (headerDiv == -1){
break;
}
headerName = req.substring(0, headerDiv);
headerValue = req.substring(headerDiv + 2);
_collectHeader(headerName.c_str(),headerValue.c_str());
// below is needed only when POST type request
if (method == HTTP_POST || method == HTTP_PUT || method == HTTP_PATCH || method == HTTP_DELETE) {
String plainBuf;
if (!isForm &&
// read content into plainBuf
(!readBytesWithTimeout<ServerType>(client, contentLength, plainBuf, HTTP_MAX_POST_WAIT)
|| (plainBuf.length() < contentLength)))
{
return false;
}

#ifdef DEBUG_ESP_HTTP_SERVER
DEBUG_OUTPUT.print(F("headerName: "));
DEBUG_OUTPUT.println(headerName);
DEBUG_OUTPUT.print(F("headerValue: "));
DEBUG_OUTPUT.println(headerValue);
#endif
if (isEncoded) {
// isEncoded => !isForm => plainBuf is not empty
// add plainBuf in search str
if (searchStr.length())
searchStr += '&';
searchStr += plainBuf;
}

if (headerName.equalsIgnoreCase(F("Host"))){
_hostHeader = headerValue;
}
// parse searchStr for key/value pairs
_parseArguments(searchStr);

if (!isForm) {
if (contentLength) {
// add key=value: plain={body} (post json or other data)
RequestArgument& arg = _currentArgs[_currentArgCount++];
arg.key = F("plain");
arg.value = plainBuf;
_currentArgsHavePlain = 1;
}
} else { // isForm is true
// here: content is not yet read (plainBuf is still empty)
if (!_parseForm(client, boundaryStr, contentLength)) {
return false;
}
}
}
_parseArguments(searchStr);
}
client.flush();
client.flush();

#ifdef DEBUG_ESP_HTTP_SERVER
DEBUG_OUTPUT.print(F("Request: "));
DEBUG_OUTPUT.println(url);
DEBUG_OUTPUT.print(F("Arguments: "));
DEBUG_OUTPUT.println(searchStr);

DEBUG_OUTPUT.println(F("final list of key/value pairs:"));
for (int i = 0; i < _currentArgCount; i++)
DEBUG_OUTPUT.printf(" key:'%s' value:'%s'\r\n",
_currentArgs[i].key.c_str(),
_currentArgs[i].value.c_str());
DEBUG_OUTPUT.print(F("Request: "));
DEBUG_OUTPUT.println(url);
DEBUG_OUTPUT.print(F("Arguments: "));
DEBUG_OUTPUT.println(searchStr);

DEBUG_OUTPUT.println(F("final list of key/value pairs:"));
for (int i = 0; i < _currentArgCount; i++)
DEBUG_OUTPUT.printf(" key:'%s' value:'%s'\r\n",
_currentArgs[i].key.c_str(),
_currentArgs[i].value.c_str());
#endif

return true;
return true;
}

template <typename ServerType>
bool ESP8266WebServerTemplate<ServerType>::_collectHeader(const char* headerName, const char* headerValue) {
for (int i = 0; i < _headerKeysCount; i++) {
if (_currentHeaders[i].key.equalsIgnoreCase(headerName)) {
_currentHeaders[i].value=headerValue;
return _collectHeader(String(headerName), String(headerValue));
}

template <typename ServerType>
bool ESP8266WebServerTemplate<ServerType>::_collectHeader(const String& headerName, const String& headerValue) {
for (int i = 0; i < _headerKeysCount; i++) {
if (_currentHeaders[i].key.equalsIgnoreCase(headerName)) {
_currentHeaders[i].value = headerValue;
return true;
}
}
return false;
}
return false;
}

template <typename ServerType>
@@ -359,9 +330,9 @@ int ESP8266WebServerTemplate<ServerType>::_parseArgumentsPrivate(const String& d
}

template <typename ServerType>
void ESP8266WebServerTemplate<ServerType>::_uploadWriteByte(uint8_t b){
if (_currentUpload->currentSize == HTTP_UPLOAD_BUFLEN){
if(_currentHandler && _currentHandler->canUpload(_currentUri))
void ESP8266WebServerTemplate<ServerType>::_uploadWriteByte(uint8_t b) {
if (_currentUpload->currentSize == HTTP_UPLOAD_BUFLEN) {
if (_currentHandler && _currentHandler->canUpload(_currentUri))
_currentHandler->upload(*this, _currentUri, *_currentUpload);
_currentUpload->totalSize += _currentUpload->currentSize;
_currentUpload->currentSize = 0;
@@ -427,8 +398,9 @@ bool ESP8266WebServerTemplate<ServerType>::_parseForm(ClientType& client, const
DEBUG_OUTPUT.println(argFilename);
#endif
//use GET to set the filename if uploading using blob
if (argFilename == F("blob") && hasArg(FPSTR(filename)))
argFilename = arg(FPSTR(filename));
String filenameArg = arg(F("filename"));
if (argFilename == F("blob") && !filenameArg.isEmpty())
argFilename = filenameArg;
}
#ifdef DEBUG_ESP_HTTP_SERVER
DEBUG_OUTPUT.print("PostArg Name: ");