[erlang-questions] inet:getstat on non-socket crashes the VM?

[Per Hedeland]

On 2018-06-25 02:42, Rickard Green wrote:
> 
> 
> On Fri, Jun 22, 2018 at 1:53 PM, Bekes, Andras G <Andras.Bekes@REDACTED <mailto:Andras.Bekes@REDACTED>> wrote:
>  >
>  > Hi Erlangers,
>  >
>  >
>  >
>  > I noticed that calling inet:getstat/1 is a dangerous operation, as it can kill the whole VM if used inappropriately.
>  >
>  >
>  >
>  > It is easy to reproduce:
>  >
>  >
>  >
>  > Eshell V9.3  (abort with ^G)
>  >
>  > 1> application:start(sasl).
>  >
>  > ok
>  >
>  > 2> [inet:getstat(X) || X<-erlang:ports()].
>  >
>  > Failed to write to erl_child_setup: 9
>  >
>  > Crash dump is being written to: erl_crash.dump...done
>  >
>  >
>  >
>  > Without sasl, it doesnt kill the VM, but the terminal gets into a faulty state and further work is not possible.
>  >
>  > I know calling it on a non-socket is an erroneous action, but crashing the VM doesnt seem to be the most appropriate consequence.
>  >
>  > What do you think?
>  >
>  >
>  >
>  > Regards,
>  >
>  >
>  >
>  > Andras G. Bekes
>  >
>  >
>  > _______________________________________________
>  > erlang-questions mailing list
>  > erlang-questions@REDACTED <mailto:erlang-questions@REDACTED>
>  > http://erlang.org/mailman/listinfo/erlang-questions
>  >
> 
> Broadcasting random stuff around is dangerous. Different ports (and processes) have different protocols that they expect to be used. A message that means something for one port may mean something 
> completely different for another port.

Agreed in principle - inet:getstat/1 and many other socket operations
(I forget which one we ran into this problem with) end up calling
erlang:port_control/3, where the 'Operation' argument is indeed an
arbitrary integer with semantics that are specific to the driver / port
program referenced by the 'Port' argument - BUT:

a) Programming errors do happen, and IMHO intentionally terminating the
VM as a means of error handling (which is what the "forker" driver does
in this case) is really not acceptable for a system with all the
sophisticated error handling and expected robustness of Erlang/OTP, and

b) it's a reasonable expectation that *all* drivers and port programs,
and specifically the drivers and port programs included in the
Erlang/OTP distribution, at least do a basic validation of the
'Operation' argument, and I would even suggest that

c) it would be reasonable for the drivers and port programs included in
the Erlang/OTP distribution to use unique values for the 'Operation'
argument, and even have a documented range of those values, to make it
possible for user-written drivers and port programs to avoid using them.

Anyway, in this particular case, the patch below (for OTP 20.3.6), which
just addresses b) for the "forker" driver, but thereby in most cases
also avoids a), seems like a good idea to me... (AFAIR, prim_inet will
actually turn the 'badarg' into 'einval', but it's still way better than
the driver terminating the VM after failing to use some random and
potentially un-initialized arguments to do what it *thought* was
requested...)

--Per Hedeland

--- a/erts/emulator/sys/unix/sys_drivers.c
+++ b/erts/emulator/sys/unix/sys_drivers.c
@@ -1851,6 +1851,11 @@ static ErlDrvSSizeT forker_control(ErlDrvData e, unsigned int cmd, char *buf,
      ErlDrvPort port_num = (ErlDrvPort)e;
      int res;

+    /* fail with badarg if wrong port_control call */
+    if (cmd != 'S') {
+	return -1;
+    }
+
      driver_enq(port_num, buf, len);
      if (driver_sizeq(port_num) > sizeof(*proto)) {
          return 0;