[search] Wait for replies from client before sending more data #6640
Conversation
Apparently we are supposed to expect a reply and to wait for it. Otherwise bad things happen.
|
|
||
| searchPackets.pop_front(); | ||
|
|
||
| encrypt(length); |
There was a problem hiding this comment.
Not relevant for this PR, but as future points: Should we:
- make
searchPacketmore abstract asOwnedSpanor similar - make
searchPacketmore of it's own thing, where it can encrypt itself and we then just stuff it into the buffer, etc. - Rename it to
SearchPacket - We now have
data_andsearchPackets- is this necessary?
There was a problem hiding this comment.
We now have data_ and searchPackets - is this necessary?
I suppose no, if we boost the internal buffer size of searchPackets to 4096 we could encrypt them ahead of time.
| do_read(); | ||
|
|
||
| deadline_.expires_after(std::chrono::milliseconds(2000)); | ||
| deadline_.expires_after(std::chrono::milliseconds(10000)); // AH searches can take quite a while |
There was a problem hiding this comment.
While I'm looking at this - what is this actually the deadline for? C2S packet, db access, S2C packet?
There was a problem hiding this comment.
It was originally intended as a way to close a hung socket, but now it serves a few more purposes
If the query simply takes too long in general it will die, though that was not intended. There are several points where we could be waiting forever on a socket and we don't want to due to malicious actors.
I suppose we may want to repurpose it into the total lifespan of a single request, with a shorter timeout for the original open.
Open -> few hundred MS waiting on arrival of first data packet -> close if it never comes
Open -> few hundred MS waiting on arrival of first data packet -> get request-> send reply -> we are now waiting again for a reply that could never come. Repeat send reply -> get request a few times and this could be waiting a long time with someone with a malicious attack.
| @@ -40,4 +40,38 @@ struct search_req | |||
| uint8 commentType; | |||
| }; | |||
|
|
|||
| class searchPacket | |||
There was a problem hiding this comment.
In the future, we might want to look at making this more abstract, or more specialised.
There was a problem hiding this comment.
agreed, it's probably useful elsewhere
| @@ -110,4 +110,6 @@ class search_handler | |||
| auto _HandleSearchRequest() -> search_req; | |||
|
|
|||
| blowfish_t blowfish; | |||
|
|
|||
| std::deque<searchPacket> searchPackets; | |||
There was a problem hiding this comment.
Under what circumstances will we end up with more than a couple of packets in here?
There was a problem hiding this comment.
AH list item packets are 20~ bytes and there are several hundred items in most equipment categories. So probably a good chunk of the time.
There was a problem hiding this comment.
uint8 PacketsCount = (uint8)((ItemList.size() / 20) + (ItemList.size() % 20 != 0) + (ItemList.empty()));
There was a problem hiding this comment.
Ah, maybe it is 20 items per chunk. Still a good amount.
I affirm:
What does this pull request do?
Apparently we are supposed to expect a reply and to wait for it. Otherwise bad things happen. (see #6633)
Steps to test these changes
search linkshell, party, /sea all, search comment, AH item list, AH sale list, AH detailed sale list on 2+ clients at a time with no errors on windows and linux