Winbind patch fixes broken client. Please have a look.

[Hannes Schmidt]

Andrew Bartlett wrote:
> Hannes Schmidt wrote:
>> 
>> Even if this is complete crap, I would appreciate any feedback
>> saying so. 
> 
> Firstly, don't expect instant responses - particularly over the
> weekend...

Sure, I guess I was getting a little too excited.

> 
>> A couple of days ago I posted a problem report to the samba list
>> with some diagnostics. I seem to have found a
>> solution on my own and provide a patch here. The problem is related
>> to the fact that the winbind client code in
>> libnss_winbind.so leaves the handle to the winbind pipe open, for
>> obvious reasons. This is perfectly ok, but some
>> programs get confused if this handle is 0,1 or 2, which they expect
>> to be a handle to a different file. The patch
>> makes the winbind client (libnss_winbind.so) code more robust.
>> 
>> Consider the following fragment of broken application code.
>> 
>> if( fork() == 0 ) {
>>     /* child */
>>     close( 0 );             /* next open() will return 0 (stdin) */
>>     getpwent(...);        /* pid changed, so wb_common.c will
>> re-open the pipe to winbindd, the handle will be 0 */
>>     fd = open( ... );     /* returned fd is *not* 0 although the
>> code below assumes this */
>>     execve( ... );         /* executed program reads from stdin
>> which - unintentionally - is the winbind pipe */
>> } else {
>>     /* parent */
>>     ...
>> }
>> 
>> Obviously, this code is broken and could be easily fixed by doing the
>> getpwent() before the close(). Unfortunately
>> the version of cron installed on my system is broken in a more
>> complex but similar way. There is probably more
>> broken code like this in other standard unix software, especially
>> since it's such a subtle mistake.
> 
> It is rather - well spotted for figuring it out.
> 
> How do other nss libraries deal with this?  NIS and LDAP both open
> sockets - find out if there is a standard solution to the problem.
> Personally, I don't like the idea of messing with a program's file
> descriptors - but this is a problem we will have to cope with.

I'll have a look at it. 

Regarding messing around with fds: An alternative would be to only 
leave the pipe handle open if it is > 2. It will get re-opened next time. 
This might slow down daemon processes which detach themselves 
from stdin and stdout.
 
>> The provided patch to wb_common.c makes sure that the handle to the
>> winbindd pipe is not 0, 1 or 2.
> 
> For future reference, patches should be in 'diff -u' format, if
> possible.

Makes sense.
 
>> *** /root/samba-2.2.5/source/nsswitch/wb_common.c Wed Jun 19
>> 03:13:44 2002 --- wb_common.c Thu Jul 25 16:36:39 2002
>> ***************
>> *** 94,99 ****
>> --- 94,120 ----
>>    }
>>   }
>> 
>> + /* Returns a duplicate of the given file descriptor that is
>> guranteed +    not to be 0, 1 or 2 (stdin, stdout or stderr
>> respectively), unless +    an error occurs. If the given fd is
>> invalid, it will be returned +    unchanged. If a different error
>> occurs, the returned file handle +    will still be valid but it may
>> be 0, 1 or 2. If a valid file +    handle is returned and the
>> returned handle is different to the +    original one, the original
>> one will be closed. If dup() is broken, +    this function might
>> never return. */ 
> 
> I don't like that idea.  It should fail - infinite loops are bad...

Yes, if we decide to use this solution I will have to limit the
recursion or turn it into a loop with a limited amount of
iterations.

>> + int make_nonstd_fd(int fd)
>> + {
>> +  int saved_fd;
>> +  if (0 >= fd && fd <= 2) {
>> +      fd = dup(saved_fd = fd);
>> +      if (fd == -1) return saved_fd;
>> +      fd = make_nonstd_fd(fd);
>> +      close(saved_fd);
>> +  }
>> +  return fd;
>> + }
>> +
>>   /* Connect to winbindd socket */
>> 
>>   int winbind_open_pipe_sock(void)
>> ***************
>> *** 158,164 ****
>>    if ((winbindd_fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
>>     return -1;
>>    }
>> !
>>    if (connect(winbindd_fd, (struct sockaddr *)&sunaddr,
>>         sizeof(sunaddr)) == -1) {
>>     close_sock();
>> --- 179,188 ----
>>    if ((winbindd_fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
>>     return -1;
>>    }
>> !
>> !  winbindd_fd = make_nonstd_fd( winbindd_fd );
>> !
>> !
>>    if (connect(winbindd_fd, (struct sockaddr *)&sunaddr,
>>         sizeof(sunaddr)) == -1) {
>>     close_sock();
>> ***************
>> *** 167,172 ****
>> --- 191,197 ----
>> 
>>    return winbindd_fd;
>>   }
>> +
>> 
>>   /* Write data to winbindd socket */