Harden put*ent functions against data injection [BZ #18724]

This patch addresses a “Bobby Tables” issue in the put*ent functions and
the getent program, similar to one of the recent libuser issues.

I believe this is just hardening because users of the put*ent functions
already have appropriate checks before they call these functions, so
this is definitely post-freeze material.

Tested on x86_64-redhat-linux-gnu.  Okay to commit after master reopens?

On 07/27/2015 11:18 AM, Florian Weimer wrote:
> This patch addresses a “Bobby Tables” issue in the put*ent functions and
> the getent program, similar to one of the recent libuser issues.
> 
> I believe this is just hardening because users of the put*ent functions
> already have appropriate checks before they call these functions, so
> this is definitely post-freeze material.
> 
> Tested on x86_64-redhat-linux-gnu.  Okay to commit after master reopens?

Looks good to me for 2.23 with testsuite comment nits fixed.

> -- Florian Weimer / Red Hat Product Security
> 
> 
> 0001-Harden-putpwent-putgrent-putspent-putspent-against-i.patch
> 
> 
> From ca909fd7c61e7821df24109182f8ec3201690e67 Mon Sep 17 00:00:00 2001
> Message-Id: <ca909fd7c61e7821df24109182f8ec3201690e67.1438010163.git.fweimer@redhat.com>
> From: Florian Weimer <fweimer@redhat.com>
> Date: Mon, 27 Jul 2015 17:13:05 +0200
> Subject: [PATCH] Harden putpwent, putgrent, putspent, putspent against
>  injection [BZ #18724]
> To: libc-alpha@sourceware.org
> 
> This prevents injection of ':' and '\n' into output functions which
> use the NSS files database syntax.  Critical fields (user/group names
> and file system paths) are checked strictly.  For backwards
> compatibility, the GECOS field is rewritten instead.
> 
> The getent program is adjusted to use the put*ent functions in libc,
> instead of local copies.  This changes the behavior of getent if user
> names start with '-' or '+'.
> ---
>  ChangeLog              |  38 +++++++++++
>  NEWS                   |   3 +-
>  grp/Makefile           |   2 +-
>  grp/putgrent.c         |  15 ++---
>  grp/tst-putgrent.c     | 165 ++++++++++++++++++++++++++++++++++++++++++++++++
>  gshadow/Makefile       |   2 +-
>  gshadow/putsgent.c     |  11 ++++
>  gshadow/tst-putsgent.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/nss.h          |  13 ++++
>  include/pwd.h          |   2 +-
>  nss/Makefile           |   8 ++-
>  nss/getent.c           |  76 +++-------------------
>  nss/rewrite_field.c    |  51 +++++++++++++++
>  nss/tst-field.c        | 100 +++++++++++++++++++++++++++++
>  nss/valid_field.c      |  31 +++++++++
>  nss/valid_list_field.c |  35 +++++++++++
>  pwd/Makefile           |   2 +-
>  pwd/putpwent.c         |  52 +++++++++-------
>  pwd/pwd.h              |   2 +-
>  pwd/tst-putpwent.c     | 166 +++++++++++++++++++++++++++++++++++++++++++++++++
>  shadow/Makefile        |   2 +-
>  shadow/putspent.c      |   9 +++
>  shadow/tst-putspent.c  | 162 +++++++++++++++++++++++++++++++++++++++++++++++
>  23 files changed, 1007 insertions(+), 106 deletions(-)
>  create mode 100644 grp/tst-putgrent.c
>  create mode 100644 gshadow/tst-putsgent.c
>  create mode 100644 nss/rewrite_field.c
>  create mode 100644 nss/tst-field.c
>  create mode 100644 nss/valid_field.c
>  create mode 100644 nss/valid_list_field.c
>  create mode 100644 pwd/tst-putpwent.c
>  create mode 100644 shadow/tst-putspent.c
> 
> diff --git a/ChangeLog b/ChangeLog
> index 3e202c4..e9d0335 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,41 @@
> +2015-07-27  Florian Weimer  <fweimer@redhat.com>
> +
> +	[BZ #18724]
> +	* include/nss.h (NSS_INVALID_FIELD_CHARACTERS): Define.
> +	(__nss_invalid_field_characters, __nss_valid_field)
> +	(__nss_valid_list_field, __nss_rewrite_field): Declare.
> +	* nss/valid_field.c, nss/valid_list_field, nss/rewrite_field.c,
> +	tst-field.c: New file.
> +	* nss/Makefile (routines): Add valid_field, rewrite_field.
> +	(tests-static): Define unconditionally.
> +	(tests): Include tests-static.
> +	[build-static-nss] (tests-static): Use append.
> +	[build-static-nss] (tests): Remove modification.
> +	* nss/getent.c (print_group): Call putgrent.  Report error.
> +	(print_gshadow): Call putsgent.  Report error.
> +	(print_passwd): Call putpwent.  Report error.
> +	(print_shadow): Call putspent.  Report error.
> +	* include/pwd.h: Include <nss.h> instead of <nss/nss.h>.
> +	* pwd/pwd.h (putpwent): Remove incorrect nonnull attribute.
> +	* pwd/putpwent.c (putpwent): Use ISO function definition.  Check
> +	name, password, directory, shell fields for valid syntax.  Rewrite
> +	GECOS field to match syntax.
> +	* pwd/Makefile (tests): Add tst-putpwent.
> +	* pwd/tst-putpwent.c: New file.
> +	* grp/putgrent.c (putgrent): Convert to ISO function definition.
> +	Check grName, grpasswd, gr_mem fields for valid syntax.
> +	Change loop variable i to size_t.
> +	* grp/Makefile (tests): Add tst-putgrent.
> +	* grp/tst-putgrent.c: New file.
> +	* shadow/putspent.c (putspent): Check sp_namp, sp_pwdp fields for
> +	valid syntax.
> +	* shadow/Makefile (tests): Add tst-putspent.
> +	* shadow/tst-putspent.c: New file.
> +	* gshadow/putsgent.c (putsgent): Check sg_namp, sg_passwd, sg_adm,
> +	sg_mem fields for valid syntax.
> +	* gshadow/Makefile (tests): Add tst-putsgent.
> +	* gshadow/tst-putsgent.c: New file.
> +
>  2015-07-26  Chung-Lin Tang  <cltang@codesourcery.com>
>  
>  	* sysdeps/nios2/dl-sysdep.h (DL_EXTERN_PROTECTED_DATA): Define.
> diff --git a/NEWS b/NEWS
> index 65b2172..649c04b 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -27,7 +27,8 @@ Version 2.22
>    18520, 18522, 18527, 18528, 18529, 18530, 18532, 18533, 18534, 18536,
>    18539, 18540, 18542, 18544, 18545, 18546, 18547, 18549, 18553, 18557,
>    18558, 18569, 18583, 18585, 18586, 18592, 18593, 18594, 18602, 18612,
> -  18613, 18619, 18633, 18641, 18643, 18648, 18657, 18676, 18694, 18696.
> +  18613, 18619, 18633, 18641, 18643, 18648, 18657, 18676, 18694, 18696,
> +  18724.
>  
>  * Cache information can be queried via sysconf() function on s390 e.g. with
>    _SC_LEVEL1_ICACHE_SIZE as argument.
> diff --git a/grp/Makefile b/grp/Makefile
> index c63b552..ed8cc2b 100644
> --- a/grp/Makefile
> +++ b/grp/Makefile
> @@ -28,7 +28,7 @@ routines := fgetgrent initgroups setgroups \
>  	    getgrent getgrgid getgrnam putgrent \
>  	    getgrent_r getgrgid_r getgrnam_r fgetgrent_r
>  
> -tests := testgrp
> +tests := testgrp tst-putgrent
>  
>  ifeq (yes,$(build-shared))
>  test-srcs :=  tst_fgetgrent
> diff --git a/grp/putgrent.c b/grp/putgrent.c
> index e4d662c..3c06d1a 100644
> --- a/grp/putgrent.c
> +++ b/grp/putgrent.c
> @@ -16,7 +16,9 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #include <errno.h>
> +#include <nss.h>
>  #include <stdio.h>
> +#include <string.h>
>  #include <grp.h>
>  
>  #define flockfile(s) _IO_flockfile (s)
> @@ -27,13 +29,14 @@
>  /* Write an entry to the given stream.
>     This must know the format of the group file.  */
>  int
> -putgrent (gr, stream)
> -     const struct group *gr;
> -     FILE *stream;
> +putgrent (const struct group *gr, FILE *stream)
>  {
>    int retval;
>  
> -  if (__glibc_unlikely (gr == NULL) || __glibc_unlikely (stream == NULL))
> +  if (__glibc_unlikely (gr == NULL) || __glibc_unlikely (stream == NULL)
> +      || gr->gr_name == NULL || !__nss_valid_field (gr->gr_name)
> +      || !__nss_valid_field (gr->gr_passwd)
> +      || !__nss_valid_list_field (gr->gr_mem))

OK.

>      {
>        __set_errno (EINVAL);
>        return -1;
> @@ -56,9 +59,7 @@ putgrent (gr, stream)
>  
>    if (gr->gr_mem != NULL)
>      {
> -      int i;
> -
> -      for (i = 0 ; gr->gr_mem[i] != NULL; i++)
> +      for (size_t i = 0 ; gr->gr_mem[i] != NULL; i++)

OK.

>  	if (fprintf (stream, i == 0 ? "%s" : ",%s", gr->gr_mem[i]) < 0)
>  	  {
>  	    /* What else can we do?  */
> diff --git a/grp/tst-putgrent.c b/grp/tst-putgrent.c
> new file mode 100644
> index 0000000..3f259ca
> --- /dev/null
> +++ b/grp/tst-putgrent.c
> @@ -0,0 +1,165 @@

Needs first line description of test with reference BZ #.

> +/* Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <grp.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdlib.h>
> +
> +static int errors;
> +
> +static void
> +check (struct group e, const char *expected)
> +{
> +  char *buf;
> +  size_t buf_size;
> +  FILE *f = open_memstream (&buf, &buf_size);
> +
> +  if (f == NULL)
> +    {
> +      printf ("open_memstream: %m\n");
> +      ++errors;
> +      return;
> +    }
> +
> +  int ret = putgrent (&e, f);
> +
> +  if (expected == NULL)
> +    {
> +      if (ret == -1)
> +	{
> +	  if (errno != EINVAL)
> +	    {
> +	      printf ("putgrent: unexpected error code: %m\n");
> +	      ++errors;
> +	    }
> +	}
> +      else
> +	{
> +	  printf ("putgrent: unexpected success (\"%s\", \"%s\")\n",
> +		  e.gr_name, e.gr_passwd);
> +	  ++errors;
> +	}
> +    }
> +  else
> +    {
> +      /* Expect success.  */
> +      size_t expected_length = strlen (expected);
> +      if (ret == 0)
> +	{
> +	  long written = ftell (f);
> +
> +	  if (written <= 0 || fflush (f) < 0)
> +	    {
> +	      printf ("stream error: %m\n");
> +	      ++errors;
> +	    }
> +	  else if (buf[written - 1] != '\n')
> +	    {
> +	      printf ("FAILED: \"%s\" without newline\n", expected);
> +	      ++errors;
> +	    }
> +	  else if (strncmp (buf, expected, written - 1) != 0
> +		   || written - 1 != expected_length)
> +	    {
> +	      buf[written - 1] = '\0';
> +	      printf ("FAILED: \"%s\" (%ld), expected \"%s\" (%zu)\n",
> +		      buf, written - 1, expected, expected_length);
> +	      ++errors;
> +	    }
> +	}
> +      else
> +	{
> +	  printf ("FAILED: putgrent (expected \"%s\"): %m\n", expected);
> +	  ++errors;
> +	}
> +    }
> +
> +  fclose (f);
> +  free (buf);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  check ((struct group) {
> +      .gr_name = (char *) "root",
> +    },
> +    "root::0:");
> +  check ((struct group) {
> +      .gr_name = (char *) "root",
> +      .gr_passwd = (char *) "password",
> +      .gr_gid = 1234,
> +      .gr_mem = (char *[2]) {(char *) "member1", NULL}
> +    },
> +    "root:password:1234:member1");
> +  check ((struct group) {
> +      .gr_name = (char *) "root",
> +      .gr_passwd = (char *) "password",
> +      .gr_gid = 1234,
> +      .gr_mem = (char *[3]) {(char *) "member1", (char *) "member2", NULL}
> +    },
> +    "root:password:1234:member1,member2");
> +
> +  /* Bad values.  */
> +  {
> +    static const char *const bad_strings[] = {
> +      ":",
> +      "\n",
> +      ":bad",
> +      "\nbad",
> +      "b:ad",
> +      "b\nad",
> +      "bad:",
> +      "bad\n",
> +      "b:a\nd"
> +      ",",
> +      "\n,",
> +      ":,",
> +      ",bad",
> +      "b,ad",
> +      "bad,",
> +      NULL
> +    };
> +    for (const char *const *bad = bad_strings; *bad != NULL; ++bad)
> +      {
> +	char *members[]
> +	  = {(char *) "first", (char *) *bad, (char *) "last", NULL};
> +	if (strpbrk (*bad, ":\n") != NULL)
> +	  {
> +	    check ((struct group) {
> +		.gr_name = (char *) *bad,
> +	      }, NULL);
> +	    check ((struct group) {
> +		.gr_name = (char *) "root",
> +		.gr_passwd = (char *) *bad,
> +	      }, NULL);
> +	  }
> +	check ((struct group) {
> +	    .gr_name = (char *) "root",
> +	    .gr_passwd = (char *) "password",
> +	    .gr_mem = members,
> +	  }, NULL);
> +      }
> +  }
> +
> +  return errors > 0;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/gshadow/Makefile b/gshadow/Makefile
> index 883e1c8..c811041 100644
> --- a/gshadow/Makefile
> +++ b/gshadow/Makefile
> @@ -26,7 +26,7 @@ headers		= gshadow.h
>  routines	= getsgent getsgnam sgetsgent fgetsgent putsgent \
>  		  getsgent_r getsgnam_r sgetsgent_r fgetsgent_r
>  
> -tests = tst-gshadow
> +tests = tst-gshadow tst-putsgent
>  
>  CFLAGS-getsgent_r.c = -fexceptions
>  CFLAGS-getsgent.c = -fexceptions
> diff --git a/gshadow/putsgent.c b/gshadow/putsgent.c
> index 0b2cad6..c1cb921 100644
> --- a/gshadow/putsgent.c
> +++ b/gshadow/putsgent.c
> @@ -15,9 +15,11 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> +#include <errno.h>
>  #include <stdbool.h>
>  #include <stdio.h>
>  #include <gshadow.h>
> +#include <nss.h>
>  
>  #define _S(x)	x ? x : ""
>  
> @@ -29,6 +31,15 @@ putsgent (const struct sgrp *g, FILE *stream)
>  {
>    int errors = 0;
>  
> +  if (g->sg_namp == NULL || !__nss_valid_field (g->sg_namp)
> +      || !__nss_valid_field (g->sg_passwd)
> +      || !__nss_valid_list_field (g->sg_adm)
> +      || !__nss_valid_list_field (g->sg_mem))
> +    {
> +      __set_errno (EINVAL);
> +      return -1;
> +    }

OK.

> +
>    _IO_flockfile (stream);
>  
>    if (fprintf (stream, "%s:%s:", g->sg_namp, _S (g->sg_passwd)) < 0)
> diff --git a/gshadow/tst-putsgent.c b/gshadow/tst-putsgent.c
> new file mode 100644
> index 0000000..f628fe6
> --- /dev/null
> +++ b/gshadow/tst-putsgent.c
> @@ -0,0 +1,166 @@

Needs first line description of test with reference BZ #.

> +/* Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <gshadow.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdlib.h>
> +
> +static int errors;
> +
> +static void
> +check (struct sgrp e, const char *expected)
> +{
> +  char *buf;
> +  size_t buf_size;
> +  FILE *f = open_memstream (&buf, &buf_size);
> +
> +  if (f == NULL)
> +    {
> +      printf ("open_memstream: %m\n");
> +      ++errors;
> +      return;
> +    }
> +
> +  int ret = putsgent (&e, f);
> +
> +  if (expected == NULL)
> +    {
> +      if (ret == -1)
> +	{
> +	  if (errno != EINVAL)
> +	    {
> +	      printf ("putsgent: unexpected error code: %m\n");
> +	      ++errors;
> +	    }
> +	}
> +      else
> +	{
> +	  printf ("putsgent: unexpected success (\"%s\")\n", e.sg_namp);
> +	  ++errors;
> +	}
> +    }
> +  else
> +    {
> +      /* Expect success.  */
> +      size_t expected_length = strlen (expected);
> +      if (ret == 0)
> +	{
> +	  long written = ftell (f);
> +
> +	  if (written <= 0 || fflush (f) < 0)
> +	    {
> +	      printf ("stream error: %m\n");
> +	      ++errors;
> +	    }
> +	  else if (buf[written - 1] != '\n')
> +	    {
> +	      printf ("FAILED: \"%s\" without newline\n", expected);
> +	      ++errors;
> +	    }
> +	  else if (strncmp (buf, expected, written - 1) != 0
> +		   || written - 1 != expected_length)
> +	    {
> +	      printf ("FAILED: \"%s\" (%ld), expected \"%s\" (%zu)\n",
> +		      buf, written - 1, expected, expected_length);
> +	      ++errors;
> +	    }
> +	}
> +      else
> +	{
> +	  printf ("FAILED: putsgent (expected \"%s\"): %m\n", expected);
> +	  ++errors;
> +	}
> +    }
> +
> +  fclose (f);
> +  free (buf);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  check ((struct sgrp) {
> +      .sg_namp = (char *) "root",
> +    },
> +    "root:::");
> +  check ((struct sgrp) {
> +      .sg_namp = (char *) "root",
> +      .sg_passwd = (char *) "password",
> +    },
> +    "root:password::");
> +  check ((struct sgrp) {
> +      .sg_namp = (char *) "root",
> +      .sg_passwd = (char *) "password",
> +      .sg_adm = (char *[]) {(char *) "adm1", (char *) "adm2", NULL},
> +      .sg_mem = (char *[]) {(char *) "mem1", (char *) "mem2", NULL},
> +    },
> +    "root:password:adm1,adm2:mem1,mem2");
> +
> +  /* Bad values.  */
> +  {
> +    static const char *const bad_strings[] = {
> +      ":",
> +      "\n",
> +      ":bad",
> +      "\nbad",
> +      "b:ad",
> +      "b\nad",
> +      "bad:",
> +      "bad\n",
> +      "b:a\nd",
> +      ",",
> +      "\n,",
> +      ":,",
> +      ",bad",
> +      "b,ad",
> +      "bad,",
> +      NULL
> +    };
> +    for (const char *const *bad = bad_strings; *bad != NULL; ++bad)
> +      {
> +	char *members[]
> +	  = {(char *) "first", (char *) *bad, (char *) "last", NULL};
> +	if (strpbrk (*bad, ":\n") != NULL)
> +	  {
> +	    check ((struct sgrp) {
> +		.sg_namp = (char *) *bad,
> +	      }, NULL);
> +	    check ((struct sgrp) {
> +		.sg_namp = (char *) "root",
> +		.sg_passwd = (char *) *bad,
> +	      }, NULL);
> +	  }
> +	check ((struct sgrp) {
> +	    .sg_namp = (char *) "root",
> +	    .sg_passwd = (char *) "password",
> +	    .sg_adm = members
> +	  }, NULL);
> +	check ((struct sgrp) {
> +	    .sg_namp = (char *) "root",
> +	    .sg_passwd = (char *) "password",
> +	    .sg_mem = members
> +	  }, NULL);
> +      }
> +  }
> +
> +  return errors > 0;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/include/nss.h b/include/nss.h
> index 0541335..1e8cc39 100644
> --- a/include/nss.h
> +++ b/include/nss.h
> @@ -1 +1,14 @@
> +#ifndef _NSS_H
>  #include <nss/nss.h>
> +
> +#define NSS_INVALID_FIELD_CHARACTERS ":\n"
> +extern const char __nss_invalid_field_characters[] attribute_hidden;
> +
> +_Bool __nss_valid_field (const char *value)
> +  attribute_hidden internal_function;
> +_Bool __nss_valid_list_field (char **list)
> +  attribute_hidden internal_function;
> +const char *__nss_rewrite_field (const char *value, char **to_be_freed)
> +  attribute_hidden internal_function;

OK.


> +
> +#endif /* _NSS_H */
> diff --git a/include/pwd.h b/include/pwd.h
> index bd7fecc..3b0f725 100644
> --- a/include/pwd.h
> +++ b/include/pwd.h
> @@ -24,7 +24,7 @@ extern int __fgetpwent_r (FILE * __stream, struct passwd *__resultbuf,
>  			  char *__buffer, size_t __buflen,
>  			  struct passwd **__result);
>  
> -#include <nss/nss.h>
> +#include <nss.h>

OK.

>  
>  struct parser_data;
>  extern int _nss_files_parse_pwent (char *line, struct passwd *result,
> diff --git a/nss/Makefile b/nss/Makefile
> index 65ab7b5..9da8b15 100644
> --- a/nss/Makefile
> +++ b/nss/Makefile
> @@ -26,6 +26,7 @@ headers			:= nss.h
>  
>  # This is the trivial part which goes into libc itself.
>  routines		= nsswitch getnssent getnssent_r digits_dots \
> +			  valid_field valid_list_field rewrite_field \

OK.

>  			  $(addsuffix -lookup,$(databases))
>  
>  # These are the databases that go through nss dispatch.
> @@ -47,7 +48,9 @@ install-bin             := getent makedb
>  makedb-modules = xmalloc hash-string
>  extra-objs		+= $(makedb-modules:=.o)
>  
> -tests			= test-netdb tst-nss-test1 test-digits-dots tst-nss-getpwent
> +tests-static            = tst-field
> +tests			= test-netdb tst-nss-test1 test-digits-dots tst-nss-getpwent \
> +			  $(tests-static)

OK.

>  xtests			= bug-erange
>  
>  # Specify rules for the nss_* modules.  We have some services.
> @@ -82,8 +85,7 @@ libnss_db-inhibit-o	= $(filter-out .os,$(object-suffixes))
>  ifeq ($(build-static-nss),yes)
>  routines                += $(libnss_files-routines)
>  static-only-routines    += $(libnss_files-routines)
> -tests-static		= tst-nss-static
> -tests			+= $(tests-static)
> +tests-static		+= tst-nss-static

OK.

>  endif
>  
>  include ../Rules
> diff --git a/nss/getent.c b/nss/getent.c
> index 34df848..996d378 100644
> --- a/nss/getent.c
> +++ b/nss/getent.c
> @@ -184,20 +184,8 @@ ethers_keys (int number, char *key[])
>  static void
>  print_group (struct group *grp)
>  {
> -  unsigned int i = 0;
> -
> -  printf ("%s:%s:%lu:", grp->gr_name ? grp->gr_name : "",
> -	  grp->gr_passwd ? grp->gr_passwd : "",
> -	  (unsigned long int) grp->gr_gid);
> -
> -  while (grp->gr_mem[i] != NULL)
> -    {
> -      fputs_unlocked (grp->gr_mem[i], stdout);
> -      ++i;
> -      if (grp->gr_mem[i] != NULL)
> -	putchar_unlocked (',');
> -    }
> -  putchar_unlocked ('\n');
> +  if (putgrent (grp, stdout) != 0)
> +    fprintf (stderr, "error writing group entry: %m\n");

OK.

>  }
>  
>  static int
> @@ -241,32 +229,8 @@ group_keys (int number, char *key[])
>  static void
>  print_gshadow (struct sgrp *sg)
>  {
> -  unsigned int i = 0;
> -
> -  printf ("%s:%s:",
> -	  sg->sg_namp ? sg->sg_namp : "",
> -	  sg->sg_passwd ? sg->sg_passwd : "");
> -
> -  while (sg->sg_adm[i] != NULL)
> -    {
> -      fputs_unlocked (sg->sg_adm[i], stdout);
> -      ++i;
> -      if (sg->sg_adm[i] != NULL)
> -	putchar_unlocked (',');
> -    }
> -
> -  putchar_unlocked (':');
> -
> -  i = 0;
> -  while (sg->sg_mem[i] != NULL)
> -    {
> -      fputs_unlocked (sg->sg_mem[i], stdout);
> -      ++i;
> -      if (sg->sg_mem[i] != NULL)
> -	putchar_unlocked (',');
> -    }
> -
> -  putchar_unlocked ('\n');
> +  if (putsgent (sg, stdout) != 0)
> +    fprintf (stderr, "error writing gshadow entry: %m\n");

OK.

>  }
>  
>  static int
> @@ -603,14 +567,8 @@ networks_keys (int number, char *key[])
>  static void
>  print_passwd (struct passwd *pwd)
>  {
> -  printf ("%s:%s:%lu:%lu:%s:%s:%s\n",
> -	  pwd->pw_name ? pwd->pw_name : "",
> -	  pwd->pw_passwd ? pwd->pw_passwd : "",
> -	  (unsigned long int) pwd->pw_uid,
> -	  (unsigned long int) pwd->pw_gid,
> -	  pwd->pw_gecos ? pwd->pw_gecos : "",
> -	  pwd->pw_dir ? pwd->pw_dir : "",
> -	  pwd->pw_shell ? pwd->pw_shell : "");
> +  if (putpwent (pwd, stdout) != 0)
> +    fprintf (stderr, "error writing passwd entry: %m\n");

OK.

>  }
>  
>  static int
> @@ -812,26 +770,8 @@ services_keys (int number, char *key[])
>  static void
>  print_shadow (struct spwd *sp)
>  {
> -  printf ("%s:%s:",
> -	  sp->sp_namp ? sp->sp_namp : "",
> -	  sp->sp_pwdp ? sp->sp_pwdp : "");
> -
> -#define SHADOW_FIELD(n) \
> -  if (sp->n == -1)							      \
> -    putchar_unlocked (':');						      \
> -  else									      \
> -    printf ("%ld:", sp->n)
> -
> -  SHADOW_FIELD (sp_lstchg);
> -  SHADOW_FIELD (sp_min);
> -  SHADOW_FIELD (sp_max);
> -  SHADOW_FIELD (sp_warn);
> -  SHADOW_FIELD (sp_inact);
> -  SHADOW_FIELD (sp_expire);
> -  if (sp->sp_flag == ~0ul)
> -    putchar_unlocked ('\n');
> -  else
> -    printf ("%lu\n", sp->sp_flag);
> +  if (putspent (sp, stdout) != 0)
> +    fprintf (stderr, "error writing shadow entry: %m\n");

OK.

>  }
>  
>  static int
> diff --git a/nss/rewrite_field.c b/nss/rewrite_field.c
> new file mode 100644
> index 0000000..fb9d274
> --- /dev/null
> +++ b/nss/rewrite_field.c
> @@ -0,0 +1,51 @@
> +/* Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <nss.h>
> +#include <string.h>
> +
> +/* Rewrite VALUE to a valid field value in the NSS database.  Invalid
> +   characters are replaced with a single space character ' '.  If
> +   VALUE is NULL, the empty string is returned.  *TO_BE_FREED is
> +   overwritten with a pointer the caller has to free if the function
> +   returns successfully.  On failure, return NULL.  */
> +const char *
> +__nss_rewrite_field (const char *value, char **to_be_freed)
> +{
> +  *to_be_freed = NULL;
> +  if (value == NULL)
> +    return "";
> +
> +  /* Search for non-allowed characters.  */
> +  const char *p = strpbrk (value, __nss_invalid_field_characters);
> +  if (p == NULL)
> +    return value;
> +  *to_be_freed = __strdup (value);
> +  if (*to_be_freed == NULL)
> +    return NULL;
> +
> +  /* Switch pointer to freshly-allocated buffer.  */
> +  char *bad = *to_be_freed + (p - value);
> +  do
> +    {
> +      *bad = ' ';
> +      bad = strpbrk (bad + 1, __nss_invalid_field_characters);
> +    }
> +  while (bad != NULL);
> +
> +  return *to_be_freed;

OK.

> +}
> diff --git a/nss/tst-field.c b/nss/tst-field.c
> new file mode 100644
> index 0000000..4aab29c
> --- /dev/null
> +++ b/nss/tst-field.c
> @@ -0,0 +1,100 @@

Needs first line description of test with reference BZ #.

> +/* Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* This test needs to be statically linked because it access hidden
> +   functions.  */
> +
> +#include <nss.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +static int errors;
> +
> +static void
> +check (const char *what, _Bool expr)
> +{
> +  if (!expr)
> +    {
> +      printf ("FAIL: %s\n", what);
> +      ++errors;
> +    }
> +}
> +
> +#define CHECK(expr) check (#expr, (expr))
> +
> +static void
> +check_rewrite (const char *input, const char *expected)
> +{
> +  char *to_free;
> +  const char *result = __nss_rewrite_field (input, &to_free);
> +  CHECK (result != NULL);
> +  if (result != NULL && strcmp (result, expected) != 0)
> +    {
> +      printf ("FAIL: rewrite \"%s\" -> \"%s\", expected \"%s\"\n",
> +	      input, result, expected);
> +      ++errors;
> +    }
> +  free (to_free);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  CHECK (__nss_valid_field (NULL));
> +  CHECK (__nss_valid_field (""));
> +  CHECK (__nss_valid_field ("+"));
> +  CHECK (__nss_valid_field ("-"));
> +  CHECK (__nss_valid_field (" "));
> +  CHECK (__nss_valid_field ("abcdef"));
> +  CHECK (__nss_valid_field ("abc def"));
> +  CHECK (__nss_valid_field ("abc\tdef"));
> +  CHECK (!__nss_valid_field ("abcdef:"));
> +  CHECK (!__nss_valid_field ("abcde:f"));
> +  CHECK (!__nss_valid_field (":abcdef"));
> +  CHECK (!__nss_valid_field ("abcdef\n"));
> +  CHECK (!__nss_valid_field ("\nabcdef"));
> +  CHECK (!__nss_valid_field (":"));
> +  CHECK (!__nss_valid_field ("\n"));
> +
> +  CHECK (__nss_valid_list_field (NULL));
> +  CHECK (__nss_valid_list_field ((char *[]) {(char *) "good", NULL}));
> +  CHECK (!__nss_valid_list_field ((char *[]) {(char *) "g,ood", NULL}));
> +  CHECK (!__nss_valid_list_field ((char *[]) {(char *) "g\nood", NULL}));
> +  CHECK (!__nss_valid_list_field ((char *[]) {(char *) "g:ood", NULL}));
> +
> +  check_rewrite (NULL, "");
> +  check_rewrite ("", "");
> +  check_rewrite ("abc", "abc");
> +  check_rewrite ("abc\n", "abc ");
> +  check_rewrite ("abc:", "abc ");
> +  check_rewrite ("\nabc", " abc");
> +  check_rewrite (":abc", " abc");
> +  check_rewrite (":", " ");
> +  check_rewrite ("\n", " ");
> +  check_rewrite ("a:b:c", "a b c");
> +  check_rewrite ("a\nb\nc", "a b c");
> +  check_rewrite ("a\nb:c", "a b c");
> +  check_rewrite ("aa\nbb\ncc", "aa bb cc");
> +  check_rewrite ("aa\nbb:cc", "aa bb cc");
> +
> +  return errors > 0;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> +
> diff --git a/nss/valid_field.c b/nss/valid_field.c
> new file mode 100644
> index 0000000..5fcddc5
> --- /dev/null
> +++ b/nss/valid_field.c
> @@ -0,0 +1,31 @@
> +/* Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <nss.h>
> +#include <string.h>
> +
> +const char __nss_invalid_field_characters[] = NSS_INVALID_FIELD_CHARACTERS;
> +
> +/* Check that VALUE is either NULL or a NUL-terminated string which
> +   does not contain characters not permitted in NSS database
> +   fields.  */
> +_Bool
> +__nss_valid_field (const char *value)
> +{
> +  return value == NULL
> +    || strpbrk (value, __nss_invalid_field_characters) == NULL;
> +}

OK.

> diff --git a/nss/valid_list_field.c b/nss/valid_list_field.c
> new file mode 100644
> index 0000000..98ab93b
> --- /dev/null
> +++ b/nss/valid_list_field.c
> @@ -0,0 +1,35 @@
> +/* Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <nss.h>
> +#include <stdbool.h>
> +#include <string.h>
> +
> +static const char invalid_characters[] = NSS_INVALID_FIELD_CHARACTERS ",";
> +
> +/* Check that all list members match the field syntax requirements and
> +   do not contain the character ','.  */
> +_Bool
> +__nss_valid_list_field (char **list)
> +{
> +  if (list == NULL)
> +    return true;
> +  for (; *list != NULL; ++list)
> +    if (strpbrk (*list, invalid_characters) != NULL)
> +      return false;
> +  return true;
> +}

OK.

> diff --git a/pwd/Makefile b/pwd/Makefile
> index 7f6de03..af09734 100644
> --- a/pwd/Makefile
> +++ b/pwd/Makefile
> @@ -28,7 +28,7 @@ routines := fgetpwent getpw putpwent \
>  	    getpwent getpwnam getpwuid \
>  	    getpwent_r getpwnam_r getpwuid_r fgetpwent_r
>  
> -tests := tst-getpw
> +tests := tst-getpw tst-putpwent
>  
>  include ../Rules
>  
> diff --git a/pwd/putpwent.c b/pwd/putpwent.c
> index 8be58c2..16b9c6f 100644
> --- a/pwd/putpwent.c
> +++ b/pwd/putpwent.c
> @@ -18,37 +18,47 @@
>  #include <errno.h>
>  #include <stdio.h>
>  #include <pwd.h>
> +#include <nss.h>
>  
>  #define _S(x)	x ?: ""
>  
> -/* Write an entry to the given stream.
> -   This must know the format of the password file.  */
> +/* Write an entry to the given stream.  This must know the format of
> +   the password file.  If the input contains invalid characters,
> +   return EINVAL, or replace them with spaces (if they are contained
> +   in the GECOS field).  */

OK.

>  int
> -putpwent (p, stream)
> -     const struct passwd *p;
> -     FILE *stream;
> +putpwent (const struct passwd *p, FILE *stream)
>  {
> -  if (p == NULL || stream == NULL)
> +  if (p == NULL || stream == NULL
> +      || p->pw_name == NULL || !__nss_valid_field (p->pw_name)
> +      || !__nss_valid_field (p->pw_passwd)
> +      || !__nss_valid_field (p->pw_dir)
> +      || !__nss_valid_field (p->pw_shell))

OK.

>      {
>        __set_errno (EINVAL);
>        return -1;
>      }
>  
> +  int ret;
> +  char *gecos_alloc;
> +  const char *gecos = __nss_rewrite_field (p->pw_gecos, &gecos_alloc);
> +
> +  if (gecos == NULL)
> +    return -1;
> +
>    if (p->pw_name[0] == '+' || p->pw_name[0] == '-')
> -    {
> -      if (fprintf (stream, "%s:%s:::%s:%s:%s\n",
> -		   p->pw_name, _S (p->pw_passwd),
> -		   _S (p->pw_gecos), _S (p->pw_dir), _S (p->pw_shell)) < 0)
> -	return -1;
> -    }
> +      ret = fprintf (stream, "%s:%s:::%s:%s:%s\n",
> +		     p->pw_name, _S (p->pw_passwd),
> +		     gecos, _S (p->pw_dir), _S (p->pw_shell));
>    else
> -    {
> -      if (fprintf (stream, "%s:%s:%lu:%lu:%s:%s:%s\n",
> -		   p->pw_name, _S (p->pw_passwd),
> -		   (unsigned long int) p->pw_uid,
> -		   (unsigned long int) p->pw_gid,
> -		   _S (p->pw_gecos), _S (p->pw_dir), _S (p->pw_shell)) < 0)
> -	return -1;
> -    }
> -  return 0;
> +      ret = fprintf (stream, "%s:%s:%lu:%lu:%s:%s:%s\n",
> +		     p->pw_name, _S (p->pw_passwd),
> +		     (unsigned long int) p->pw_uid,
> +		     (unsigned long int) p->pw_gid,
> +		     gecos, _S (p->pw_dir), _S (p->pw_shell));
> +
> +  free (gecos_alloc);
> +  if (ret >= 0)
> +    ret = 0;
> +  return ret;
>  }

OK.

> diff --git a/pwd/pwd.h b/pwd/pwd.h
> index fcfb2ab..70a051d 100644
> --- a/pwd/pwd.h
> +++ b/pwd/pwd.h
> @@ -100,7 +100,7 @@ extern struct passwd *fgetpwent (FILE *__stream) __nonnull ((1));
>     or due to the implementation it is a cancellation point and
>     therefore not marked with __THROW.  */
>  extern int putpwent (const struct passwd *__restrict __p,
> -		     FILE *__restrict __f) __nonnull ((1, 2));
> +		     FILE *__restrict __f);
>  #endif
>  
>  /* Search for an entry with a matching user ID.
> diff --git a/pwd/tst-putpwent.c b/pwd/tst-putpwent.c
> new file mode 100644
> index 0000000..cff239c
> --- /dev/null
> +++ b/pwd/tst-putpwent.c
> @@ -0,0 +1,166 @@

Needs first line description of test with reference BZ #.

> +/* Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <pwd.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdlib.h>
> +
> +static int errors;
> +
> +static void
> +check (struct passwd p, const char *expected)
> +{
> +  char *buf;
> +  size_t buf_size;
> +  FILE *f = open_memstream (&buf, &buf_size);
> +
> +  if (f == NULL)
> +    {
> +      printf ("open_memstream: %m\n");
> +      ++errors;
> +      return;
> +    }
> +
> +  int ret = putpwent (&p, f);
> +
> +  if (expected == NULL)
> +    {
> +      if (ret == -1)
> +	{
> +	  if (errno != EINVAL)
> +	    {
> +	      printf ("putpwent: unexpected error code: %m\n");
> +	      ++errors;
> +	    }
> +	}
> +      else
> +	{
> +	  printf ("putpwent: unexpected success (\"%s\")\n", p.pw_name);
> +	  ++errors;
> +	}
> +    }
> +  else
> +    {
> +      /* Expect success.  */
> +      size_t expected_length = strlen (expected);
> +      if (ret == 0)
> +	{
> +	  long written = ftell (f);
> +
> +	  if (written <= 0 || fflush (f) < 0)
> +	    {
> +	      printf ("stream error: %m\n");
> +	      ++errors;
> +	    }
> +	  else if (buf[written - 1] != '\n')
> +	    {
> +	      printf ("FAILED: \"%s\" without newline\n", expected);
> +	      ++errors;
> +	    }
> +	  else if (strncmp (buf, expected, written - 1) != 0
> +		   || written - 1 != expected_length)
> +	    {
> +	      printf ("FAILED: \"%s\" (%ld), expected \"%s\" (%zu)\n",
> +		      buf, written - 1, expected, expected_length);
> +	      ++errors;
> +	    }
> +	}
> +      else
> +	{
> +	  printf ("FAILED: putpwent (expected \"%s\"): %m\n", expected);
> +	  ++errors;
> +	}
> +    }
> +
> +  fclose (f);
> +  free (buf);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  check ((struct passwd) {
> +      .pw_name = (char *) "root",
> +    },
> +    "root::0:0:::");
> +  check ((struct passwd) {
> +      .pw_name = (char *) "root",
> +      .pw_passwd = (char *) "password",
> +    },
> +    "root:password:0:0:::");
> +  check ((struct passwd) {
> +      .pw_name = (char *) "root",
> +      .pw_passwd = (char *) "password",
> +      .pw_uid = 12,
> +      .pw_gid = 34,
> +      .pw_gecos = (char *) "gecos",
> +      .pw_dir = (char *) "home",
> +      .pw_shell = (char *) "shell",
> +    },
> +    "root:password:12:34:gecos:home:shell");
> +  check ((struct passwd) {
> +      .pw_name = (char *) "root",
> +      .pw_passwd = (char *) "password",
> +      .pw_uid = 12,
> +      .pw_gid = 34,
> +      .pw_gecos = (char *) ":ge\n:cos\n",
> +      .pw_dir = (char *) "home",
> +      .pw_shell = (char *) "shell",
> +    },
> +    "root:password:12:34: ge  cos :home:shell");
> +
> +  /* Bad values.  */
> +  {
> +    static const char *const bad_strings[] = {
> +      ":",
> +      "\n",
> +      ":bad",
> +      "\nbad",
> +      "b:ad",
> +      "b\nad",
> +      "bad:",
> +      "bad\n",
> +      "b:a\nd",
> +      NULL
> +    };
> +    for (const char *const *bad = bad_strings; *bad != NULL; ++bad)
> +      {
> +	check ((struct passwd) {
> +	    .pw_name = (char *) *bad,
> +	  }, NULL);
> +	check ((struct passwd) {
> +	    .pw_name = (char *) "root",
> +	    .pw_passwd = (char *) *bad,
> +	  }, NULL);
> +	check ((struct passwd) {
> +	    .pw_name = (char *) "root",
> +	    .pw_dir = (char *) *bad,
> +	  }, NULL);
> +	check ((struct passwd) {
> +	    .pw_name = (char *) "root",
> +	    .pw_shell = (char *) *bad,
> +	  }, NULL);
> +      }
> +  }
> +
> +  return errors > 0;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/shadow/Makefile b/shadow/Makefile
> index 8291c61..6990f33 100644
> --- a/shadow/Makefile
> +++ b/shadow/Makefile
> @@ -27,7 +27,7 @@ routines	= getspent getspnam sgetspent fgetspent putspent \
>  		  getspent_r getspnam_r sgetspent_r fgetspent_r \
>  		  lckpwdf
>  
> -tests = tst-shadow
> +tests = tst-shadow tst-putspent
>  
>  CFLAGS-getspent_r.c = -fexceptions
>  CFLAGS-getspent.c = -fexceptions
> diff --git a/shadow/putspent.c b/shadow/putspent.c
> index 142e697..ba8230a 100644
> --- a/shadow/putspent.c
> +++ b/shadow/putspent.c
> @@ -15,6 +15,8 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> +#include <errno.h>
> +#include <nss.h>
>  #include <stdio.h>
>  #include <shadow.h>
>  
> @@ -31,6 +33,13 @@ putspent (const struct spwd *p, FILE *stream)
>  {
>    int errors = 0;
>  
> +  if (p->sp_namp == NULL || !__nss_valid_field (p->sp_namp)
> +      || !__nss_valid_field (p->sp_pwdp))
> +    {
> +      __set_errno (EINVAL);
> +      return -1;
> +    }
> +

OK.

>    flockfile (stream);
>  
>    if (fprintf (stream, "%s:%s:", p->sp_namp, _S (p->sp_pwdp)) < 0)
> diff --git a/shadow/tst-putspent.c b/shadow/tst-putspent.c
> new file mode 100644
> index 0000000..4554ee4
> --- /dev/null
> +++ b/shadow/tst-putspent.c
> @@ -0,0 +1,162 @@

Needs first line description of test with reference BZ #.

> +/* Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <shadow.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdlib.h>
> +
> +static int errors;
> +
> +static void
> +check (struct spwd p, const char *expected)
> +{
> +  char *buf;
> +  size_t buf_size;
> +  FILE *f = open_memstream (&buf, &buf_size);
> +
> +  if (f == NULL)
> +    {
> +      printf ("open_memstream: %m\n");
> +      ++errors;
> +      return;
> +    }
> +
> +  int ret = putspent (&p, f);
> +
> +  if (expected == NULL)
> +    {
> +      if (ret == -1)
> +	{
> +	  if (errno != EINVAL)
> +	    {
> +	      printf ("putspent: unexpected error code: %m\n");
> +	      ++errors;
> +	    }
> +	}
> +      else
> +	{
> +	  printf ("putspent: unexpected success (\"%s\")\n", p.sp_namp);
> +	  ++errors;
> +	}
> +    }
> +  else
> +    {
> +      /* Expect success.  */
> +      size_t expected_length = strlen (expected);
> +      if (ret == 0)
> +	{
> +	  long written = ftell (f);
> +
> +	  if (written <= 0 || fflush (f) < 0)
> +	    {
> +	      printf ("stream error: %m\n");
> +	      ++errors;
> +	    }
> +	  else if (buf[written - 1] != '\n')
> +	    {
> +	      printf ("FAILED: \"%s\" without newline\n", expected);
> +	      ++errors;
> +	    }
> +	  else if (strncmp (buf, expected, written - 1) != 0
> +		   || written - 1 != expected_length)
> +	    {
> +	      printf ("FAILED: \"%s\" (%ld), expected \"%s\" (%zu)\n",
> +		      buf, written - 1, expected, expected_length);
> +	      ++errors;
> +	    }
> +	}
> +      else
> +	{
> +	  printf ("FAILED: putspent (expected \"%s\"): %m\n", expected);
> +	  ++errors;
> +	}
> +    }
> +
> +  fclose (f);
> +  free (buf);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  check ((struct spwd) {
> +      .sp_namp = (char *) "root",
> +    },
> +    "root::0:0:0:0:0:0:0");
> +  check ((struct spwd) {
> +      .sp_namp = (char *) "root",
> +      .sp_pwdp = (char *) "password",
> +    },
> +    "root:password:0:0:0:0:0:0:0");
> +  check ((struct spwd) {
> +      .sp_namp = (char *) "root",
> +      .sp_pwdp = (char *) "password",
> +      .sp_lstchg = -1,
> +      .sp_min = -1,
> +      .sp_max = -1,
> +      .sp_warn = -1,
> +      .sp_inact = -1,
> +      .sp_expire = -1,
> +      .sp_flag = -1
> +    },
> +    "root:password:::::::");
> +  check ((struct spwd) {
> +      .sp_namp = (char *) "root",
> +      .sp_pwdp = (char *) "password",
> +      .sp_lstchg = 1,
> +      .sp_min = 2,
> +      .sp_max = 3,
> +      .sp_warn = 4,
> +      .sp_inact = 5,
> +      .sp_expire = 6,
> +      .sp_flag = 7
> +    },
> +    "root:password:1:2:3:4:5:6:7");
> +
> +  /* Bad values.  */
> +  {
> +    static const char *const bad_strings[] = {
> +      ":",
> +      "\n",
> +      ":bad",
> +      "\nbad",
> +      "b:ad",
> +      "b\nad",
> +      "bad:",
> +      "bad\n",
> +      "b:a\nd",
> +      NULL
> +    };
> +    for (const char *const *bad = bad_strings; *bad != NULL; ++bad)
> +      {
> +	check ((struct spwd) {
> +	    .sp_namp = (char *) *bad,
> +	  }, NULL);
> +	check ((struct spwd) {
> +	    .sp_namp = (char *) "root",
> +	    .sp_pwdp = (char *) *bad,
> +	  }, NULL);
> +      }
> +  }
> +
> +  return errors > 0;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> -- 2.4.3

On 27 Jul 2015 17:18, Florian Weimer wrote:
> -      int i;
> -
> -      for (i = 0 ; gr->gr_mem[i] != NULL; i++)
> +      for (size_t i = 0 ; gr->gr_mem[i] != NULL; i++)

if you're tweaking style(ish), should trim the space before the first ; too


> --- /dev/null
> +++ b/grp/tst-putgrent.c
>
> +      ++errors;
> ...
> +  return errors > 0;

is an error count really necessary ?  just make it a bool.
<paranoid>don't want it to overflow</paranoid>

> +check (const char *what, _Bool expr)

why not "bool" ?
-mike

On 07/28/2015 05:19 AM, Mike Frysinger wrote:
> On 27 Jul 2015 17:18, Florian Weimer wrote:
>> -      int i;
>> -
>> -      for (i = 0 ; gr->gr_mem[i] != NULL; i++)
>> +      for (size_t i = 0 ; gr->gr_mem[i] != NULL; i++)
> 
> if you're tweaking style(ish), should trim the space before the first ; too

Fixed.

Carlos, I also added the BZ# references to the test cases.

>> --- /dev/null
>> +++ b/grp/tst-putgrent.c
>>
>> +      ++errors;
>> ...
>> +  return errors > 0;
> 
> is an error count really necessary ?  just make it a bool.

“++errors;” is clearer to me than “errors |= true;”.  Other test suite
code uses the counter approach, too.

> <paranoid>don't want it to overflow</paranoid>

Can't really happen here. :)

>> +check (const char *what, _Bool expr)
> 
> why not "bool" ?

It requires #include <stdbool.h>.

On 28 Jul 2015 14:30, Florian Weimer wrote:
> On 07/28/2015 05:19 AM, Mike Frysinger wrote:
> > On 27 Jul 2015 17:18, Florian Weimer wrote:
> >> --- /dev/null
> >> +++ b/grp/tst-putgrent.c
> >>
> >> +      ++errors;
> >> ...
> >> +  return errors > 0;
> > 
> > is an error count really necessary ?  just make it a bool.
> 
> “++errors;” is clearer to me than “errors |= true;”.  Other test suite
> code uses the counter approach, too.

i meant:
	error = true;
or:
	error = 1;

there's no point in using |= operations here

> >> +check (const char *what, _Bool expr)
> > 
> > why not "bool" ?
> 
> It requires #include <stdbool.h>.

so ?  include it then.  better imo than using private _Bool types.
-mike

Mike Frysinger <vapier@gentoo.org> writes:

> On 28 Jul 2015 14:30, Florian Weimer wrote:
>> On 07/28/2015 05:19 AM, Mike Frysinger wrote:
>> > On 27 Jul 2015 17:18, Florian Weimer wrote:
>> >> --- /dev/null
>> >> +++ b/grp/tst-putgrent.c
>> >>
>> >> +      ++errors;
>> >> ...
>> >> +  return errors > 0;
>> > 
>> > is an error count really necessary ?  just make it a bool.
>> 
>> “++errors;” is clearer to me than “errors |= true;”.  Other test suite
>> code uses the counter approach, too.
>
> i meant:
> 	error = true;
> or:
> 	error = 1;
>
> there's no point in using |= operations here

FWIW, ++errors is doing TRT for bool (equivalent to errors = true).

Andreas.

On 07/27/2015 09:59 PM, Carlos O'Donell wrote:
> On 07/27/2015 11:18 AM, Florian Weimer wrote:
>> This patch addresses a “Bobby Tables” issue in the put*ent functions and
>> the getent program, similar to one of the recent libuser issues.
>>
>> I believe this is just hardening because users of the put*ent functions
>> already have appropriate checks before they call these functions, so
>> this is definitely post-freeze material.
>>
>> Tested on x86_64-redhat-linux-gnu.  Okay to commit after master reopens?
> 
> Looks good to me for 2.23 with testsuite comment nits fixed.

Thanks, I committed this with the changes suggested in this thread.

Florian