EINTR, vev, and mgt_cli

[Devon H. O'Dell]

I came across a rather interesting (but probably super low priority) bug in
our Varnish tree recently while integrating some monitoring tools, and it
appears to still be present in Varnish 4. In particular, if the manager
receives a signal that is handled by the vev interface while it is waiting
for a CLI response from a blocked child, it is possible that the manager
blocks forever. This can happen when the following events occur:

Manager goes into mgt_cli_askchild, which calls VCLI_ReadResult. This calls
read_tmo, which calls poll(2), waiting for the child to put data on the fd.
The child gets the CLI request, and this causes the child to go catatonic
without crashing for whatever reason (let's say it's blocked in a syscall
as a strawman).

The manager receives a signal (let's say SIGTERM or SIGINT, since those
have vev handlers associated) before poll(2) times out. Vev's signal
handler is run, setting up the signal for asynchronous processing. After
exiting the signal handler, poll(2) returns -1 with errno being EINTR (but
poll(2) is not restartable, so SA_RESTART doesn't help the situation).
Read_tmo does not check poll(2) returning an error, and so it happily
continues into the read(2) call, which will never return because the child
will never write data onto the fd because its CLI thread is stuck.

In the meanwhile, all other threads in the child that are not blocked
continue happily, so traffic is still served (which could be a bad thing;
maybe you urgently need to ban an object).

Even worse, the manager will not be able to restart the child without
taking another signal to get it out of the read(2), which is at least
non-obvious. But a frustrated administrator might simply SIGKILL the
manager and child.

I get that the normal use case of Varnish is unlikely to see this issue,
but debugging it isn't likely to be easy for an administrator. I think
therefore it's worth fixing, but I'm not entirely sure what the correct fix
is for upstream purposes. What I've done in our tree is manage the timeout
outside of poll(2) when it returns EINTR, and either restart for the
remainder of the time, or simply return -1 with ETIMEDOUT. But there are
two problems:

 * When the timeout is zero, we theoretically bock indefinitely. The
problem here is that restarting the poll(2) will exhibit the same behavior
of blocking forever as long as the child CLI continues not to produce data.
However, treating EINTR as an error here has the possibility of
desynchronizing the manager from the child if the child ever produces a
result. (However, if it does not, at least the manager will reap it when
its next ping fails.)
 * It is unclear whether there are any consumers of libvarnish not using
vev who expect this behavior in read_tmo (though I struggle to come up with
a practical case that relies on continuing into read(2) after EINTR).

Does upstream Varnish even consider this a problem? if so, does anybody
have ideas for a better solution if the one I've implemented is not
desirable? (I've implemented the time delta check in our Varnish since we
don't have cases that do not time out, and we have infrastructure for
resynchronizing the CLI channel with a cookie.)

--dho
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://www.varnish-cache.org/lists/pipermail/varnish-dev/attachments/20160123/6a8299ae/attachment.html>