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

Make it possible to forcibly RST a net.Socket #27428

Closed
pimterry opened this issue Apr 26, 2019 · 17 comments
Closed

Make it possible to forcibly RST a net.Socket #27428

pimterry opened this issue Apr 26, 2019 · 17 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@pimterry
Copy link
Member

Is your feature request related to a problem? Please describe.

I'd like to forcibly reset a socket. Primarily for testing - I'm seeing unexpected RST packets in production, which is crashing my node HTTP proxy (my bug, that's fine), and I'd like to be able to write automated tests to ensure I've fixed correctly.

As far as I can tell it's only possible to cleanly close sockets with FIN, you can never intentionally RST.

Describe the solution you'd like

Something like socket.reset() would be great for my specific case, or alternatively low-level controls (e.g. the ability to set socket options as in the Python code below) would be equally useful.

Describe alternatives you've considered

For now, I've implemented this in Python instead. This Python 2.7.1 code is working for me:

import socket
import time
import struct

TCP_IP = '127.0.0.1'
TCP_PORT = 8000

# Connect to the server
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM, 0)
s.connect((TCP_IP, TCP_PORT))

# Start an HTTP request
s.send("CONNECT example.com:80 HTTP/1.1\r\n\
Host: example.com\r\n\
\r\n\
GET / HTTP/1.1\r\n\
Host: example.com\r\n\
")
time.sleep(0.1)

# RST the socket without reading the response
s.setsockopt(socket.SOL_SOCKET, socket.SO_LINGER, struct.pack('ii', 1, 0))
s.close()

Obviously I'd much prefer to test my Node server with JavaScript though, instead of having to bring in another language.

@sam-github
Copy link
Contributor

Have you tried doing a socket.destroy()? It won't send a RST, but if more data arrives from the server after the .destroy(), that should trigger a RST response.

@pimterry
Copy link
Member Author

Interesting! You're right, it's possible to make that work, but it ends up a little hacky. You can get close by translating the Python code and removing the delay, but you have a race to make sure you destroy after the request is sent but before the server response. To test I'm doing that with setTimeout(f, 0) straight after write(), which seems to win that race reliably, but it doesn't fill me with confidence.

I'd like to leave this open, as I would much prefer to be able to explicitly send the RST myself on demand rather than depending on this. It's easy to imagine similar situations where you don't know that there's imminent incoming data from the server, so destroy() wouldn't work. It's useful for me & very interesting though, thanks!

The resulting JS code for reference:

const net = require('net');

const socket = new net.Socket();
socket.connect(8000, "127.0.0.1", () => {
    socket.write(
`CONNECT example.com:80 HTTP/1.1
Host: example.com

GET / HTTP/1.1
Host: example.com
`
    );
    // If this is synchronous, it doesn't work
    // If it waits for write(), but no longer, it 50% works
    // Scheduling it immediately seems fairly reliable
    setTimeout(() => socket.destroy(), 0);
});

@sam-github sam-github added the feature request Issues that request new features to be added to Node.js. label Apr 26, 2019
@sam-github
Copy link
Contributor

Get rid of the setTimeout, its racy because setTimeout(..., 0) actually means setTimeout(..., 1) (check the docs), and there is no causal relationship between 1 ms of time passing and the data exchange with the server. Try socket.write('blah', () => socket.destroy());.

Btw, you describe this as hacky, but I'm not sure that is fair. Many things can cause an RST, but most of them are TCP protocol violations, and faking a bug in the OS TCP stack is probably not something node tests need to do.

The most common cause of an RST is exactly what you are doing: a socket is read-closed, but the peer continues to send data.

@pimterry
Copy link
Member Author

Get rid of the setTimeout

Sadly I tested socket.write('blah', () => socket.destroy()) before, and it's significantly less reliable - works a little over half the time, talking to a local server. Just manual tests, nothing rigorous, but I'm yet to see the setTimeout approach fail at all.

faking a bug in the OS TCP stack is probably not something node tests need to do.

I'm not sure about the reasons, but I'm seeing a fair few unexpected RST packets from end-user clients, in real-world HTTP traffic. My product is an HTTP debugger (httptoolkit.tech) - effectively a local HTTP proxy for developers - this is coming from my end-user error reporting, with RST packets being sent by whatever people are connecting to that. For me it'd be really useful to able to precisely reproduce that behaviour in my tests.

@sam-github
Copy link
Contributor

Any client that does write(socket, 'GET ...'); exit(); will cause RSTs when the server replies (exit implicitly closes all open sockets), you could be seeing variations of that.

@bnoordhuis
Copy link
Member

See libuv/libuv#1991 (comment), the main problem is the interaction between SO_LINGER and shutdown().

@pimterry
Copy link
Member Author

Since this suggestion, uv_tcp_close_reset is now available in libuv to do exactly this, for similar reasons. Presumably this could now presumably be implemented relatively easily by exposing that as a method on sockets.

I'd still find this useful. I'm in the same situation again right now of trying to test certain kinds of error reporting & handling with RST packets, and some cases are still not testable in node without a separate tool (Python).

@bnoordhuis
Copy link
Member

@pimterry That's right. Pull request welcome, the places to look at are src/tcp_wrap.cc and lib/net.js. Adding the method to the former exposes it on socket._handle in the latter (but only when it's a wrapper around a TCP connection, of course.)

I'm inclined to leave out a forwarding method on tls.Socket for now.

@kolontsov
Copy link

I'd like to prepare PR, any suggestions on method name?

@bnoordhuis
Copy link
Member

@kolontsov Just spitballing: net.Socket.prototype.reset()?

@kolontsov
Copy link

@bnoordhuis sounds great. ok.

@mmomtchev
Copy link
Contributor

Any progress on that one?

@micromashor
Copy link

I've been investigating this for a while, although I'm a bit lost in the source code...

Will update if I make any progress

@PupilTong
Copy link
Contributor

Any update on this issue?
This way works for node 16,
https://stackoverflow.com/questions/22836424/simulate-an-econnreset-error-on-a-node-js-net-socket-instance
but this will let the node process crash with unhandled exception.

@micromashor
Copy link

I haven't found any new ways, but if nodejs ever introduces a way to set socket options, the way to send a RST (on UNIX-like systems) is to use setsockopt(2) to enable SO_LINGER with a timeout of 0, and close(2) the socket. No idea for Windows though.

@PupilTong
Copy link
Contributor

Hi folks,
After some weeks of reading the code, I'm making some progress on this feature.

Now I can reset a TCP socket in my Javascript code, using my own Node build.

To achieve it, I added a wrap function TCPWrap::Reset for uv_tcp_close_reset in the TCPWrap Class.
I'm referencing the HandleWrap::Close code to implement it.

Also, I have some suggestions and questions to ask:

  1. Socket.prototype.resetAndDestroy could be better?
    Since uv_tcp_close_reset will reset and close the handle.
  2. If Socket is not a TCP handle (For example PIPE), do we throw an Error? Or do we do nothing?
    I guess we have three options: a) do nothing b) call destroy c)throw Error
  3. Are we going to emit the close event?
    I prefer 'yes'

PupilTong added a commit to PupilTong/node that referenced this issue May 15, 2022
make it possible to forcibly rest a tcp socket:

* add a new method `Socket.prototype.resetAndDestroy`

* add a new flag `resetAndClosing` to make `_destroy` calls
the `reset` instead of close while destroying a Socket.

* add new methods `TCPWrap::Reset` to be a wrap of `uv_tcp_close_reset`

* change `HandleWrap::state_` from private to protected.
This is essential for keeping the same behaviour between
`TCPWrap::Reset` and `HandleWrap::Close`

Fixes: nodejs#27428
@PupilTong
Copy link
Contributor

#43112
just created a draft PR for it. Still have some test cases to add.

PupilTong added a commit to PupilTong/node that referenced this issue May 18, 2022
make it possible to forcibly rest a tcp socket:

* add a new method `Socket.prototype.resetAndDestroy`

* add a new flag `resetAndClosing` to make `_destroy` calls
the `reset` instead of close while destroying a Socket.

* add new methods `TCPWrap::Reset` to be a wrap of `uv_tcp_close_reset`

* change `HandleWrap::state_` from private to protected.
This is essential for keeping the same behaviour between
`TCPWrap::Reset` and `HandleWrap::Close`

Fixes: nodejs#27428
PupilTong added a commit to PupilTong/node that referenced this issue May 18, 2022
make it possible to forcibly rest a tcp socket:

* add a new method `Socket.prototype.resetAndDestroy`

* add a new flag `resetAndClosing` to make `_destroy` calls
the `reset` instead of close while destroying a Socket.

* add new methods `TCPWrap::Reset` to be a wrap of `uv_tcp_close_reset`

* change `HandleWrap::state_` from private to protected.
This is essential for keeping the same behaviour between
`TCPWrap::Reset` and `HandleWrap::Close`

Fixes: nodejs#27428
PupilTong added a commit to PupilTong/node that referenced this issue May 18, 2022
make it possible to forcibly rest a tcp socket:

* add a new method `Socket.prototype.resetAndDestroy`

* add a new flag `resetAndClosing` to make `_destroy` calls
the `reset` instead of close while destroying a Socket.

* add new methods `TCPWrap::Reset` to be a wrap of `uv_tcp_close_reset`

* change `HandleWrap::state_` from private to protected.
This is essential for keeping the same behaviour between
`TCPWrap::Reset` and `HandleWrap::Close`

Fixes: nodejs#27428
PupilTong added a commit to PupilTong/node that referenced this issue May 18, 2022
make it possible to forcibly rest a tcp socket:

* add a new method `Socket.prototype.resetAndDestroy`

* add a new flag `resetAndClosing` to make `_destroy` calls
the `reset` instead of close while destroying a Socket.

* add new methods `TCPWrap::Reset` to be a wrap of `uv_tcp_close_reset`

* change `HandleWrap::state_` from private to protected.
This is essential for keeping the same behaviour between
`TCPWrap::Reset` and `HandleWrap::Close`

Fixes: nodejs#27428
bengl pushed a commit that referenced this issue May 30, 2022
Fixes: #27428
PR-URL: #43112
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
targos pushed a commit that referenced this issue Jul 12, 2022
Fixes: #27428
PR-URL: #43112
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
targos pushed a commit that referenced this issue Jul 31, 2022
Fixes: #27428
PR-URL: #43112
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Oct 10, 2022
Fixes: nodejs/node#27428
PR-URL: nodejs/node#43112
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@avivkeller avivkeller moved this from Awaiting Triage to Done in Node.js feature requests Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants