Skip to content
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

Flooding requests results in memory leak #43548

Closed
nitcord opened this issue Jun 23, 2022 · 8 comments · Fixed by #46523, #46584 or #46769
Closed

Flooding requests results in memory leak #43548

nitcord opened this issue Jun 23, 2022 · 8 comments · Fixed by #46523, #46584 or #46769
Labels
http Issues or PRs related to the http subsystem.

Comments

@nitcord
Copy link

nitcord commented Jun 23, 2022

Version

v16.15.1

Platform

Linux DNSBox 5.15.0-39-generic #42-Ubuntu SMP Thu Jun 9 23:42:32 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

HTTP

What steps will reproduce the bug?

1. Run the code below using node without requiring any additional dependencies

const fs = require('fs');
const http = require('http');

const app = (req, res) => {
  res.end(JSON.stringify({
    message: 'Hello World!'
  }));
};

const httpServer = http.createServer(app);

httpServer.listen(80, () => {
    console.log('HTTP Server running on port 80');
});

2. Use this site to stress-test the server using the following configuration below.

Type: Layer 7
Target URL: ...
Duration (Seconds): 300
Network (Power): Basic (1 Thread)
Request Type: GET
Attack Method: HTTP/s (SPAM)

How often does it reproduce? Is there a required condition?

You might have to run the test twice for it to take effect but you have to wait for the current test to finish before sending another one. After all of the tests are complete, the server will get a lot of MaxListenersExceededWarning errors and will eventually crash because it will reach the memory limit.

What is the expected behavior?

It shouldn't show a memory leak error and should just continue the requests as normal.

What do you see instead?

(node:16418) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added to [TLSSocket]. Use emitter.setMaxListeners() to increase limit
    at _addListener (node:events:601:17)
    at TLSSocket.addListener (node:events:619:10)
    at TLSSocket.Readable.on (node:internal/streams/readable:875:35)
    at TLSSocket.socketListenerWrap [as on] (node:_http_server:1007:54)
    at TLSSocket.socketOnError (node:_http_server:672:8)
    at onParserExecuteCommon (node:_http_server:702:19)
    at onParserExecute (node:_http_server:646:3)

Additional information

Source

What I have debugged so far in Node.js is that the case seems to be a flaw in the logic of that internal socketOnError function. When it is called, it removes itself from the list of error listeners and then adds a new error listener, noop. It seems that at some point, that listener was the only way socketOnError was invoked. But you will find in their code there are now multiple places in which socketOnError is invoked, and removing that error listener doesn't stop them all. Additional invokations for the same socket keep adding that noop listener, and the state at which the warning happens, there are 10 noop listeners on the socket, and the warning happens when the 11th noop listener is added.

@VoltrexKeyva VoltrexKeyva added the http Issues or PRs related to the http subsystem. label Jun 23, 2022
@aduh95
Copy link
Contributor

aduh95 commented Jun 23, 2022

/cc @nodejs/http

@nitcord
Copy link
Author

nitcord commented Jun 28, 2022

@ShogunPanda Thank you!

@nitcord nitcord closed this as completed Jun 28, 2022
@aduh95 aduh95 reopened this Jun 28, 2022
@cesarjorgemartinez
Copy link

Hi, exist fix for node 16.x?

Regards

@ShogunPanda
Copy link
Contributor

@nodejs/tsc I'm still not an expert of how our backporting strategy works, but I assume since 16 LTS it will only receive critical and security updates. Am I right?

If that's the case, then @cesarjorgemartinez there will be no backport of this.

@mcollina
Copy link
Member

mcollina commented Aug 1, 2022

It seems unlikely we'll get that fix in v18.x as it breaks express and Koa.

@BethGriggs
Copy link
Member

@ShogunPanda, https://github.com/nodejs/release#release-phases possibly has information regarding the backporting strategy that you may find useful. (Note that there's a distinction between Active LTS and maintenance, so it is possible to get non-critical updates and features in Active LTS lines.)

@ShogunPanda
Copy link
Contributor

@BethGriggs Thanks, highly appreciated. I'll take a look to it very soon!

@cesarjorgemartinez
Copy link

cesarjorgemartinez commented Aug 2, 2022 via email

danielleadams pushed a commit that referenced this issue Aug 16, 2022

Verified

This commit was signed with the committer’s verified signature.
danielleadams Danielle Adams
PR-URL: #43587
Fixes: #43548
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
ruyadorno pushed a commit that referenced this issue Aug 23, 2022

Verified

This commit was signed with the committer’s verified signature.
ruyadorno Ruy Adorno
PR-URL: #43587
Fixes: #43548
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
targos pushed a commit that referenced this issue Sep 5, 2022

Verified

This commit was signed with the committer’s verified signature.
targos Michaël Zasso
PR-URL: #43587
Fixes: #43548
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
juanarbol pushed a commit that referenced this issue Oct 10, 2022
PR-URL: #43587
Fixes: #43548
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this issue Oct 11, 2022
PR-URL: nodejs#43587
Fixes: nodejs#43548
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment