search

KatelynHaworth / go-tproxy

Linux Transparent Proxy library for Golang
MIT License
305 stars 59 forks source link

Double closing file descriptor #1

Closed ppacher closed 5 years ago

ppacher commented 7 years ago

Hi,

Thanks for this project! Just needed TPROXY in Go and thought I would need to implement it on my own, luckily you already tackled that :)

I just wanted to point out two minor "bugs": In the UDP and TCP implementations, you are using syscall.Close to close the cloned file-descriptor for the listeners. However, due to the defered Close() on the *os.File, this actually double-closes the FD. It doesn't do any harm but just wanted to point it out.

tproxy_tcp.go#L73 tproxy-tcp.go#L31 and #L36

Cheers, Patrick

KatelynHaworth commented 7 years ago

Hey Patrick,

I'm glad this library is of some help to someone. With the closing of the *os.File variables, the closing of those files doesn't affect the underlying socket, it just closes the access to the file descriptor definer. Where as calling syscall.Close on the file descriptor will instruct the kernel to close the socket.

See https://godoc.org/net#TCPConn.File

ppacher commented 7 years ago

Hi,

maybe I'm wrong but for my understanding listener.File() returns a clone(3)ed file descriptor pointing to the same socket structure. The defered file.Close() will actually close the file descriptor by invoking close(3), which has the same effect as syscall.Close(). The kernel is not going to do shutdown the socket as long as active file descriptors are attached to it. Since the listener maintains it's own fd, the socket will be kept open. (That's also how childs can inherit sockets from parents). If you really want to force the listener socket to be closed, you'll need to call listener.Close() or shutdown(3) on the clone file-descriptor. May also refer to the manpage of close(3), I think it's "more or less" clear on that.

Please correct me when I'm wrong.

Br, Patrick

KatelynHaworth commented 7 years ago

Hey Patrick,

From some reading and going through what I implemented you are correct, I should be calling Close() on the listener instead of the cloned fd when a error occurs. Good catch!

If you want, submit a pull request with the changes and I'll approve them. Also feel free to add a contributes section in the README and add yourself to it.

Kind regards,

Liam Haworth.

KatelynHaworth commented 5 years ago

Issue resolved via PR #7