Add option to listen to unix socket #541
Conversation
websockify/websockifyserver.py
Outdated
| raise | ||
| lsock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) | ||
| lsock.bind(self.listen_sock) | ||
| os.chmod(self.listen_sock, self.listen_sock_mode) |
There was a problem hiding this comment.
I wonder if this is race:y...
The safe way is to use umask. I.e.:
oldmask = os.umask(0777 ^ self.listen_sock_mode)
try:
lsock.bind(self.listen_sock)
finally:
os.umask(oldmask)There was a problem hiding this comment.
Not 100% sure that there isn´t a race condition, so far it worked for me, but that does not say too much.
With chmod you`d be sure you´re only changing permissions on that one file (while being unfamiliar with the project and only looking at that one line of code), while with umask there might be ways you end up changing permissions on other files that are created simultaneously. Though, if you´re sure about that change can do ofc.
Also if there where a race condition, would not this suffer from the same issue? Just instead of chmod failing it would change back to the old umask before the file was created?
And, whats this for? 0777 ^
There was a problem hiding this comment.
With chmod you`d be sure you´re only changing permissions on that one file (while being unfamiliar with the project and only looking at that one line of code), while with umask there might be ways you end up changing permissions on other files that are created simultaneously. Though, if you´re sure about that change can do ofc.
We don't have threads (at least not at this point), so the finally should be enough to make sure we don't mess with any other files.
Also if there where a race condition, would not this suffer from the same issue? Just instead of chmod failing it would change back to the old umask before the file was created?
The race isn't about chmod() failing, but rather about someone opening the socket before chmod() has been run. This new code is safe since there is no point where the socket exists with the wrong permissions.
And, whats this for? 0777 ^
umask() is inverted in that it states the bits you don't want, rather than the bits you want. Did you check if you go the expected result with the new code?
There was a problem hiding this comment.
Sorry for the long time without any response.
You´re right about that. It´s indeed needed.
I did make that change too. Besides the lack of tests it now looks good? Gonna have to take a look at the existing tests and see whats tested there. Seems hard to test proxies. It probably would be enough if I just manage to validate the new settings work and send any packet at all over the socket file. As all the rest should (tm) work the same.
There was a problem hiding this comment.
Accidentally posted this with the wrong account. Did not know I was logged in with another account. Mostly using gitlab myself. Should really delete the other one not much point in it.
websockify/websocketproxy.py
Outdated
| @@ -492,6 +494,12 @@ def websockify_init(): | |||
| parser.add_option("--ssl-ciphers", action="store", | |||
| help="list of ciphers allowed for connection. For a list of " | |||
| "supported ciphers run `openssl ciphers`") | |||
| parser.add_option("--unix-listen", | |||
| dest="listen_sock", | |||
There was a problem hiding this comment.
We've avoided using dest for pretty much everything else, so let's try to keep the same pattern for new stuff.
There was a problem hiding this comment.
Would --listen-unix-sock / listen_unix_sock and --listen-unix-sock-mode / listen_unix_sock_mode be fine instead? Or remove the -sock to make it shorter?
Personally, I find the unix part to be more telling because there are many other socket types one can listen to but with unix listen I would generally default to thinking thats a unix socket automatically. Though, maybe that´s just me.
There was a problem hiding this comment.
Did remove dest now. Though, the reason I did pick the names I did is because there already is --unix-target, so made some sence to call it --unix-listen in the CLI option to me. However, the variables in the code for listen all start with listen listen_host / listen_port / listen_fd.
For the --inetd option dest is not used but it´s rewritten later to another variable.
if opts.inetd:
opts.listen_fd = sys.stdin.fileno()
Should I do that or just leave it as is? (with the variables called unix_listen / unix_listen_mode)
There was a problem hiding this comment.
I think your new suggestion looks good. There is still "listen" in the variable name, and we have a nice consistency with the arguments and the variables matching.
|
Did you check if there are some unit tests that could be added for this change? |
No haven´t checked how to unit test this. |
|
I did do that now. Still, have a branch with the individual commits if anything went wrong can try again. Though, the resulting commit looks fine to me. Also tested if it works and it does. |
#539