Export to GitHub

mongoose - issue #370

logic error in read_request() which would produce INVALID failure reports when the remote would send enough data to flood the entire buffer in one pull() [fix included]


Posted on Jul 2, 2012 by Swift Ox

What steps will reproduce the problem? 1. Use a client or CGI app which sends enough (all) data all at once. 2. Code review. Reason through read_request() while assuming the pull() will fill the buffer entirely in one go. get_request_len() isn't called then when it should.

What is the expected output? What do you see instead?

mongoose should correctly locate the 2xCRLF (or 2xLF in case of CGI) which terminates the request/response header there, but it doesn't when read_request() only needs one pull() to fill the entire caller's buffer.

Related issues: issue 224.

What version of the product are you using? On what operating system?

mongoose HEAD revision, any platform

Please provide any additional information below.

https://github.com/GerHobbelt/mongoose/commit/9b7f9f9879355c2d72ae6d4094f7248b359a3832 --> fix logic error in read_request() which would produce INVALID failure reports when the remote would send enough data to flood the entire buffer in one pull(): get_request_len() would not be called and hence no HTTP header would be detected, while there /would/ be one in the buffer.

Comment #1

Posted on Jul 2, 2012 by Swift Ox

(No comment was entered for this change.)

Attachments

Comment #2

Posted on Jul 2, 2012 by Swift Ox

And for the next time, also apply this one to get better error log output, covering the eventualities:

Attachments

Comment #4

Posted on Aug 17, 2012 by Massive Horse

I believe the logic error could be fixed by changing *nread < bufsiz to *nread <= bufsiz This was done in https://github.com/valenok/mongoose/commit/6c54370aa13b65a2c79f08d35fa9264e00dd7d4d Can you confirm that that was indeed the fix and current code does not have that logic error please?

Comment #5

Posted on Aug 18, 2012 by Swift Ox

Well, looks like it when considered naively. (flow analysis); only your way of solving this is walking the narrow edge of the abyss as pull() will now get an input buffer length of zero and not everybody out there is going to like that: note that UNIX man pages and OpenSSL don't state what happens exactly when you feed recv() and friends the senseless/illogical buflen==0 value; in fact, recv() man pages specifically state that the recv() RETURN VALUE of ZERO is used to signal connection closed and your feeding the bugger a buflen==0 would give return value zero a whole different meaning.

Hence, expect things to break in very odd ways on various platforms; this issue will only happen in very particular edge cases (read_request() has to hit that buffer edge). It's up to you if you like to debug those probable failures later on; one thing's for sure: they'll be bloody hard to reproduce, once they happen.

(provided patch was done considering such edge scenarios; meanwhile the code has moved on, so it might be handiest if I file a pull request for this one, now that you're on github.)

Comment #6

Posted on Aug 18, 2012 by Swift Ox

pull request filed:

https://github.com/valenok/mongoose/pull/5

Comment #7

Posted on Aug 18, 2012 by Massive Horse

Merged, thank you.

Status: Fixed

Labels:
Type-Defect Priority-Medium