Some improvements to multipart parser

[aemseemann]

This PR addresses some issues with the current multipart parser implementation.

• Allow part headers to span multiple TCP packets (similar to how HTTP headers are handled). It is very unlikely for this issue to occur for requests with only one part, since all the headers are probably part of the first TCP packet. However, requests with multiple parts may fail randomly if a part header just happens to span across a TCP packet boundary.
• Allow construction of the MultipartParser instance to fail, e.g. when the 'boundary' part of the Content-Type header is missing.
• Make parser state (multipart_parser_t) a member of MultipartParser to avoid additional dynamic memory allocation.
• Allow (and ignore) preamble data. Although preamble data is rarely used in practice, it has been observed that some clients add an additional newline before the message body, which, technically, qualifies as preamble data.

[mikee47]

Optimisation: indexOf(_F("boundary="), 9)

[aemseemann]

Is there any downside in using indexOf(FS("boundary=")) instead?
At least for small strings and thanks to your SSO, WString should not do a heap allocation, so both _F and FS with its implicit operator WString will essentially do a memcpy_aligned.

[mikee47]

Good point. With SSO indexOf(F("boundary=")) is also an option.

[mikee47]

Looks good! just a couple of suggestions.

One thing that's been bugging me about the MultipartParser is that the entire framework needs rebuilding when ENABLE_HTTP_SERVER_MULTIPART changes. I've got an idea about that, will push something to your repo for consideration.

[mikee47]

...the entire framework needs rebuilding when ENABLE_HTTP_SERVER_MULTIPART changes.

I've pushed a commit to mikee47@ca58413.

[slaff]

That code should be tested. My very quick test resulted in getting stuck here.

[aemseemann]

Strange, it works for me (tested with the otatool.py upload from PR#2006, modified to include some preamble data).
Can you describe your test setup?
(The preamble data must end with '\r\n' before the first boundary)

[slaff]

@aemseemann This is C code. And in C you should use strcpy to copy strings. Here you are making a copy of a pointer that is invalid when you actually have to access multipart_boundary.

[aemseemann]

The intention is to copy the pointer and not the string content (to avoid an additional allocation).

It is the caller's responsibility to ensure that the content stays stays valid (and unchanged) throughout the lifetime of the multipart_parser. This is ensured, because the caller is the MultipartParser constructor and both multipart_parser and the boundary string are both members of MultipartParser.

[slaff]

@aemseemann can you try the following: Compile HttpServer_FirmwareUpload in debug mode for the Host

cd $SMING_HOME/../sample/HttpServer_FirmwareUpload
make dist-clean
make clean
make SMING_ARCH=Host ENABLE_GDB=1 ENABLE_LWIPDEBUG=1 ENABLE_MALLOC_COUNT=0
make flash

Once that finishes if you have valgrind run the application by calling

# setup your virtual network interface. see: https://sming.readthedocs.io/en/latest/_inc/Sming/Arch/Host/Components/lwip/index.html
make valgrind

At least on my system valgrind starts to report issues with the multipart_boundary. Example:

==23134== Invalid read of size 1
==23134==    at 0x809CC14: multipart_parser_execute (multipart_parser.c:112)

You can also run the application in a visual debugger and start monitoring the bytes starting at the memory address that multipart_boundary points to. As far as I can see the first bytes are overwritten.

[aemseemann]

Can confirm the bug, thanks for the explanation!
I accidentally took the address from the MultipartParser constructor argument, which was shadowing the boundary member. This is fixed now.

However, valgrind discovered an issue with TcpConnection::checkSelfFree:

==8800== Invalid read of size 1
==8800==    at 0x12C374: checkSelfFree (TcpConnection.h:191)
==8800==    by 0x12C374: TcpConnection::checkSelfFree() (TcpConnection.h:189)
==8800==    by 0x12CD26: TcpConnection::internalOnPoll() (TcpConnection.cpp:507)

The reason is in TcpConnection.cpp:500pp

err_t TcpConnection::internalOnPoll()
{
	//if (tcp->state != ESTABLISHED)
	//	return ERR_OK;

	sleep++;
	err_t res = onPoll();
	checkSelfFree(); /// <<< instance was deleted inside `onPoll`
	debug_tcp_ext("<poll");
	return res;
}

Could be fixed with:

	err_t res = onPoll(); 
	if (res != ERR_TIMEOUT) {
		// on ERR_TIMEOUT, checkSelfFree is already called inside checkSelfFree
		checkSelfFree();
	}

However, I lack the in-depth knowledge of the low level classes and relying on the error code seems quite fragile.

[aemseemann]

Moving sources to 'src' and code formatting is done.

I am still thinking about @mikee47's proposal. Maybe we can go a step further: Instead of just avoiding the SmingCore recompilation, MultipartParser could be converted into a Library. Then remove ENABLE_HTTP_SERVER_MULTIPART and make it the user's responsibility to do the following call during web server configuration:

server.setBodyParser(MIME_FORM_MULTIPART, formMultipartParser);

[slaff]

Can confirm the bug, thanks for the explanation!

@aemseemann Awesome, thanks a lot for fixing it :)

and make it the user's responsibility to do the following call during web server configuration:

Yes, let's do it. Add the related changes as part of this PR.

The reason is in TcpConnection.cpp:500pp

Create a separate PR for this issue.

[aemseemann]

MultipartParser conversion into a Library is done.

The TcpConnection fix will have to wait untill later this week.

[slaff]

@aemseemann This PR is scheduled for merging. Once the small changes are committed I will take care to merge it in develop.