Re: [PATCH] nis+/getgrent: don't set errno when there are no more entries

[Jim Meyering <jim at meyering dot net>]

Jim Meyering <jim@meyering.net> wrote:
> There is a bug in group-related nis+ support.
> [FYI, I've just filed this as http://bugzilla.redhat.com/437945, too]
>
> First symptom was this report against id from coreutils':
>
>     http://savannah.gnu.org/bugs/?22505
>
> Here's code to demonstrate the bug:
>
> cat <<\EOF > getgrent-test.c
> #include <sys/types.h>
> #include <grp.h>
> #include <errno.h>
> #include <stdio.h>
>
> static int
> count_groups (void)
> {
>   int count = 0;
>
>   setgrent ();
>   while (1)
>     {
>       errno = 0;
>       struct group *grp = getgrent ();
>       if (!grp)
>         break;
>       ++count;
>     }
>
>   if (errno != 0)
>     {
>       perror ("getgrent failed");
>       count = -1;
>     }
>
>   int saved_errno = errno;
>   endgrent ();
>   errno = saved_errno;
>
>   return count;
> }
>
> int
> main ()
> {
>   return count_groups () < 0;
> }
> EOF
> gcc -O -W -Wall getgrent-test.c && ./a.out && echo ok
> ======================================
>
> When I run that on a rawhide system on which /etc/nsswitch.conf has this:
>
>   group: files
>
> I get an exit status of 0, as expected.
> However, when I change nsswitch.conf, appending " nisplus"
> (and I haven't configured NIS+ at all):
>
>   group: files nisplus
>
> and run ./a.out again, I get this output:
>
>   getgrent failed: No such file or directory
>   [Exit 1]
>
> Investigating, I saw suspicious differences between the
> _nss_nisplus_getgrgid_r and _nss_nisplus_getgrnam_r
> functions in nis/nss_nisplus/nisplus-grp.c.  They should be nearly
> identical.  Eliminating one difference fixes the bug.
> Eliminating the other avoids a useless invocation of __set_errno.
> Here's the patch, which also normalizes some inconsequential parts,
> too, so that the difference between the two functions is minimal.
>
> I've confirmed this also affects at least RHEL4.6 and FC5.

The patch I posted minutes ago is worth applying, but was not
relevant to the getgrent bug.  Sorry about the mix-up.

Here's the patch that is likely to actually solve the problem.

2008-03-18  Jim Meyering  <meyering@redhat.com>

	nis+/getgrent: don't set errno when there are no more entries
	* nis/nis_call.c (__libc_lock_define_initialized): Don't let
	ENOENT from failure to stat "/var/nis/NIS_COLD_START" affect
	leak out to confuse the getgrent caller, who must be able to
	distinguish a return value of NULL indicating no more values
	and a return value of NULL indicating an error occurred.

diff --git a/nis/nis_call.c b/nis/nis_call.c
index c571e8f..3020141 100644
--- a/nis/nis_call.c
+++ b/nis/nis_call.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 1997, 1998, 2001, 2004, 2005, 2006, 2007
+/* Copyright (C) 1997, 1998, 2001, 2004, 2005, 2006, 2007, 2008
    Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Thorsten Kukuk <kukuk@vt.uni-paderborn.de>, 1997.
@@ -591,9 +591,11 @@ nis_server_cache_search (const_nis_name name, int search_parent,
   char *addr;
   XDR xdrs;
   struct stat64 st;
+  int saved_errno = errno;

   if (stat64 ("/var/nis/NIS_COLD_START", &st) < 0)
     st.st_mtime = nis_cold_start_mtime + 1;
+  __set_errno (saved_errno);

   __libc_lock_lock (nis_server_cache_lock);

--
1.5.5.rc0.7.g57e83