nss_db: protect against empty mappings

  Resolves: #24696

(note to reviewers: Sergi contributed the one-line fix, I did the
rest, for the purposes of copyright-line-counts)

2019-06-17  DJ Delorie  <dj@redhat.com>
	    Sergei Trofimovich <slyfox@inbox.ru>

	* nss/nss_db/db-open.c (internal_endent): Protect against NULL
	mappings.
	* nss/tst-nss-db-endgrent.c: New.
	* nss/tst-nss-db-endgrent.root: New.
	* nss/Makefile: Add new test.
  

On 6/17/19 8:28 PM, DJ Delorie wrote:
> Resolves: #24696
> 
> (note to reviewers: Sergi contributed the one-line fix, I did the
> rest, for the purposes of copyright-line-counts)
> 
> 2019-06-17  DJ Delorie  <dj@redhat.com>
> 	    Sergei Trofimovich <slyfox@inbox.ru>
> 

Requires bug # reference.

> 	* nss/nss_db/db-open.c (internal_endent): Protect against NULL
> 	mappings.
> 	* nss/tst-nss-db-endgrent.c: New.
> 	* nss/tst-nss-db-endgrent.root: New.
> 	* nss/Makefile: Add new test.
> 

Please post v2.

> diff --git a/nss/Makefile b/nss/Makefile
> index 15fc410cf1..a15c3b7d90 100644
> --- a/nss/Makefile
> +++ b/nss/Makefile
> @@ -62,7 +62,8 @@ xtests			= bug-erange
>  tests-container = \
>  			  tst-nss-test3 \
>  			  tst-nss-files-hosts-long \
> -			  tst-nss-db-endpwent
> +			  tst-nss-db-endpwent \
> +			  tst-nss-db-endgrent

OK, new container test.

>  
>  # Tests which need libdl
>  ifeq (yes,$(build-shared))
> diff --git a/nss/nss_db/db-open.c b/nss/nss_db/db-open.c
> index f7c53b4486..3fa11e9ab0 100644
> --- a/nss/nss_db/db-open.c
> +++ b/nss/nss_db/db-open.c
> @@ -63,6 +63,9 @@ internal_setent (const char *file, struct nss_db_map *mapping)
>  void
>  internal_endent (struct nss_db_map *mapping)
>  {
> -  munmap (mapping->header, mapping->len);
> -  mapping->header = NULL;
> +  if (mapping->header != NULL)

OK, if we never loaded a mapping don't attempt to unmap, otherwise we'll clobber
errno via munmap that fails.

> +    {
> +      munmap (mapping->header, mapping->len);
> +      mapping->header = NULL;

No. This merges the fix for 24695 also. We should be careful here to fix just
the thing that's broken and avoid merging both fixes which are coincidentally
in the same place. I would suggest removing the mapping->header = NULL; change
and leave it for your other patch to change.

> +    }
>  }
> diff --git a/nss/tst-nss-db-endgrent.c b/nss/tst-nss-db-endgrent.c
> new file mode 100644
> index 0000000000..f4c1e14d60
> --- /dev/null
> +++ b/nss/tst-nss-db-endgrent.c
> @@ -0,0 +1,46 @@
> +/* Test for endgrent changing errno.

Reference bug # in first line please.

> +   Copyright (C) 2019 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 <stdlib.h>
> +#include <sys/types.h>
> +#include <grp.h>
> +#include <unistd.h>
> +#include <errno.h>
> +
> +#include <support/check.h>
> +#include <support/support.h>
> +
> +

Needs a paragraph explaining what this test is doing.

e.g.

/* The expectation is that a simple call to endgrent
   with only the db and files databases should never set
   errno because there is nothing that can fail. The
   database was never opened and so endgrent is no-op.  */

> +static int
> +do_test (void)
> +{
> +  /* Just make sure it's not there, although usually it won't be.  */
> +  unlink ("/var/db/group.db");
> +
> +  getgrent ();

Do you need this getgrent() or can you call endgrent() right away?

A call to endgrent() should be idempotent, it should do nothing,
and we should be able to call it many times without errno being
set.

It would be a simpler test if we just called endgrent().

> +
> +  int saved_errno = errno;

This isn't correct because errno is not defined, or at least you
haven't verified getgrent might have set it.

Suggest:

errno = 0;

> +
> +  endgrent ();
> +
> +  if (errno != saved_errno)

then:

if (errno != 0)

> +    FAIL_EXIT1 ("endgrent changed errno from %d o %d\n", saved_errno, errno);

FAIL_EXIT1 ("endgrent failed no-op call with errno of %d\n", errno);

> +
> +  return 0;
> +}
> +#include <support/test-driver.c>
> diff --git a/nss/tst-nss-db-endgrent.root/etc/nsswitch.conf b/nss/tst-nss-db-endgrent.root/etc/nsswitch.conf
> new file mode 100644
> index 0000000000..21471df94f
> --- /dev/null
> +++ b/nss/tst-nss-db-endgrent.root/etc/nsswitch.conf
> @@ -0,0 +1 @@
> +group : db files
>