Surplus of errno_assert() leading to deamon crash

[lytboris]

A daemon is a program that is designed to run forever so every single error that is not fatal should be handled and the show must go on. Currently ZMQ has 404 errno_assert calls - 404 ways to make a daemon crash with SIGABRT. Please consider this function from tcp.cpp:

void zmq::tune_tcp_socket (fd_t s_)
{
// Disable Nagle's algorithm. We are doing data batching on 0MQ level,
// so using Nagle wouldn't improve throughput in anyway, but it would
// hurt latency.
int nodelay = 1;
int rc = setsockopt (s_, IPPROTO_TCP, TCP_NODELAY, (char*) &nodelay,
sizeof (int));
#ifdef ZMQ_HAVE_WINDOWS
wsa_assert (rc != SOCKET_ERROR);
#else
errno_assert (rc == 0);
#endif

#ifdef ZMQ_HAVE_OPENVMS
// Disable delayed acknowledgements as they hurt latency significantly.
int nodelack = 1;
rc = setsockopt (s_, IPPROTO_TCP, TCP_NODELACK, (char*) &nodelack,
sizeof (int));
errno_assert (rc != SOCKET_ERROR);
#endif
}

When setsockopt() returns an error, your daemon would crash. And there is a trivial error-free scenario when this could happen - remote side can send TCP Reset packet that will immediately invalidate the socket but instead of reconnecting, ZMQ will crash whole app.

I was debugging my app that coredumped at this particular function:

Thread 1 (Thread 802007c00 (LWP 101563/firsthop-receiver)):
#0 0x0000000801896dcc in thr_kill () from /lib/libc.so.7
#1 0x000000080193d72b in abort () from /lib/libc.so.7
#2 0x0000000000415ac1 in zmq::zmq_abort (errmsg_=Could not find the frame base for "zmq::zmq_abort(char const*)".
) at src/err.cpp:84
#3 0x0000000000453a6e in zmq::tune_tcp_socket (s_=17) at src/tcp.cpp:60
#4 0x0000000000454524 in zmq::tcp_connecter_t::out_event (this=0x80285a600) at src/tcp_connecter.cpp:134
#5 0x0000000000416be6 in zmq::kqueue_t::loop (this=0x802051300) at src/kqueue.cpp:205
#6 0x0000000000416ce5 in zmq::kqueue_t::worker_routine (arg_=0x802051300) at src/kqueue.cpp:222
#7 0x0000000000434bd8 in thread_routine (arg_=0x802051380) at src/thread.cpp:96
#8 0x0000000801618e14 in pthread_getprio () from /lib/libthr.so.3
#9 0x0000000000000000 in ?? ()
(gdb) thread 1
[Switching to thread 1 (Thread 802007c00 (LWP 101563/firsthop-receiver))]#3 0x0000000000453a6e in zmq::tune_tcp_socket (s_=17)
at src/tcp.cpp:60
60 errno_assert (rc == 0);
(gdb) p errstr
$4 = 0x801b7b240 "Connection reset by peer"
(gdb)

Sure I can rewrite this function to ignore failure non-disabled Naggle and delayed-ACKs, but 402 of errno_assert()s will remain in code. Am I missing something?

[bluca]

This is all intentional and by design:

http://zguide.zeromq.org/php:chapter2#toc17
http://grokbase.com/t/zeromq/zeromq-dev/08cf6qpw56/error-handling-assert
https://en.wikipedia.org/wiki/Fail-fast

[lytboris]

Well, I do error-checking for each ZMQ call but the thing I am trying to tell is that this particular crash was not produced or induced by ZMQ API call - it is something inside ZMQ itself made this crash happen. The app is a simple SUB app that does read_zmq() on single ZMQ socket and inbound RST packet from a publisher crashed the app.

[bluca]

Please read again the links and the discussion - these are all errors that are intentionally causing a sigabort so that they are found immediately and fixed

[lytboris]

@bluca, I got your point clear.
Sometimes setsockopt() calls may fail because of external cause - e.g. faulty WAN connection that can not be fixed and in these cases I do expect from ZMQ to behave as described in your first link: as robust as possible against external attacks and errors.
So I do agree that most of asserts are there for good but setsockopt() code should be altered to provide robust to external threats.

[bluca]

The problem in this case is that simply ignoring the error won't do - as the inline comment suggests, these options are needed for the correct functioning of the library, so I'm not sure if there's an alternative

[lytboris]

I don't ask for simple ignoring :)
One can treat errno == ECONNABORTED (and may be EINTR) as non-fatal thing (much alike connection timeout) - emit warning, close that broken socket and try again. How's about that?

[lytboris]

And please take a look on trivial scenario I made for your reference to prove that something with setsockopt() should be changed: a ZMQ-based publisher that is open to the Internet and a hacker.

Compile server and client, run server and start client. You will get server stopped with SIGABRT in less than a minute.

hammerclient.zip

[jakecobb]

Can confirm this client reliably crashes our 4.2.0 services, tested against both ROUTER and PUB sockets on a TCP endpoint (although reading the report I would not expect the ZMQ socket pattern to matter).

[sem-hub]

Sadly you have no chance to control it within your application. It's an easy DOS attack vector.
You can rebuild 0mq library with -DNDEBUG flag but you lost all errors checking this way.

[Asmod4n]

Didn't something like this happen in vs. 2 or earlier? E.g. where it was too strict how a internet facing socket should ack from external issues. It got resolved afaik.
Isn't it the zmq way to be as rebust as possible from external attacks and crash hard on internal errors?

[jakecobb]

@Asmod4n Yes, there's even an entry in the current FAQ about it:

Is it true that it is not safe to use ZeroMQ over the internet because it will crash?

Earlier versions of the ZeroMQ library (before 2.1) were not very resilient against "fuzzing" attacks. A malformed packet or garbage data could cause an old version of the library to assert and exit. Since the release of 2.1, all reported cases of assertions caused by bad data have been fixed. If your testing uncovers a problem in this area, please file a bug report.

[evoskuil]

While fail fast is the intended paradigm, it is meant to apply only to unrecoverable internal failures, which constitute either bugs or underlying platform and/or dependency instability. Data received over an externally-facing socket should never be able to bring down the process otherwise.

[bluca]

TCP_NODELAY and the other options are used because they are necessary and cannot be just ignored. If there's a better solution to handle this critical failure, please send a PR to implement it.

[jakecobb]

I'm looking into a possible PR, but there's some disagreement in the code about which error codes should be asserted or not. Here are the three sets I've found for *nix platforms:

In tcp_listener.cpp, these are non-asserting:

EAGAIN
EWOULDBLOCK
EINTR
ECONNABORTED
EPROTO
ENOBUFS
ENOMEM
EMFILE
ENFILE

In tcp_connector.cpp, only these assert:

EBADF
ENOPROTOOPT
ENOTSOCK
ENOBUFS

And in socks_connector.cpp, these are non-asserting:

ECONNREFUSED
ECONNRESET
ETIMEDOUT
EHOSTUNREACH
ENETUNREACH
ENETDOWN
EINVAL

When I crash an OS X server with the hammerclient, errno is EINVAL (22, "Invalid argument").