[PATCH] nss: Add ERANGE testing to tst-nss-test4 (bug 33361)

[DJ Delorie]

"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.

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.

> +#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.

> -  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.

> +  /* 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.

> +      /* 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.

> +      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).

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)

> +	      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.

> +	  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...