[PATCH] nss: Add ERANGE testing to tst-nss-test4 (bug 33361)
Carlos O'Donell
carlos@redhat.com
Fri Nov 7 23:09:28 GMT 2025
On 11/7/25 4:29 PM, DJ Delorie wrote:
> "Carlos O'Donell" <carlos@redhat.com> writes:
>> This adds testing for the fix added in commit:
>> 0fceed254559836b57ee05188deac649bc505d05
>> "nss: Group merge does not react to ERANGE during merge (bug 33361)"
>>
>> The in-use group size is increased large enough to trigger ERANGE
>> for initial buffers and cause a retry. The actualy size is
>> approximately twice that required to trigger the defect, though
>> any size larger than NSS_BUFLEN_GROUP triggers the defect.
>>
>> Without the fix the group is not merged and the failure is detected,
>> but with the fix the ERANGE error is handled, buffers are enlarged
>> and subsequently correctly merged.
>
> My two concerns on this, which I consider non-blocking (i.e. we could do
> better, but we don't need to ;), are:
>
> 1. The use of "files" instead of adding a test3 service, or
> containerizing the "files" service table.
See my comments below on this.
>
> 2. The location of the endgrent call
>
> However, I think the alignment tests need reworking. They happen to
> work with our implementation, but they are not correct.
Thanks, let me review...
>> +#include <assert.h>
>> #include <nss.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <string.h>
>> +#include <array_length.h>
>> +/* For NSS_BUFLEN_GROUP define. */
>> +#include "nss/grp.h"
>>
>> +#include <support/test-driver.h>
>> #include <support/support.h>
>
> Ok.
>
>> +/* Enough entries to exceed NSS_BUFLEN_GROUP and trigger ERANGE. */
>> +static char *group_2[256];
>>
>> +static char *merge_1[array_length(group_1) + array_length (group_2) - 1];
>
> Ok.
>
>> +/* In order to trigger ERANGE checking the minimum size of
>> + group_table_data2 should exceed NSS_BUFLEN_GROUP which is used
>> + internally by getgrgid. We use 8 bytes per group_2 string as
>> + a lower bound. */
>> +_Static_assert (sizeof (group_table_data2) + array_length (group_2) * 8
>> + >= NSS_BUFLEN_GROUP,
>> + "test group table size should exceed NSS_BUFLEN_GROUP");
>> +
>
> Ok.
>
>> /* This is the data we compare against. */
>> static struct group group_table[] = {
>> GRP_N(1, "name1", merge_1),
>
> Noting that group_table's "name1" group links to the merge1 list.
OK. Correct.
>> - int i;
>> + int i, member_cnt;
>
> Ok.
>
>> - __nss_configure_lookup ("group", "test1 [SUCCESS=merge] test2");
>> + /* At least 3 service modules are needed to reproduce BZ#33361. */
>> + __nss_configure_lookup ("group", "test1 [SUCCESS=merge] test2 files");
>
> I'll note that the content of the "files" service is not controlled
> here, but given we use "test names" for the groups instead of
> likely-to-conflict realistic names, this is probably not an issue.
For the test to pass falsely we would need files to contain the data the
test was expecting, which is highly unlikely?
Only in the scenario where an implementation defect occurs again would we
get data from files.
>> + /* Test increasing sizes of group_2 to see if we fail, starting with
>> + member_cnt == 1 to ensure we always check for no de-duplication
>> + e.g. { "foo", NULL } */
>> + for (member_cnt = 1; member_cnt < array_length (group_2); member_cnt++)
>> + {
>> + verbose_printf ("Outer loop - member_cnt is %d\n", member_cnt);
>
> Ok.
>
>> + /* Initialize group_2 */
>> + for (i = 0; i < member_cnt; i++)
>> + {
>> + /* Note that deduplication is NOT supposed to happen. */
>> + if (i == 0)
>> + group_2[i] = xstrdup ("foo");
>> + else
>> + group_2[i] = xasprintf ("foobar%d", i);
>> + }
>> + group_2[member_cnt] = NULL;
>
> Since member_cnt < array_length (256), member_cnt will max out at 255.
> group_2[member_cnt] thus stops at group_2[255]. Ok.
OK.
>> + /* Create the merged list to verify against */
>> +
>> + /* Copy group_1 to the merge list (excluding NULL) */
>> + for (i = 0; i < array_length (group_1) - 1; i++)
>> + {
>> + merge_1[i] = xasprintf ("%s", group_1[i]);
>> + verbose_printf ("MERGED LIST of [%d] is %s\n", i, merge_1[i]);
>> + }
>
> Ok.
>
>> + /* Add group_2 to the merge list */
>> + int group2_index = 0;
>> + for (i = array_length (group_1) - 1;
>> + i < array_length (group_1) - 1 + member_cnt; i++)
>> {
>> + merge_1[i] = xasprintf ("%s", group_2[group2_index++]);
>
> starts group_2 at [0], ok.
>
> last entry would be [3 - 1 + 255] vs size of [3 + 256 - 1]; ok.
>
>> + verbose_printf ("MERGED LIST of [%d] is %s\n", i, merge_1[i]);
>> + }
>> + merge_1[array_length(group_1) - 1 + member_cnt]= NULL;
>
> Ok.
>
>> + align_mask = __alignof__ (struct group *) - 1;
>
> This notes the alignment of a pointer pointing to "struct group", not
> the alignment of "struct group" itself.
Correct. I want to compare the alignment of the storage to what I get back.
>> + setgrent ();
>> +
>> + for (i = 0; group_table[i].gr_gid; ++i)
>> + {
>
> for each group in the list; only the first is the big merged table.
>
>> + g = getgrgid (group_table[i].gr_gid);
>> + if (g)
>> {
>> + retval += compare_groups (i, g, & group_table[i]);
>
> This compares the merged list. Note, if the "files" service *does*
> provide a matching group, this will fail.
>
>> + if ((uintptr_t)g & align_mask)
>> + {
>> + printf ("FAIL: [%d] unaligned group %p\n", i, g);
>> + ++retval;
>> + }
>
> This is comparing the alignment of the value of g (the address of what g
> points to), with the calculated alignment of the storage of g itself
> (not the storage that g points to).
Agreed, I need to drop the "*" above, which is a typo.
The value of g must be sufficiently aligned for the underlying storage.
> Consider an implentation with "struct group" needing 128-bit alignment,
> on a 32-bit host. align_mask would be for 32-bit (~0x03) but the
> pointer g must be aligned to 128-bit (~0x1f)
Correct.
How about this?
diff --git a/nss/tst-nss-test4.c b/nss/tst-nss-test4.c
index 0dc0ad2c29..2c6512341b 100644
--- a/nss/tst-nss-test4.c
+++ b/nss/tst-nss-test4.c
@@ -95,6 +95,7 @@ do_test (void)
int i, member_cnt;
struct group *g = NULL;
uintptr_t align_mask;
+ uintptr_t align_mem_mask;
/* At least 3 service modules are needed to reproduce BZ#33361. */
__nss_configure_lookup ("group", "test1 [SUCCESS=merge] test2 files");
@@ -136,7 +137,8 @@ do_test (void)
}
merge_1[array_length(group_1) - 1 + member_cnt]= NULL;
- align_mask = __alignof__ (struct group *) - 1;
+ align_mask = __alignof__ (struct group) - 1;
+ align_mem_mask = __alignof__ (char) - 1;
setgrent ();
@@ -151,7 +153,7 @@ do_test (void)
printf ("FAIL: [%d] unaligned group %p\n", i, g);
++retval;
}
- if ((uintptr_t)(g->gr_mem) & align_mask)
+ if ((uintptr_t)(g->gr_mem[0]) & align_mem_mask)
{
printf ("FAIL: [%d] unaligned member list %p\n",
i, g->gr_mem);
>> + if ((uintptr_t)(g->gr_mem) & align_mask)
>> + {
>> + printf ("FAIL: [%d] unaligned member list %p\n",
>> + i, g->gr_mem);
>> + ++retval;
>> + }
>> }
>
> This is comparing the alignment of a pointer to the alignment of a
> pointer, so is OK, but note that if you fix align_mask to be the
> alignment of "struct group", you'll need to fix this too.
The underlying storage is char, and so g->gr_mem[0] should be
sufficiently aligned. Unlikely that it won't be, but it's belt
and suspenders.
>> + else
>> {
>> + printf ("FAIL: [%d] group %u.%s not found\n", i,
>> + group_table[i].gr_gid, group_table[i].gr_name);
>> ++retval;
>> }
>> }
>
> Ok.
>
>>
>> - endgrent ();
>> + /* Free malloc'd array members (including the NULL) */
>> + for (i = 0; i < member_cnt; i++)
>> + free (group_2[i]);
>> + for (i = 0; i < array_length (group_1) + member_cnt; i++)
>> + free (merge_1[i]);
>> +
>> + endgrent ();
>> + }
>
> I don't think it makes a difference in this case (because we control the
> service list), but I think endgrent() should be called before you free
> the data it might be looking at...
>
Fixed. Good point for future reference.
--
Cheers,
Carlos.
More information about the Libc-alpha
mailing list