Skip to content

Fix ssl read#1501

Closed
michaelortmann wants to merge 4 commits intoeggheads:developfrom
michaelortmann:ssl.read
Closed

Fix ssl read#1501
michaelortmann wants to merge 4 commits intoeggheads:developfrom
michaelortmann:ssl.read

Conversation

@michaelortmann
Copy link
Copy Markdown
Member

@michaelortmann michaelortmann commented Nov 21, 2023

Found by: pym67 and PeGaSuS
Patch by: michaelortmann with help from the whole eggheads crew
Fixes: #1496

One-line summary:
When there are more than 511 bytes available to SSL_read() from an ssl sock, eggdrop would not SSL_read() the next bytes, until more data arrives for this sock.

Additional description (if needed):
For ssl connections, eggdrop grabs max 511 bytes via SSL_read(). If there where more bytes, the next call to select() in sockread() does not fire for such sock. After select() there is a call to SSL_pending(), but thats not reached, because select doesnt fire for the sock, but only for the "secondly" select() timeout (heartbeat) und thus returns 0. Only, when new data arrives, like in our example, when an IRC server sends a "Registration Timeout" message (because eggy didnt read and respond to the PING), only then eggy reads again, and again only up to those 511 bytes. Now it will read the PING the server was waiting for us to respond to, but its too late and the server already disconnects us.

In out testcase the server (UnrealIRCd-6.1.1.1) sends more than 511 bytes because of a long "CAP 302 multiline reply".

This patch checks for SSL_pending() before the select() and then treats the sock as if select() would have fired for it.

The bug was probably effecting many ssl conections, but its hard to notice, if the other end of the ssl connection constantly sends traffic triggering new SSL_read()s. In such case i would expect only a delay here and there.

This patch is likely to fix #1496, because the commit f9d8af4 referenced there introduced a change to not handle SSL_pending() for when select() doesnt fire for the sock:
f9d8af4#diff-e83dab64ed96e8a50f94260f6a3dea5712ef4150a7c2aa3c1c282d4227c93b66R923-R924

So, the bug was introduced with eggdrop 1.9.3rc1

Now, that the bug got fixed, if we look at the size of the buffer that we read into in sockread(): Some time ago the buffer size was upped because of tags and is currently above 8k. SSL_read() will return max 16K, see man page. So, this PR now also upps the "grab" value from 511 to RECVLINEMAX-1. This is more efficient for SSL_read(), read() and eggdrops IO buffer handling.

Please, can anyone check, that #1496 is indeed fixed by this PR?

Test cases demonstrating functionality (if applicable):
For the following tests, finegrained timestamps, see #1087 and #1441 and additional debug was added.
Before:
https://pastebin.com/A7HFC9jV
After:
https://pastebin.com/0aCMzgY4

Copy link
Copy Markdown
Member

@skralg skralg left a comment

Choose a reason for hiding this comment

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

I have not tested this, but it look plausible.

…already way above 512 due to tag functionality, so, lets grab as much as fits into that buffer instead of limiting to 511 here
@michaelortmann
Copy link
Copy Markdown
Member Author

this PR is obsolete by #1506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

botnet userfile transfer issue since v1.9.3, due to commit f9d8af4

2 participants