Fixes for PR #508 and #509 ('botched 'Bad netgroup' error message' and

'cycle in netgroup check too greedy').

PR #508 is apparently due to an inconsistency in the way the 4.4BSD
netgroup code deals with bad netgroups. When 4.4BSD code encounters
a badly formed netgroup entry (e.g. (somehost,-somedomain), which,
because of the missing comma between the '-' and 'somedomain,' has
only 2 fields instead of 3), it generates an error message and
then bails out without doing any more processing on the netgroup
containing the bad entry. Conversely, every other *NIX in the world
that usees netgroups just tries to parse the entry as best it can
and then silently continues on its way.

The result is that two bad things happen: 1) we ignore other valid entries
within the netgroup containing the bogus entry, which prevents
us from interoperating with other systems that don't behave this way,
and 2) by printing an error to stderr from inside libc, we hose certain
programs, in this case rlogind. In the problem report, Bill Fenner
noted that the 'B' from 'Bad' was missing, and that rlogind exited
immediately after generating the error. The missing 'B' is apparently
not caused by any problem in getnetgrent.c; more likely it's getting
swallowed up by rlogind somehow, and the error message itself causes
rlogind to become confused. I was able to duplicate this problem and
discovered that running a simple test program on my FreeBSD system
resulted in a properly formatted (if confusing) error, whereas triggering
the error by trying to rlogin to the machine yielded the missing 'B'
problem.

Anyway, the fixes for this are as follows:

- The error message has been reformatted so that it prints out more useful
  information (e.g. Bad entry (somehost,-somedomain) in netgroup "foo").
  We check for NULL entries so that we don't print '(null)' anymore too. :)

- Rearranged things in parse_netgrp()  so that we make a best guess at
  what bad entries are supposed to look like and then continue processing
  instead of bailing out.

- Even though the error message has been cleaned up, it's wrapped inside
  a #ifdef DEBUG. This way we match the behavior of other systems. Since we
  now handle the error condition better anyway, this error message becomes
  less important.

PR #507 is another case of inconsistency. The code that handles
duplicate/circular netgroup entries isn't really 'too greedy; -- it's
just too noisy. If you have a netgroup containing duplicate entries,
the code actually does the right thing, but it also generates an error
message. As with the 'Bad netgroup' message, spewing this out from
inside libc can also hose certain programs (like rlogind). Again, no
other system generates an error message in this case.

The only change here is to hide the error message inside an #ifdef DEBUG.
Like the other message, it's largely superfluous since the code handles
the condition correctly.

Note that PR #510 (+@netgroup host matching in /etc/hosts.equiv) is still
being investigated. I haven't been able to duplicate it myself, and I
strongly suspect it to be a configuration problem of some kind. However,
I'm leaving all three PRs open until I get 510 resolved just for the
sake of paranoia.

1 files changed, 43 insertions, 7 deletions

diff --git a/lib/libc/gen/getnetgrent.c b/lib/libc/gen/getnetgrent.c
index 1ad838e..658cf63 100644
--- a/lib/libc/gen/getnetgrent.c
+++ b/lib/libc/gen/getnetgrent.c
@@ -192,7 +192,7 @@ innetgr(group, host, user, dom)
 	char *hst, *usr, *dm;
 
 	/* Sanity check */
-
+	
 	if (group == NULL || !strlen(group))
 		return (0);
 
@@ -217,6 +217,9 @@ parse_netgrp(group)
 {
 	register char *spos, *epos;
 	register int len, strpos;
+#ifdef DEBUG
+	register int fields;
+#endif
 	char *pos, *gpos;
 	struct netgrp *grp;
 	struct linelist *lp = linehead;
@@ -233,7 +236,15 @@ parse_netgrp(group)
 	    (lp = read_for_group(group)) == (struct linelist *)0)
 		return (1);
 	if (lp->l_parsed) {
+#ifdef DEBUG
+		/*
+		 * This error message is largely superflous since the
+		 * code handles the error condition sucessfully, and
+		 * spewing it out from inside libc can actually hose
+		 * certain programs.
+		 */
 		fprintf(stderr, "Cycle in netgroup %s\n", lp->l_groupname);
+#endif
 		return (1);
 	} else
 		lp->l_parsed = 1;
@@ -247,8 +258,14 @@ parse_netgrp(group)
 			grouphead.gr = grp;
 			pos++;
 			gpos = strsep(&pos, ")");
+#ifdef DEBUG
+			fields = 0;
+#endif
 			for (strpos = 0; strpos < 3; strpos++) {
 				if (spos = strsep(&gpos, ",")) {
+#ifdef DEBUG
+					fields++;
+#endif
 					while (*spos == ' ' || *spos == '\t')
 						spos++;
 					if (epos = strpbrk(spos, " \t")) {
@@ -262,9 +279,32 @@ parse_netgrp(group)
 						bcopy(spos, grp->ng_str[strpos],
 							len + 1);
 					}
-				} else
-					goto errout;
+				} else {
+					/*
+					 * All other systems I've tested
+					 * return NULL for empty netgroup
+					 * fields. It's up to user programs
+					 * to handle the NULLs appropriately.
+					 */
+					grp->ng_str[strpos] = NULL;
+				}
 			}
+#ifdef DEBUG
+			/*
+			 * Note: on other platforms, malformed netgroup
+			 * entries are not normally flagged. While we
+			 * can catch bad entries and report them, we should
+			 * stay silent by default for compatibility's sake.
+			 */
+			if (fields < 3)
+					fprintf(stderr, "Bad entry (%s%s%s%s%s) in netgroup \"%s\"\n",
+						grp->ng_str[NG_HOST] == NULL ? "" : grp->ng_str[NG_HOST],
+						grp->ng_str[NG_USER] == NULL ? "" : ",",
+						grp->ng_str[NG_USER] == NULL ? "" : grp->ng_str[NG_USER],
+						grp->ng_str[NG_DOM] == NULL ? "" : ",",
+						grp->ng_str[NG_DOM] == NULL ? "" : grp->ng_str[NG_DOM],
+						lp->l_groupname);
+#endif
 		} else {
 			spos = strsep(&pos, ", \t");
 			if (parse_netgrp(spos))
@@ -276,10 +316,6 @@ parse_netgrp(group)
 				pos++;
 	}
 	return (0);
-errout:
-	fprintf(stderr, "Bad netgroup %s at ..%s\n", lp->l_groupname,
-		spos);
-	return (1);
 }
 
 /*