diff mbox series

[1/1] nss: files-initgroups.c use shared nss parse tools

Message ID 20260304221715.1703198-3-riehecky@fnal.gov (mailing list archive)
State Changes Requested
Headers
Series [1/1] nss: files-initgroups.c use shared nss parse tools |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
redhat-pt-bot/TryBot-32bit fail Patch caused testsuite regressions
linaro-tcwg-bot/tcwg_glibc_check--master-arm fail Test failed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 fail Test failed

Commit Message

Patrick Riehecky March 4, 2026, 10:16 p.m. UTC 
  From: Pat Riehecky <riehecky@fnal.gov>

files-initgroups.c open-coded line reading using __getline,
fgetpos/fsetpos, and __feof_unlocked.  The interface from
files-XXX.c uses __nss_readline and __nss_parse_line_result.

Replace the local file handling with __nss_readline and
__nss_parse_line_result to align files-initgroups.c with
files-XXX.c.

stdio_ext.h is no longer needed and is removed.

No behaviour change intended.

Fixes: https://sourceware.org/bugzilla/show_bug.cgi?id=33960
Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
---
 nss/nss_files/files-initgroups.c | 74 +++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 29 deletions(-)

Comments

Florian Weimer March 5, 2026, 7:45 a.m. UTC | #1 
* Patrick Riehecky:

