fix client buffer not read correctly

[IgorYbema]

Some changes to how the read buffer is read (parsed) using the client buffer. These changes are necessary for some cases (like boundaries split on the end of the client buffer). Also, changing the client buffer from 128 (default) to higher (for example 512) now works with this changes.

[CurlyMoo]

Do you have test cases that trigger the issue? In my main https://github.com/CurlyMoo/webserver repository i use test cases to validate various parts of the webserver. Without it, i can't guarantee that each issue is properly adressed in future code changes.

[IgorYbema]

Yes, see Egyras#481. The concast testrule from that issue cause the post to timeout. The pr/121 code isn't necesarry as the bug is due to the length of the rule. The fact that the rule uses commands from pr/121 isn't important.
I can create a wireshark dump if you want but I am always doing it wrong I believe :)
But it it constant reproducable. Maybe on your computer also. Just copy paste that testrule into heishamon and hit 'save'.

After a few hours of debugging I found that it didn't find the ending boundary with CRLN with '--' in the mulitpart decoding as the client buffer was split exaclty at this point. My temporary fix to check on CRLN with only one dash '-' fixed it also but in this PR there is a better way. It just rolls back the client buf into the read buf if these chars are found on the end of the client buf.

Then I tried enlarging the client buffer from 128 to higher and stumbled on more bugs. I believe these changes are fixing these issue but I am sure it isn't working for all corner situation yet.

[IgorYbema]

Can you explain how I can run this myself?

[CurlyMoo]

My advice would be to fix the webserver code here https://github.com/CurlyMoo/webserver by providing additional unittests. I think the best approach would be not to sent me Wireshark output, but instead create a additional unittest yourself.

You may use the test_receive_binary binary for it. That now tests all kinds of (previously) failing firmware uploads.

[CurlyMoo]

Can you explain how I can run this myself?

https://github.com/CurlyMoo/webserver?tab=readme-ov-file#linux

[IgorYbema]

The code in that github repo isn't up to date on which heishamon has. For example the posEnd part (even without my changes) are missing in the parse_request function substep 4.
If i run that without my changes it fails also.

owh it even fails without any changes to the repo

[CurlyMoo]

Wierd stuff, i though i just checked if it worked before posting.

[CurlyMoo]

I see, the travis connection broke so i didn't got feedback anymore on top of my own checks.

[CurlyMoo]

At least i know what i can do next; fix the current webserver code.

[IgorYbema]

Ok let me know when it works again and then i'll test my changes.

[IgorYbema]

The strnstr function contains a bug also. If the str to find is at end of first string then it gets into overflow.
The inner for loop runs beyond the length of the strings due to check b<=len instead of b<len

This issue can explain some issue's I had seen before with not detecting the end boundary

*** strnstr.cpp 2024-05-07 16:03:55.171353199 +0200
--- strnstr.cpp.1       2024-05-07 16:03:43.794708120 +0200
***************
*** 18,24 ****
      if(ch == str2[0] && a+len <= size) {
        c = a;
        a++;
!       for(b=1;b<len;a++, b++) {
          ch = str1[a];
          if(str2[b] != ch) {
            break;
--- 18,24 ----
      if(ch == str2[0] && a+len <= size) {
        c = a;
        a++;
!       for(b=1;b<=len;a++, b++) {
          ch = str1[a];
          if(str2[b] != ch) {
            break;

[CurlyMoo]

You mean this case?

strnstr((unsigned char *)"foo bar", "bar", strlen("foo bar"));

[IgorYbema]

You mean this case?

strnstr((unsigned char *)"foo bar", "bar", strlen("foo bar"));

Yes.
Len = 3 (length of "bar")
In the for loop, b starts at '1' to check the chars after the first matched char.
b can reach up to 3 (because b<=3). But str2[3] is out of bounds. Also str1[a] (where a will be at 'size') will also be out of bounds.

[CurlyMoo]

I see that the webserver uses it's own strnstr to make it anymore clear 😕

https://github.com/CurlyMoo/webserver/blob/main/webserver.cpp#L745-L761

[IgorYbema]

I see that the webserver uses it's own strnstr to make it anymore clear 😕

https://github.com/CurlyMoo/webserver/blob/main/webserver.cpp#L745-L761

LOL,.. but that is char * result and all calls are unsigned char * so it matches to the one in strnstr.h

[CurlyMoo]

Stupid C++ vs C

[CurlyMoo]

I'm a bit further in testing the unittests and why it suddenly failed after some years.

The tests as defined in this script all work fine:
https://github.com/CurlyMoo/webserver/blob/main/ci/travis-script.sh

However, a test with just the defaults WEBSERVER_MAX_SENDLIST 10 and WEBSERVER_SENDLIST_BUFSIZE 128 doesn't. So, i though i had various test situations covered except just testing the defaults 😄

[CurlyMoo]

This is the commit that started to fail running the defaults:
CurlyMoo/webserver@0907796

[CurlyMoo]

I reverted that specific commit because it was crap. The unittests work again.

[IgorYbema]

Just added a PR to your repo to fix a few issues and add the test 16 which caused it to fail before
CurlyMoo/webserver#1

[CurlyMoo]

If you sync the webserver to here, i will do so in the rules PR after this PR is merged.

[IgorYbema]

Hmm I tried some local syncing and now notice that it seems to use 'root' as name :)
I'll test the code soon and fix the commit issue.

[CurlyMoo]

I saw. That’s why I left out the ‘signed-off’ message.

[IgorYbema]

I think this should be it. I believe the revert of that commit you did should have been merged also but not sure so if you can do a quick visual check of the diff that would be great.

[CurlyMoo]

Why don't you just overwrite webserver.cpp from the one in my repo? webserver.h can also be overwritten. The only thing that should be taken care of are the values of the defines. Those are a bit different.

[IgorYbema]

That was too easy and I thought I wasn't using git properly then :-)
Anyway it also required me to re-add the serial logging lines and I just checked the diff and it looks fine.

For the ESP32 code I need to apply the diff also so I think this is the best way now.

[CurlyMoo]

There are some lines though that I don’t recognize.

[IgorYbema]

Please let me know which ones

[CurlyMoo]

The ones that cannot be found in my own webserver repo.

[IgorYbema]

As I said, those are the necessary changes for heishamon to work with serial logging

[CurlyMoo]

I don't see them anymore, so i think it's fine.