Skip to content

Feature request: limit WiFiServer backlog size #5380

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
douniwan5788 opened this issue Nov 26, 2018 · 5 comments · Fixed by #6887
Closed

Feature request: limit WiFiServer backlog size #5380

douniwan5788 opened this issue Nov 26, 2018 · 5 comments · Fixed by #6887

Comments

@douniwan5788
Copy link

Please limit the _unclaimed chain as listen backlog (http://man7.org/linux/man-pages/man2/listen.2.html)

long WiFiServer::_accept(tcp_pcb* apcb, long err) {
(void) err;
DEBUGV("WS:ac\r\n");
ClientContext* client = new ClientContext(apcb, &WiFiServer::_s_discard, this);
_unclaimed = slist_append_tail(_unclaimed, client);
tcp_accepted(_pcb);
return ERR_OK;
}

@d-a-v
Copy link
Collaborator

d-a-v commented Nov 26, 2018

This is a very fair request, to prevent DoS and memory filling.
We could use lwIP's tcp_listen_with_backlog (instead of tcp_listen), but it does the same job as the _unclaimed list already implemented by brilliant core developpers. In both case we must limit that number.

What are the issues you are facing ?
What would be a fair amount of maximum backlogged connections with your use case ?
I think a maximum number between 4 and 8 should be sufficient for the general case.
If there are more incoming clients, their tcp stack will have to wait for ESP to effectively accept/honor the previous ones or they will timeout without filling our memory.

Would you make a PR for that ?

I don't think this has any relation with #928, why do you think so ?

@douniwan5788
Copy link
Author

This is a very fair request, to prevent DoS and memory filling.
We could use lwIP's tcp_listen_with_backlog (instead of tcp_listen), but it does the same job as the _unclaimed list already implemented by brilliant core developpers. In both case we must limit that number.

What are the issues you are facing ?
What would be a fair amount of maximum backlogged connections with your use case ?
I think a maximum number between 4 and 8 should be sufficient for the general case.
If there are more incoming clients, their tcp stack will have to wait for ESP to effectively accept/honor the previous ones or they will timeout without filling our memory.

Would you make a PR for that ?

I don't think this has any relation with #928, why do you think so ?

Sorry, it has nothing to do with #928, should I delete it? I just encounter with Links2004's post while googling this backlog problem

I am not familiar with Arduino, but if I have free time, I will make a PR.

My issue is arendst/Tasmota#4460

@d-a-v
Copy link
Collaborator

d-a-v commented Nov 26, 2018

Sorry, it has nothing to do with #928, should I delete it?

No worry, I was just asking because you posted at the two places

I am not familiar with Arduino, but if I have free time, I will make a PR.

This will be welcome !

@d-a-v
Copy link
Collaborator

d-a-v commented Nov 26, 2018

Tasmota is currently releasing updates only using arduino core-2.3.0 (AFAIK, until core-2.5.0 is out and has its chances).
#928 issue no longer happens with core-2.4.0 and later when lwIP-v2 is enabled, but the issue you are reporting here is still valid with any version of lwIP.

@d-a-v d-a-v added the waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. label Dec 4, 2018
@d-a-v d-a-v changed the title WiFiServer backlog memory leak Feature request: limit WiFiServer backlog size Dec 4, 2018
@d-a-v d-a-v added this to the 2.6.0 milestone Dec 4, 2018
@devyte
Copy link
Collaborator

devyte commented Oct 29, 2019

Pushing back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants