[PATCH] Avoid downloading whole NIS services.by{,service}name for getservby{name,port}{,_r} (, NULL)

[PATCH] Avoid downloading whole NIS services.by{,service}name for getservby{name,port}{,_r} (, NULL)

[Jakub Jelinek]

Hi!

Only lightly tested so far.
It is not neccessary to download, allocate and copy whole services
map and then search through it.
Instead, we can search already in the foreach callback and if we find
something tell the caller we don't need further input.

BTW: __xdr_ypresp_all uses the foreach callback return value as
0 -> need further data, != 0 break the loop.
But saveit callbacks I see return 0 when further data should be
examined and YP_FALSE (== 0) on error (e.g. memory failures).
This certainly doesn't sound right.

2004-03-30  Jakub Jelinek  <jakub@redhat.com>

	* nis/nss_nis/nis-service.c (struct search_t): New type.
	(dosearch): New function.
	(_nss_nis_getservbyname_r): Use it.  Call yp_get_default_domain
	unconditionally.
	(_nss_nis_getservbyport_r): Likewise.

--- libc/nis/nss_nis/nis-service.c.jj	2003-01-18 11:18:41.000000000 +0100
+++ libc/nis/nss_nis/nis-service.c	2004-03-30 00:49:23.341175888 +0200
@@ -51,6 +51,18 @@ typedef struct intern_t intern_t;
 
 static intern_t intern = { NULL, NULL };
 
+struct search_t
+{
+  const char *name;
+  const char *proto;
+  int port;
+  enum nss_status status;
+  struct servent *serv;
+  char *buffer;
+  size_t buflen;
+  int *errnop;
+};
+
 static int
 saveit (int instatus, char *inkey, int inkeylen, char *inval,
         int invallen, char *indata)
@@ -87,6 +99,68 @@ saveit (int instatus, char *inkey, int i
   return 0;
 }
 
