Closed socket leads to undefined values in socket.remote* properties

[orihjfrog]

Version

v19.5.0

Platform

Microsoft Windows NT 10.0.19045.0 x64

Subsystem

net

What steps will reproduce the bug?

Run the following Node.js code:

import * as http from 'http';

async function a() {
    return true;
}
async function onrequest(req, res) {
    const socket = req.socket;
    await a();
    console.log("Remote port is: " + socket.remotePort)
}
const server = http.createServer()

server.on('request', onrequest)

server.listen(12345)

Then, in a separate shell, run the following command:

curl -X GET "http://127.0.0.1:12345" -H "Content-Length:" --data "1"

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

Always

What is the expected behavior? Why is that the expected behavior?

Expected the output:

Remote port is: <port number>

The output should include the port that the client used to connect to the server.

What do you see instead?

Remote port is: undefined

Additional information

The code that handles the _getpeername attribute of sockets in the file net.js is incharge of retrieving a peername object with data about the socket in use. This object is used for retrieving the attributes remoteAddress, remotePort and remoteFamily. In 2015 the code of the function was updated in a way that will allow these properties to still be accessed even after the socket is closed. The logic of the fix was as follows:

• Check if this._peername has a value
• If this._peername has a value already, return it
• If it doesn’t have a value, check if this._handle has a value
• If this._handle has a value, use this._handle to create this._peername
• If it doesn’t have a value, return an empty object

The code of the function has changed since 2015, but the main idea of this logic remains. The reason that before this fix the above properties weren’t available after the socket closed is that the previous code checked this._handle before checking this._peername. Since after the socket closes this._handle becomes null, the _getpeername function will return an empty object immediately. The fix to the code changed the order, and assuming this._peername exists, it will return it even after the socket closes. The fix assumes that this._peername was created sometime before the socket was closed.

The assumption that this._peername always exists before closing the socket was found to be not necessarily true. In certain cases, the socket may be closed before the first time _getpeername is called. In such cases, both this._peername and this._handle will be null, causing the function to return an empty object.

The code provided above in the reproduction steps is one example for this kind of behavior. As shown above, the remotePort property is undefined. The reason for that is that the underlying HTTP server received a valid HTTP request, followed by the character “1”, which is not a valid request. The server calls our onrequest function with the valid request as an argument. Then, the onrequest code calls an async function. This call allows Node to do some other logic. In the meantime, the server has read the “1” character and decided that it is a bad request. It sends a “400 Bad Request” response to the client and closes the socket. Then, the async function returns, allowing the rest of the onrequest function to run. The code tries to read socket.remotePort, but since this is the first time the code tries to reference this property, this._peername is still null. Since the socket has already been destroyed, also this._handle is null, causing the _getpeername function to return an empty object.

It is important to note that the current documentation of node documents the fact that remoteAddress may be undefined despite the code fixes that were supposed to fix this issue, but this fact is not documented for remotePort or remoteFamily, which may be undefined too.

Developers might assume that the socket.remote* properties will always have valid values. An undefined value in these properties may lead to unexpected application behavior or errors.

Real-World Use Cases
This bug may affect many projects that assume socket.remote* are always available, even after closing the socket. We searched for other projects that may be impacted by the bug. One example we found was in the project hapi. We found an issue and a commit that fixes it. The issue addresses a case where remoteAddress is undefined. Although the flow in their code doesn’t necessarily match the pattern we discussed (await and then reference to remoteAddress), they did suffer from a similar problem without even knowing that it may be a bug in the core of Node.js. We encountered another project that is affected by this bug, causing a denial of service vulnerability, and reported it to them.

[bnoordhuis]

As you say, the entry for socket.remoteAddress mentions that it can be undefined. The entries for the other properties should be updated to say the same thing.

From a correctness perspective, it's working as intended. The properties are intentionally lazy-loaded for performance reasons and I don't expect we'll change that.

[Basa198]

Can I take this?
I will add in the docs that socket.remotePort and socket.remoteFamily may be undefined as well