Winbind patch fixes broken client. Please have a look.

[Andrew Bartlett]

Hannes Schmidt wrote:
> 
> As you suggested, I checked how other NSS libraries deal with this problem.
> The libnss_files module in glibc doesn't leave files open like
> libnss_winbind.
> 
> E.g., getpwent() (I should say its implementation in libnss_files) and
> setpwent()
> open /etc/passwd and endpwent() closes it. getpwnam() and getpwuid() close
> it before returning.
> 
> I didn't completely understand libnss_nis, but it seems to do likewise.

nss_ldap would be worth a look.   It does keep it's fd open, so should
be more like winbind.

> I noticed another interesting thing: all the above NSS libraries set the
> close-on-exec flag on the files they open. This is a good idea for two
> reasons:
> 
> (A) security - see http://www.pgp.com/research/covert/advisories/028.asp.
> 
> (B) saving resources - an exec() overwrites all data, even the
> shared libraries data. Consequently, the variable which holds the pipe/file
> handle gets reset too and there is no way to close the file. It will get
> opened
> a second time. For each exec() one file handle will be wasted.

Very interesting.  This would certainly 'fix' your problem, and seems
like a good idea.

> My patch fixes both problems - it sets the close-on-exec flag for the
> winbindd
> pipe handle and it makes sure that the handle isn't 0, 1, 2. I tested it on
> my three
> production servers. No problems.
> 
> diff -u -x *.o -x *.so -x *.po samba-2.2.5/source/nsswitch/wb_common.c
> samba-2.2.5-modified/source/nsswitch/wb_common.c
> --- samba-2.2.5/source/nsswitch/wb_common.c Wed Jun 19 03:13:44 2002
> +++ samba-2.2.5-modified/source/nsswitch/wb_common.c Wed Jul 31 15:54:10
> 2002
> @@ -102,6 +102,7 @@
>   static pid_t our_pid;
>   struct stat st;
>   pstring path;
> + int old_fd, result, flags;
> 
>   if (our_pid != getpid()) {
>    close_sock();
> @@ -159,6 +160,30 @@
>    return -1;
>   }
> 
> + /* Make sure socket handle isn't stdin, stdout or stderr */
> +
> + if (winbindd_fd < 3) {
> +  old_fd = winbindd_fd;
> +  if ((winbindd_fd = fcntl(winbindd_fd, F_DUPFD, 3)) == -1) {

The only thing that worries me is portability.  How portable is this?

> +   winbindd_fd = old_fd;
> +   close_sock();
> +   return -1;
> +  }
> +  close( old_fd );
> + }
> +
> + /* Socket should be closed on exec() */
> +
> + result = flags = fcntl(winbindd_fd, F_GETFD, 0);
> + if (flags >= 0) {
> +  flags != FD_CLOEXEC;
> +  result = fcntl( winbindd_fd, F_SETFD, flags );

And in particular, how portable is these...

> + }
> + if (result < 0) {
> +  close_sock();
> +  return -1;
> + }
> +
>   if (connect(winbindd_fd, (struct sockaddr *)&sunaddr,
>        sizeof(sunaddr)) == -1) {
>    close_sock();
> diff -u -x *.o -x *.so -x *.po
> samba-2.2.5/source/nsswitch/winbind_nss_config.h
> samba-2.2.5-modified/source/nsswitch/winbind_nss_config.h
> --- samba-2.2.5/source/nsswitch/winbind_nss_config.h Wed Jun 19 03:13:44
> 2002
> +++ samba-2.2.5-modified/source/nsswitch/winbind_nss_config.h Wed Jul 31
> 15:21:25 2002
> @@ -62,6 +62,10 @@
>  #include <string.h>
>  #endif
> 
> +#ifdef HAVE_FCNTL_H
> +#include <fcntl.h>
> +#endif
> +
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <errno.h>
> 

Thankyou very much for this.  The portability needs to be to quite a few
systems, as this code is used for all systems, not just systems with
nsswitch etc.

If you can get back to me on those (and implement configure
tests/alternate behaviours if required) I think I'll be able to apply
it.

Nice work!

Andrew Bartlett

-- 
Andrew Bartlett                                 abartlet at pcug.org.au
Manager, Authentication Subsystems, Samba Team  abartlet at samba.org
Student Network Administrator, Hawker College   abartlet at hawkerc.net
http://samba.org     http://build.samba.org     http://hawkerc.net