> @@ -53,34 +51,52 @@ _nss_files_initgroups_dyn (const char *user, gid_t group, long int *start,
>    /* We have to iterate over the entire file.  */
>    while (1)
>      {
> +      off64_t original_offset;
> +      int ret = __nss_readline (stream, tmpbuf.data, tmpbuf.length,
> +                                &original_offset);
> +      if (ret == ENOENT)
> +        /* End of file.  */
> +        break;
> +      else if (ret != 0)
> +        {
> +          *errnop = ret;
> +          status = NSS_STATUS_UNAVAIL;
> +          break;
> +        }

I think this is missing logic to resize the buffer.  Looking at
internal_getgrouplist, I don't see support for handling the ERANGE error
that __nss_readline may produce.  In the old code, getline handled this
internally.

In general, getline is more efficient than retrying __nss_readline, even
with an exponential buffer resizing policy.  Some /etc/group files
contain very long lines, with many group members, but group lists for
individual users might still be very short, so that a retry is not
needed.  This could be the reason why initgroups was not converted.

Thanks,
Florian
Patrick Riehecky March 5, 2026, 2:17 p.m. UTC | #2 
>In general, getline is more efficient than retrying __nss_readline,
> even with an exponential buffer resizing policy.

It sounds like I headed in the wrong direction... I'm fine to abandon this plan rather than perform more invasive work on the resolver. The current release works fine.

My environment doesn't have very large group files, so I don't have a great way to test those regressions.

Thanks for the prompt review!

Pat
Carlos O'Donell March 9, 2026, 1:28 p.m. UTC | #3 
On 3/5/26 9:17 AM, Patrick Riehecky wrote:
>> In general, getline is more efficient than retrying
>> __nss_readline, even with an exponential buffer resizing policy.
> 
> It sounds like I headed in the wrong direction... I'm fine to
> abandon this plan rather than perform more invasive work on the
> resolver. The current release works fine.
> 
> My environment doesn't have very large group files, so I don't have
> a great way to test those regressions.
> 
> Thanks for the prompt review!

Please note that pre-commit CI fails for this patch on the nscd-tst-nscd-basic.
https://patchwork.sourceware.org/project/glibc/patch/20260304221715.1703198-3-riehecky@fnal.gov/
Florian Weimer March 9, 2026, 1:43 p.m. UTC | #4 
* Carlos O'Donell:

> On 3/5/26 9:17 AM, Patrick Riehecky wrote:
>>> In general, getline is more efficient than retrying
>>> __nss_readline, even with an exponential buffer resizing policy.
>> 
>> It sounds like I headed in the wrong direction... I'm fine to
>> abandon this plan rather than perform more invasive work on the
>> resolver. The current release works fine.
>> 
>> My environment doesn't have very large group files, so I don't have
>> a great way to test those regressions.
>> 
>> Thanks for the prompt review!
>
> Please note that pre-commit CI fails for this patch on the nscd-tst-nscd-basic.
> https://patchwork.sourceware.org/project/glibc/patch/20260304221715.1703198-3-riehecky@fnal.gov/

Yeah, was worring if we had a test for the error condition I mentioned
above.  And indeed this test covers the scenario:

tst-nscd-basic.c:228: numeric comparison failure
   left: 1 (0x1); from: getgrouplist ("user3", 1500, groups, &n)
  right: 8998 (0x2326); from: expected
tst-nscd-basic.c:229: numeric comparison failure
   left: 1 (0x1); from: n
  right: 8998 (0x2326); from: expected
error: tst-nscd-basic.c:235: not true: found[i]
error: tst-nscd-basic.c:235: not true: found[i]
error: tst-nscd-basic.c:235: not true: found[i]
[…]

Thanks,
Florian
diff mbox series

Patch

diff --git a/nss/nss_files/files-initgroups.c b/nss/nss_files/files-initgroups.c
index 018b51e25f..dd6c7b4b82 100644
--- a/nss/nss_files/files-initgroups.c
+++ b/nss/nss_files/files-initgroups.c
@@ -19,7 +19,6 @@ 
 #include <errno.h>
 #include <grp.h>
 #include <nss.h>
-#include <stdio_ext.h>
 #include <string.h>
 #include <sys/param.h>
 #include <stdbool.h>
@@ -41,7 +40,6 @@  _nss_files_initgroups_dyn (const char *user, gid_t group, long int *start,
     }
 
   char *line = NULL;
-  size_t linelen = 0;
   enum nss_status status = NSS_STATUS_SUCCESS;
   bool any = false;
 
@@ -53,34 +51,52 @@  _nss_files_initgroups_dyn (const char *user, gid_t group, long int *start,
   /* We have to iterate over the entire file.  */
   while (1)
     {
-      fpos_t pos;
-      fgetpos (stream, &pos);
-      ssize_t n = __getline (&line, &linelen, stream);
-      if (n < 0)
-	{
-	  if (! __feof_unlocked (stream))
-	    status = ((*errnop = errno) == ENOMEM
-		      ? NSS_STATUS_TRYAGAIN : NSS_STATUS_UNAVAIL);
-	  break;
-	}
-
+      off64_t original_offset;
+      int ret = __nss_readline (stream, tmpbuf.data, tmpbuf.length,
+                                &original_offset);
+      if (ret == ENOENT)
+        /* End of file.  */
+        break;
+      else if (ret != 0)
+        {
+          *errnop = ret;
+          status = NSS_STATUS_UNAVAIL;
+          break;
+        }
+
+      /* For grent, sizeof (struct parser_data) == 0: ENTDATA is not defined.
+         The parser from fgetgrent.c is used for grent rather than defining
+         its own LINE_PARSER with an accompanying ENTDATA struct.
+         The struct therefore contains only the zero-length linebuffer
+         member. So tmpbuf.data serves as both the line buffer and parser
+         scratch space. The parser tokenizes the line in-place and carves
+         the gr_mem pointer array into the remaining space up to the size of
+         tmpbuf.data + tmpbuf.length.  */
       struct group grp;
-      int res = _nss_files_parse_grent (line, &grp,
-					tmpbuf.data, tmpbuf.length, errnop);
-      if (res == -1)
-	{
-	  if (!scratch_buffer_grow (&tmpbuf))
-	    {
-	      *errnop = ENOMEM;
-	      status = NSS_STATUS_TRYAGAIN;
-	      goto out;
-	    }
-	  /* Reread current line, the parser has clobbered it.  */
-	  fsetpos (stream, &pos);
-	  continue;
-	}
-
-      if (res > 0 && grp.gr_gid != group)
+      int parse_ret = _nss_files_parse_grent (tmpbuf.data, &grp, tmpbuf.data,
+                                              tmpbuf.length, errnop);
+      ret = __nss_parse_line_result (stream, original_offset, parse_ret);
+      if (ret == EINVAL)
+        /* Malformed line; skip it.  */
+        continue;
+      else if (ret == ERANGE)
+        {
+          if (!scratch_buffer_grow (&tmpbuf))
+            {
+              *errnop = ENOMEM;
+              status = NSS_STATUS_TRYAGAIN;
+              goto out;
+            }
+          continue;
+        }
+      else if (ret != 0)
+        {
+          *errnop = ret;
+          status = NSS_STATUS_UNAVAIL;
+          goto out;
+        }
+
+      if (grp.gr_gid != group)
 	for (char **m = grp.gr_mem; *m != NULL; ++m)
 	  if (strcmp (*m, user) == 0)
 	    {