BIND 10 #1522: Implementatio of isc::server_common::SocketRequestor

[BIND 10 Development]

#1522: Implementatio of isc::server_common::SocketRequestor
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120110
  blocker                            |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  10
Feature Depending on Ticket:         |
  Socket creator                     |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:20 vorner]:

 > > I'm afraid I was misunderstood.  Assuming your comment is about
 > > "btw I suspect this is also the case for the boss", what I wanted to
 > > point out is that a bug in a client component could make the boss
 > > crash, too.
 >
 > Ah, right, that would be bad. But I believe boss ignores SIGPIPE already
 for some other reason.

 Ah, okay, you're right about the boss ignoring it.

 > > I understood that, but it can be done from the fact that the
 > > application dies itself.  In any case, as long as a socket could be in
 > > a broken state, I don't think it acceptable to pretend that it's safe
 > > to keep using it.  The choices I can see are: drop everything and
 > > create all necessary sockets from the scratch; similar to the previous
 > > one, but just abort the application and restart it; or separate the
 > > socket state and connection state.
 >
 > Maybe killing the application for now could be the way to go. If it
 turns out it happens in practice, we could have a look if it can be solved
 in a better way, right?

 That works for me.

 > > The difference is that in the current style recv_fd() can be called
 > > only after we examine the status, and it just wouldn't make sense to
 > > call it if we know the status code is bogus.  If we piggy-back the
 > > status in the recvmsg, we can detect the code is bogus while still
 > > retrieving the FD from the ancillary data, then close the FD (all
 > > within recv_fd()).
 >
 > But then, calling close on _something_ we got from the auxiliary data
 (if the status is bogus, then the socket may be as well) might lead to
 closing something completely unrelated. That doesn't sound too good to me
 either.

 The value returned as the ancillary data for SOL_SOCKET/SCM_RIGHTS
 should be more reliable than the 1 byte user data; the former should
 be a valid file descriptor unless the kernel has a bug.  The latter
 could be bogus due to a sender application's bug.  If we still want to
 cover kernel bugs, I don't see other options than aborting the program
 at this point.

 > > Okay, done, and while doing so I noticed that the current code could
 > > accept bogus 'protocol' and 'share mode'.  Even though it may be
 > > rejected after exchanging messages with the boss, it would be better
 > > to reject such a case at an earlier point.  I added the check and
 > > corresponding tests.
 >
 > Well, these are enums, so the application should not be able to create
 the invalid values in any legal way, but being too paranoid is probably
 better than being too careless.

 Exactly.  I added the code to be safe against a buggy or malicious
 caller.

 Now, unless I miss something there shouldn't be no more open issue for
 this ticket, right?

-- 
Ticket URL: <http://bind10.isc.org/ticket/1522#comment:21>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development