Memory issues about websocket-node HOT 2 CLOSED

Hi @ManishDariyani, I'm pretty confident that the library itself doesn't have any memory leaks. It runs continuously on Worlize.com and has for a couple years, requiring restarts only for code updates. It's also running http://browserquest.mozilla.org/ which has quite a bit of activity. I haven't had any other reports of memory leaks.

I'm always interested if a leak can be discovered, and would appreciate a reproducible use case in such a scenario that can be used as a test. That being said, there are a number of issues in your code that you posted above. Below are my notes. I'll be referencing line numbers throughout.

Line 27

You should process HTML sanitization of received data on the receiving client. This does not need to be done on the server.

Line 41

This one is critical. Even though you are not intending to handle non-websocket HTTP requests, it is still possible for a client, web crawler, etc., to attempt to make a standard HTTP request to the server. You must handle all HTTP requests, even if it's just to return a 404 "Not Found" response. In your implementation, any HTTP requests that come in will just remain open on the server until a timeout occurs (there may not be one), because it is not being handled. These requests will gradually consume more and more resources. This is a leak of memory resources and local operating system network connection limitations.

Lines 66-69

String.prototype.replace is not the best way to transform the origin for sanitization. You should look into doing correct and full URI parsing through Node's URL module.

Line 73

This one is critical. return false; does nothing. Specifically, it does not reject the connection. You need to explicitly call request.reject(). Since you are returning from the incoming request handler function without either accepting or rejecting the connection, the request will sit there waiting for you to do something with it until it times out (possibly never), uselessly consuming server resources. This is a leak of memory and local operating system network connection limitations.

Line 78

The way you are adding/removing objects from the clients array causes a memory leak. The length of the clients array will continually grow without bound as clients connect and disconnect. Setting an item to null will free it up to be garbage collected, but that "slot" in the array will still consume RAM. This is a memory leak. Instead, when a user disconnects, you should use Array.prototype.indexOf() to determine the index of the connection within the clients array, and then use Array.prototype.splice() to remove the connection from the array. In this way, the array is only as long as the number of connected clients.

Line 91

You are calling JSON.parse() without wrapping it in a try/catch block. If a user passes invalid JSON, it will crash your server, since JSON.parse() throws an exception on invalid JSON.

Line 97

You are using eval to process user-provided data. Don't do this. Ever. Period. This is a gaping security hole. eval is deprecated and will be largely removed from ECMAScript 5. It should not ever be used for any reason. A user would only have to pass in some not-very-cleverly adjusted javascript in the action field of your JSON message to execute arbitrary code directly on your server, or crash the server by passing a garbage value. For example, I could log something to your server's console by passing this JSON:

{
  "action": "ping; console.log(\"Hello there, remote code execution vulnerability!\");(function(){})"
}

Identical behavior can be achieved to suit your purpose without using eval and without the possibility of remote code execution by putting your handler functions into an Object, then looking them up from that object based on the action property and calling them:

var actionHandlers = {
  questionAttempted: function() { /* your handler here */ },
  ping: function() { /* your handler here */ },
  init: function() { /* your handler here */ },
  clearMemory: function() { /* your handler here */ }
};

Then replace lines 96-97 with the following:

var handler = actionHandlers[parseMessage['action']];
if (typeof(handler) === 'function') { // sanity check
  handler(index);
}

Lines 120, 133, 142, and 147

You seem to only be broadcasting messages to all connected clients. Ignoring the fact that calling clients[i].sendUTF() will probably throw an error if any of your clients have disconnected (see discussion above about the clients array), you can use the built-in WebSocketServer.prototype.broadcastUTF() function to send a message to all connected clients.

from websocket-node.

Thanks :) I will update the code.

from websocket-node.