Re: [Nbd] NBD server terminates on SIGPIPE during negotiation

[Wouter Verhelst <w@...112...>]

Hi,

Sorry for the late reply. Last weekend[1] was fairly busy for me...

On Mon, Jan 28, 2013 at 08:57:46AM +0000, Tuomas Räsänen wrote:
> Hi
> 
> Problem: The listener/root server process terminates on SIGPIPE during
> negotiation.

Crap.

> This is hardly the desired behavior, since any malfunctioning client
> can brought the listener server down by closing the socket
> unexpectedly.

Yes, indeed. We've had CVE-2011-1925 a few years back, which basically
is a similar problem to this.

> A strace showing the core of the problem (thanks to Juha):
> 
> 01: select(5, [4], NULL, NULL, NULL)        = 1 (in [4])
> 02: write(1, "accept, ", 8)                 = 8
> 03: accept(4, {sa_family=AF_INET, sin_port=htons(45734), sin_addr=inet_addr("10.249.8.2")}, [16]) = 5
> 04: write(5, "NBDMAGIC", 8)                 = 8
> 05: write(5, "IHAVEOPT", 8)                 = 8
> 06: write(5, "\0\1", 2)                     = 2
> 07: read(5, 0x7fff07bea2c0, 4)              = -1 ECONNRESET (Connection reset by peer)
> 08: write(2, "Error: Negotiation failed/4: Con"..., 63) = 63
> 09: read(5, "", 8)                          = 0
> 10: read(5, "", 4)                          = 0
> 11: writev(5, [{"\0\3\350\211\4Ue\251", 8}, {"\0\0\0\0", 4}, {"\200\0\0\1", 4}, {"\0\0\0\0", 4}, {NULL, 0}], 5) = -1 EPIPE (Broken pipe)
> --- SIGPIPE (Broken pipe) @ 0 (0) ---
> 
> The trace above can be traced back to serveloop() and
> negotiate(). select() is called from serveloop(), the modern socket is
> ready and accepted and the negotiation steps 1-3 (writes 04, 05 and
> 06) are successful. Then something happens to the client which causes
> it to close the socket. The server gets notified about this with
> ECONNRESET (07), and it's logged (08), but then the server continues
> the negotiation process as if nothing had happened (09 and 10).

That's obviously wrong; when we have an ECONNRESET, we need to close our
side of the connection and abort negotiation, rather than trying to
continue.

> The
> problem boils down to the next writev() (11): writing to the broken
> pipe causes SIGPIPE to be sent and since this is all is happening in
> the listener/root process context and SIGPIPE action is the default
> (Term), the process terminates.
> 
> (Obviously, there is a reason why client behaves as it behaves, but
> that is out of the scope of this report. The point is that the server
> should not fail even if one client misbehaves.)
> 
> How this should be fixed?
> 
> #1 Set the default action of SIGPIPE to Ign.
> 
> This is simplest to do, but not the right thing in my opinion.

It feels utterly wrong to me. If the client isn't talking to us anymore,
we need to stop, not try to continue. Doing otherwise could cause
different issues down the road; I don't want to go there.

While the current situation is problematic, this is because it can kill
the master process; not because of the kill in itself.

> It would probably work and fix this specific issue, but in my opinion
> the current way of pushing through the negotation procedure no matter
> what happens seems a bit flawed.

I doubt it would, actually.

> #2 Move the negotation procedure to the child process.
> 
> This would be quite clean way to ensure that catastrophic failure in
> one connection would not affect other clients. But it definitely would
> not remove the need for more robust error handling in general. And could
> it have some nasty unexpected consequences? It seems that not, but someone
> who knows the complete codebase should verify this.

I don't think it would have nasty consequences; negotiation should only
change the environment for the client we're talking to, anyway. If it
changes different things, too, that's a bug. I don't think it's buggy in
*that* way.

> #3 Fix the error handling.
> 
> Fix the error handling in negotiate() to handle all failure situations
> and abort the process if nothing can be done. I think this is something
> which should be done anyways. The current way of walking through the
> procedure no matter if it fails or not is a bit ugly.

Yes, negotiate() should be fixed. It should probably be rewritten.

I wrote it the way it is because I was a bit lazy at the time, and
because I usually feel that writing the same code twice is almost always
the wrong thing to do. However, the result is unreadable, hard to
maintain, and pretty hairy.

> #4 The combination: #1 + #2 + #3
> 
> At this point, I'd would probably go with combining at least #1 and
> #3. I'm not sure about the #2, but if it makes sense too, then I don't
> see any problem in combining all three.

I don't think #1 is correct. I would instead suggest to combine #2 and
#3. Changing the code so that a client is always talking to a child
process and never to a parent, then the worst it can ever do in the
presence of buggy server-side code is kill its own connection; that's
not really a problem.

Improving error handling certainly is never a bad idea.

But allowing a server to continue in the face of an obviously wrong
situation feels utterly utterly wrong to me. In the worst case, it could
cause a server to go berserk upon disconnect of a client, and have it
eat all CPU or memory or whatever resource. That's not going to pass my
review ;-)

[1] https://fosdem.org/2013/about/#team

-- 
Copyshops should do vouchers. So that next time some bureaucracy requires you
to mail a form in triplicate, you can mail it just once, add a voucher, and
save on postage.