+static int
+dosearch (int instatus, char *inkey, int inkeylen, char *inval,
+	  int invallen, char *indata)
+{
+  struct search_t *req = (struct search_t *) indata;
+
+  if (instatus != YP_TRUE)
+    return 1;
+
+  if (inkey && inkeylen > 0 && inval && invallen > 0)
+    {
+      struct parser_data *pdata = (void *) req->buffer;
+      int parse_res;
+      char *p;
+
+      if ((size_t) (invallen + 1) > req->buflen)
+	{
+	  *req->errnop = ERANGE;
+	  req->status = NSS_STATUS_TRYAGAIN;
+	  return 1;
+	}
+
+      p = strncpy (req->buffer, inval, invallen);
+      req->buffer[invallen] = '\0';
+      while (isspace (*p))
+        ++p;
+
+      parse_res = _nss_files_parse_servent (p, req->serv, pdata, req->buflen,
+					    req->errnop);
+      if (parse_res == -1)
+	{
+	  req->status = NSS_STATUS_TRYAGAIN;
+	  return 1;
+	}
+
+      if (!parse_res)
+        return 0;
+
+      if (req->proto != NULL && strcmp (req->serv->s_proto, req->proto) != 0)
+	return 0;
+
+      if (req->port != -1 && req->serv->s_port != req->port)
+	return 0;
+
+      if (req->name != NULL && strcmp (req->serv->s_name, req->name) != 0)
+	{
+	  char **cp;
+	  for (cp = req->serv->s_aliases; *cp; cp++)
+	    if (strcmp (req->name, *cp) == 0)
+	      break;
+
+	  if (*cp == NULL)
+	    return 0;
+	}
+
+      req->status = NSS_STATUS_SUCCESS;
+      return 1;
+    }
+
+  return 0;
+}
+
 static enum nss_status
 internal_nis_endservent (intern_t * intern)
 {
@@ -136,6 +210,7 @@ internal_nis_setservent (intern_t *inter
 
   return status;
 }
+
 enum nss_status
 _nss_nis_setservent (int stayopen)
 {
@@ -201,9 +276,8 @@ _nss_nis_getservbyname_r (const char *na
 			  struct servent *serv, char *buffer, size_t buflen,
 			  int *errnop)
 {
-  intern_t data = { NULL, NULL };
   enum nss_status status;
-  int found;
+  char *domain;
 
   if (name == NULL)
     {
@@ -211,19 +285,18 @@ _nss_nis_getservbyname_r (const char *na
       return NSS_STATUS_UNAVAIL;
     }
 
+  if (yp_get_default_domain (&domain))
+    return NSS_STATUS_UNAVAIL;
+
   /* If the protocol is given, we could try if our NIS server knows
      about services.byservicename map. If yes, we only need one query */
   if (protocol != NULL)
     {
       char key[strlen (name) + strlen (protocol) + 2];
-      char *cp, *domain, *result;
+      char *cp, *result;
       size_t keylen, len;
       int int_len;
 
-      /* If this fails, the other solution will also fail. */
-      if (yp_get_default_domain (&domain))
-	return NSS_STATUS_UNAVAIL;
-
       /* key is: "name/protocol" */
       cp = stpcpy (key, name);
       *cp++ = '/';
@@ -267,34 +340,25 @@ _nss_nis_getservbyname_r (const char *na
 	}
     }
 
-  status = internal_nis_setservent (&data);
-  if (status != NSS_STATUS_SUCCESS)
-    return status;
-
-  found = 0;
-  while (!found &&
-         ((status = internal_nis_getservent_r (serv, buffer, buflen, errnop,
-					       &data)) == NSS_STATUS_SUCCESS))
-    {
-      if (protocol == NULL || strcmp (serv->s_proto, protocol) == 0)
-	{
-	  char **cp;
-
-	  if (strcmp (serv->s_name, name) == 0)
-	    found = 1;
-	  else
-	    for (cp = serv->s_aliases; *cp; cp++)
-	      if (strcmp (name, *cp) == 0)
-		found = 1;
-	}
-    }
+  struct ypall_callback ypcb;
+  struct search_t req;
 
-  internal_nis_endservent (&data);
+  ypcb.foreach = dosearch;
+  ypcb.data = (char *) &req;
+  req.name = name;
+  req.proto = protocol;
+  req.port = -1;
+  req.serv = serv;
+  req.buffer = buffer;
+  req.buflen = buflen;
+  req.errnop = errnop;
+  req.status = NSS_STATUS_NOTFOUND;
+  status = yperr2nss (yp_all (domain, "services.byservicename", &ypcb));
 
-  if (!found && status == NSS_STATUS_SUCCESS)
-    return NSS_STATUS_NOTFOUND;
-  else
+  if (status != NSS_STATUS_SUCCESS)
     return status;
+
+  return req.status;
 }
 
 enum nss_status
@@ -302,22 +366,20 @@ _nss_nis_getservbyport_r (int port, cons
 			  struct servent *serv, char *buffer,
 			  size_t buflen, int *errnop)
 {
-  intern_t data = { NULL, NULL };
   enum nss_status status;
-  int found;
+  char *domain;
+
+  if (yp_get_default_domain (&domain))
+    return NSS_STATUS_UNAVAIL;
 
   /* If the protocol is given, we only need one query */
   if (protocol != NULL)
     {
       char key[100 + strlen (protocol) + 2];
-      char *domain, *result;
+      char *result;
       size_t keylen, len;
       int int_len;
 
-      /* If this fails, the other solution will also fail. */
-      if (yp_get_default_domain (&domain))
-	return NSS_STATUS_UNAVAIL;
-
       /* key is: "port/protocol" */
       keylen = snprintf (key, sizeof (key), "%d/%s", port, protocol);
       status = yperr2nss (yp_match (domain, "services.byname", key,
@@ -358,22 +420,26 @@ _nss_nis_getservbyport_r (int port, cons
 	}
     }
 
-  status = internal_nis_setservent (&data);
-  if (status != NSS_STATUS_SUCCESS)
-    return status;
+  if (port == -1)
+    return NSS_STATUS_NOTFOUND;
 
-  found = 0;
-  while (!found &&
-         ((status = internal_nis_getservent_r (serv, buffer, buflen, errnop,
-					       &data)) == NSS_STATUS_SUCCESS))
-    if (serv->s_port == port &&
-	(protocol == NULL || strcmp (serv->s_proto, protocol) == 0))
-      found = 1;
+  struct ypall_callback ypcb;
+  struct search_t req;
 
-  internal_nis_endservent (&data);
+  ypcb.foreach = dosearch;
+  ypcb.data = (char *) &req;
+  req.name = NULL;
+  req.proto = protocol;
+  req.port = port;
+  req.serv = serv;
+  req.buffer = buffer;
+  req.buflen = buflen;
+  req.errnop = errnop;
+  req.status = NSS_STATUS_NOTFOUND;
+  status = yperr2nss (yp_all (domain, "services.byname", &ypcb));
 
-  if (!found && status == NSS_STATUS_SUCCESS)
-    return NSS_STATUS_NOTFOUND;
-  else
+  if (status != NSS_STATUS_SUCCESS)
     return status;
+
+  return req.status;
 }

	Jakub

Re: [PATCH] Avoid downloading whole NIS services.by{,service}name for getservby{name,port}{,_r} (, NULL)

[Ulrich Drepper]

Seems to work nicely, I've applied it.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

Re: [PATCH] Avoid downloading whole NIS services.by{,service}name for getservby{name,port}{,_r} (, NULL)

[Thorsten Kukuk]

On Mon, Mar 29, Jakub Jelinek wrote:

> Hi!
> 
> Only lightly tested so far.
> It is not neccessary to download, allocate and copy whole services
> map and then search through it.
> Instead, we can search already in the foreach callback and if we find
> something tell the caller we don't need further input.
> 
> BTW: __xdr_ypresp_all uses the foreach callback return value as
> 0 -> need further data, != 0 break the loop.
> But saveit callbacks I see return 0 when further data should be
> examined and YP_FALSE (== 0) on error (e.g. memory failures).
> This certainly doesn't sound right.

There are two wrong checks:

  if (instatus != YP_TRUE)
    return instatus;

and

      if (newp == NULL)
        return YP_FALSE; /* We have no error code for out of memory */


are both wrong. I will look later at it and try to fix it.
Seems I mixed the instatus and with the return value of the foreach
function.

 Thanks,
  Thorsten

-- 
Thorsten Kukuk       http://www.suse.de/~kukuk/        kukuk@suse.de
SuSE Linux AG        Maxfeldstr. 5                 D-90409 Nuernberg
--------------------------------------------------------------------    
Key fingerprint = A368 676B 5E1B 3E46 CFCE  2D97 F8FD 4E23 56C6 FB4B

Re: [PATCH] Avoid downloading whole NIS services.by{,service}name for getservby{name,port}{,_r} (, NULL)

[Thorsten Kukuk]

[-- Attachment #1: Type: text/plain, Size: 1399 bytes --]

On Tue, Mar 30, Thorsten Kukuk wrote:

> On Mon, Mar 29, Jakub Jelinek wrote:
> 
> > Hi!
> > 
> > Only lightly tested so far.
> > It is not neccessary to download, allocate and copy whole services
> > map and then search through it.
> > Instead, we can search already in the foreach callback and if we find
> > something tell the caller we don't need further input.
> > 
> > BTW: __xdr_ypresp_all uses the foreach callback return value as
> > 0 -> need further data, != 0 break the loop.
> > But saveit callbacks I see return 0 when further data should be
> > examined and YP_FALSE (== 0) on error (e.g. memory failures).
> > This certainly doesn't sound right.
> 
> There are two wrong checks:
> 
>   if (instatus != YP_TRUE)
>     return instatus;
> 
> and
> 
>       if (newp == NULL)
>         return YP_FALSE; /* We have no error code for out of memory */
> 
> 
> are both wrong. I will look later at it and try to fix it.
> Seems I mixed the instatus and with the return value of the foreach
> function.

Attached is a patch to solve the problem. In error case, return
value should always be "1".

  Thorsten
-- 
Thorsten Kukuk       http://www.suse.de/~kukuk/        kukuk@suse.de
SuSE Linux AG        Maxfeldstr. 5                 D-90409 Nuernberg
--------------------------------------------------------------------    
Key fingerprint = A368 676B 5E1B 3E46 CFCE  2D97 F8FD 4E23 56C6 FB4B

[-- Attachment #2: glibc-2.3.3-nss_nis-return-code.diff --]
[-- Type: text/plain, Size: 3102 bytes --]

2004-04-02  Thorsten Kukuk  <kukuk@firun.suse.de>

	* nis/nss_nis/nis-ethers.c (saveit): Fix return codes in error case.
	* nis/nss_nis/nis-initgroups.c (saveit): Likewise.
	* nis/nss_nis/nis-proto.c (saveit): Likewise.
	* nis/nss_nis/nis-rpc.c (saveit): Likewise.
	* nis/nss_nis/nis-service.c (saveit): Likewise.

--- nis/nss_nis/nis-ethers.c
+++ nis/nss_nis/nis-ethers.c	2004/04/02 08:26:51
@@ -52,13 +52,13 @@
 	int invallen, char *indata)
 {
   if (instatus != YP_TRUE)
-    return instatus;
+    return 1;
 
   if (inkey && inkeylen > 0 && inval && invallen > 0)
     {
       struct response *newp = malloc (sizeof (struct response) + invallen + 1);
       if (newp == NULL)
-	return YP_FALSE; /* We have no error code for out of memory */
+	return 1; /* We have no error code for out of memory */
 
       if (start == NULL)
 	start = newp;
--- nis/nss_nis/nis-initgroups.c
+++ nis/nss_nis/nis-initgroups.c	2004/04/02 08:33:11
@@ -59,14 +59,14 @@
   intern_t *intern = (intern_t *) indata;
 
   if (instatus != YP_TRUE)
-    return instatus;
+    return 1;
 
   if (inkey && inkeylen > 0 && inval && invallen > 0)
     {
       struct response_t *newp = malloc (sizeof (struct response_t)
 					+ invallen + 1);
       if (newp == NULL)
-	return YP_FALSE; /* We have no error code for out of memory */
+	return 1; /* We have no error code for out of memory */
 
       if (intern->start == NULL)
 	intern->start = newp;
--- nis/nss_nis/nis-proto.c
+++ nis/nss_nis/nis-proto.c	2004/04/02 08:33:26
@@ -49,13 +49,13 @@
         int invallen, char *indata)
 {
   if (instatus != YP_TRUE)
-    return instatus;
+    return 1;
 
   if (inkey && inkeylen > 0 && inval && invallen > 0)
     {
       struct response *newp = malloc (sizeof (struct response) + invallen + 1);
       if (newp == NULL)
-	return YP_FALSE; /* We have no error code for out of memory */
+	return 1; /* We have no error code for out of memory */
 
       if (start == NULL)
 	start = newp;
--- nis/nss_nis/nis-rpc.c
+++ nis/nss_nis/nis-rpc.c	2004/04/02 08:33:40
@@ -57,14 +57,14 @@
   intern_t *intern = (intern_t *)indata;
 
   if (instatus != YP_TRUE)
-    return instatus;
+    return 1;
 
   if (inkey && inkeylen > 0 && inval && invallen > 0)
     {
       struct response_t *newp = malloc (sizeof (struct response_t)
 					+ invallen + 1);
       if (newp == NULL)
-	return YP_FALSE; /* We have no error code for out of memory */
+	return 1; /* We have no error code for out of memory */
 
       if (intern->start == NULL)
 	intern->start = newp;
--- nis/nss_nis/nis-service.c
+++ nis/nss_nis/nis-service.c	2004/04/02 08:34:14
@@ -70,14 +70,14 @@
   intern_t *intern = (intern_t *) indata;
 
   if (instatus != YP_TRUE)
-    return instatus;
+    return 1;
 
   if (inkey && inkeylen > 0 && inval && invallen > 0)
     {
       struct response_t *newp = malloc (sizeof (struct response_t)
 					+ invallen + 1);
       if (newp == NULL)
-	return YP_FALSE; /* We have no error code for out of memory */
+	return 1; /* We have no error code for out of memory */
 
       if (intern->start == NULL)
 	intern->start = newp;

Re: [PATCH] Avoid downloading whole NIS services.by{,service}name for getservby{name,port}{,_r} (, NULL)

[Ulrich Drepper]

Applied.  Although the nis-initgroups.c patch had an offset.  You might
want to check your sources.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