Add hal transmitter and socket support for remote hal #434
Conversation
| sock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); | ||
| server_address.sin_family = AF_INET; | ||
| server_address.sin_port = htons(port_requested); | ||
| server_address.sin_addr.s_addr = INADDR_ANY; |
There was a problem hiding this comment.
Always binding to all interfaces, with no way to override, seems like a possible security concern.
There was a problem hiding this comment.
what would you prefer here? I'm fairly new to sockets.
There was a problem hiding this comment.
See e.g. Apache's configuration, https://httpd.apache.org/docs/2.4/bind.html, where users can specify the IP address to listen to in addition to the port. How this should look in code I'm not entirely sure of, it depends on how this PR later ends up used, which I have not looked at yet.
There was a problem hiding this comment.
I'm happy to extend this to an optional address and default to this (and document that) - would you be happy with that?
There was a problem hiding this comment.
That seems okay, though keep in mind that depending on the address, we may also need AF_INET6 instead of AF_INET.
Question: since the purpose of the HAL process is to take untrusted code from a remote client and execute it, what is the overall security going to look like?
There was a problem hiding this comment.
it does add risk, but the user should be aware that this does remotely run processes, so your suggestion of limiting it to a particular IP address sounds good. Also it's not got any superuser priviledges.
For that matter, you can override the libhal as a shared library and grab things that way.
|
|
||
| hal_socket_transmitter::error_code hal_socket_transmitter::setup_connection( | ||
| bool server) { | ||
| // @return 0 if success, otherwise -1 and errno set |
There was a problem hiding this comment.
Does a @return comment work here?
coldav
force-pushed
the
colin/hal_transmitter
branch
4 times, most recently
from
4c8f584
to
0729da9
Compare
coldav
force-pushed
the
colin/hal_transmitter
branch
5 times, most recently
from
db1cdd2
to
a612fb4
Compare
Add base class hal_transmitter which can be used for simple send/ receive operations of data. This can be used by client and servers for different models of communication. Also added a socket version which will be the default use case.
coldav
force-pushed
the
colin/hal_transmitter
branch
from
a612fb4
to
207e068
Compare
| } | ||
|
|
||
| int hal_socket_transmitter::listen() { | ||
| if (const int res = ::listen(sock, 1) != 0) { |
There was a problem hiding this comment.
This means res is initialised to ::listen(sock, 1) != 0, i.e. it will always be zero or one, rather than returning the result of ::listen(sock, 1). Also, the if (res != 0) return res; else return 0; logic looks overly complicated, that amounts to just return res;.
But given that the API here is "1 is failure, 0 is success", whereas below for accept(), it's "-1 is failure, anything else, possibly zero, is the file descriptor that must not be used", I wonder if it's worth updating both to just return bool?
There was a problem hiding this comment.
This pattern of const int res = ... != 0 appears in a few other places where that does not look intentional.
| // don't support port 0 | ||
| if (port_requested == 0) { | ||
| if (debug_enabled()) { | ||
| (void)fprintf(stderr, "port 0 requested. This is disallowed\n"); |
There was a problem hiding this comment.
Given that port 0 is the default, this may not actually have been requested, would an error message along the lines of "The port to listen on must be specified" work?
coldav
force-pushed
the
colin/hal_transmitter
branch
from
207e068
to
dcc0a29
Compare
Overview
Add hal transmitter and socket support for remote hal
Reason for change
part of the support needed for communications for remote hal support
Description of change
Add base class hal_transmitter which can be used for simple send/ receive operations of data. This can be used by client and servers for different models of communication.
Also added a socket version which will be the default use case.