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 #4936

Closed
nitcord opened this issue Jun 21, 2022 · 10 comments
Closed

Flooding requests results in memory leak #4936

nitcord opened this issue Jun 21, 2022 · 10 comments

Comments

@nitcord
Copy link

nitcord commented Jun 21, 2022

Hello,

Using the built-in http and https packages causes a memory leak when the server gets flooded with requests from different IPs which eventually crashes the server.

Code

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

const app = express();

app.enable('trust proxy');

app.all('*', (req, res) => {
  return res.send({
    message: 'Hello World!'
  });
});

const privateKey = fs.readFileSync('/etc/letsencrypt/live/.../privkey.pem', 'utf8');
const certificate = fs.readFileSync('/etc/letsencrypt/live/.../cert.pem', 'utf8');
const ca = fs.readFileSync('/etc/letsencrypt/live/.../chain.pem', 'utf8');
const credentials = {
    key: privateKey,
    cert: certificate,
    ca: ca
};

const httpServer = http.createServer(app);
const httpsServer = https.createServer(credentials, app);

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

httpsServer.listen(443, () => {
    console.log('HTTPS Server running on port 443');
});

Error

(node:14043) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added to [TLSSocket]. Use emitter.setMaxListeners() to increase limit
@nitcord nitcord changed the title Memory Leaks Flooding Requests Memory Leak Jun 21, 2022
@nitcord nitcord changed the title Flooding Requests Memory Leak Flooding requests results in memory leak Jun 21, 2022
@dougwilson
Copy link
Contributor

Hi @VeryHamburger very sorry you are encountering this. We would love to help investigate. Is it possible to provide steps to reproduce that error message as well as the exact version of Node.js and Express.js you are using when it happens? Also, would it be possible to see if the following code still has the issue or not?

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

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

const privateKey = fs.readFileSync('/etc/letsencrypt/live/.../privkey.pem', 'utf8');
const certificate = fs.readFileSync('/etc/letsencrypt/live/.../cert.pem', 'utf8');
const ca = fs.readFileSync('/etc/letsencrypt/live/.../chain.pem', 'utf8');
const credentials = {
    key: privateKey,
    cert: certificate,
    ca: ca
};

const httpServer = http.createServer(app);
const httpsServer = https.createServer(credentials, app);

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

httpsServer.listen(443, () => {
    console.log('HTTPS Server running on port 443');
});

@nitcord
Copy link
Author

nitcord commented Jun 22, 2022

My Node.js version is v18.4.0 and my Express.js version is 4.18.1.

To reproduce the error message on the code I was using, you would have to use a stress tester and run it for around 300 seconds and it will show that error.

@dougwilson
Copy link
Contributor

Thank you. I tried a stress tester, wrk without success. What one are you using and can you state all the options you are specifying?

@nitcord
Copy link
Author

nitcord commented Jun 22, 2022

I'm using this site which only requires an account to use and is free to use.

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

If you don't get an error, you might have to run it again.

@dougwilson
Copy link
Contributor

dougwilson commented Jun 22, 2022

Thank you, I will try that tomorrow (it is past 1am my time). In the meanwhile, can you start your server with https://nodejs.org/dist/latest-v18.x/docs/api/cli.html#--trace-warnings and paste the full stack trace for the warning you are getting? That will help narrow down what is adding that event listener (at least, the 11th one). I am suspecting this is ultimately a Node.js issue, as there is nothing in your example script in Express.js that adds error event listeners.

@nitcord
Copy link
Author

nitcord commented Jun 22, 2022

You can the error output below and I started my server using the --trace-warnings option.

(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)

I have also tried the LTS version of Node.js which didn't really make a difference. This error only happens when using Express and not when using the built-in modules on their own.

@dougwilson
Copy link
Contributor

Hi @VeryHamburger so I was able to set up a server and use the site you gave and reproduce the warning you got, both with and without Express.js. It seems you were also able to reproduce the warning without Express.js in use as well, though. I looked into the warning, and it is all happening within Node.js itself, before Express.js can do anything. Unfortunately you likely will need to report this issue to Node.js. You're welcome to use the code I posted above for the report, which doesn't use Express.js at all, to help the Node.js project reproduce the issue 👍

@nitcord
Copy link
Author

nitcord commented Jun 23, 2022

I only get the warning with Express and not in Node.js itself. I thought it happened on both but my index.js file didn't update.

@dougwilson
Copy link
Contributor

dougwilson commented Jun 23, 2022

Hm, very strange.I was able to reproduce on both. But in Express.js code, nothinf is adding the error listener that is leaking with the warning. That event listener is being added by Node.js itself, before the request is ever even handed to Express.js. It is unclear how Express.js can fix the issue you have provided, as the code it all within the Node.js code base and not Express.js...

https://github.com/nodejs/node/blob/e339e9c5d71b72fd09e6abd38b10678e0c592ae7/lib/_http_server.js#L749

@dougwilson
Copy link
Contributor

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 thw 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.

@dougwilson dougwilson closed this as not planned Won't fix, can't repro, duplicate, stale Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants