nss_files: Fix /etc/aliases null pointer dereference [BZ #24059]

Message ID 87a7k1531v.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series
  • nss_files: Fix /etc/aliases null pointer dereference [BZ #24059]
Related show

Commit Message

Florian Weimer Jan. 15, 2019, 3:54 p.m.
If /etc/aliases ends with a continuation line (a line that starts
with whitespace) which does not have a training newline character,
the file parser would crash due to a null pointer dereference.

2019-01-15  Florian Weimer  <fweimer@redhat.com>

	[BZ #24059]
	* nss/nss_files/files-alias.c (get_next_alias): Handle
	continuation line without newline at the end.
	* nss/tst-nss-files-alias-truncated.c: New file.
	* nss/Makefile [$(build-shared)] (tests): Add
	tst-nss-files-alias-truncated.
	(tst-nss-files-alias-truncated): Link with libnss_files.so.
	* support/namespace.h (struct support_chroot_configuration): Add
	aliases member.
	(struct support_chroot): Add path_aliases member.
	* support/support_chroot.c (support_chroot_create): Handle
	aliases.
	(support_chroot_free): Free path_aliases.

Comments

Carlos O'Donell Jan. 15, 2019, 4:51 p.m. | #1
On 1/15/19 10:54 AM, Florian Weimer wrote:
> If /etc/aliases ends with a continuation line (a line that starts

> with whitespace) which does not have a training newline character,

> the file parser would crash due to a null pointer dereference.

> 

> 2019-01-15  Florian Weimer  <fweimer@redhat.com>

> 

> 	[BZ #24059]

> 	* nss/nss_files/files-alias.c (get_next_alias): Handle

> 	continuation line without newline at the end.

> 	* nss/tst-nss-files-alias-truncated.c: New file.

> 	* nss/Makefile [$(build-shared)] (tests): Add

> 	tst-nss-files-alias-truncated.

> 	(tst-nss-files-alias-truncated): Link with libnss_files.so.

> 	* support/namespace.h (struct support_chroot_configuration): Add

> 	aliases member.

> 	(struct support_chroot): Add path_aliases member.

> 	* support/support_chroot.c (support_chroot_create): Handle

> 	aliases.

> 	(support_chroot_free): Free path_aliases.

> 


OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>


> diff --git a/nss/Makefile b/nss/Makefile

> index 0fa28f0c5e..e66e01d45a 100644

> --- a/nss/Makefile

> +++ b/nss/Makefile

> @@ -68,6 +68,7 @@ tests += tst-nss-files-hosts-erange

>  tests += tst-nss-files-hosts-multi

>  tests += tst-nss-files-hosts-getent

>  tests += tst-nss-files-alias-leak

> +tests += tst-nss-files-alias-truncated


OK.

>  endif

>  

>  # If we have a thread library then we can test cancellation against

> @@ -176,3 +177,4 @@ $(objpfx)tst-nss-files-hosts-multi: $(libdl)

>  $(objpfx)tst-nss-files-hosts-getent: $(libdl)

>  $(objpfx)tst-nss-files-alias-leak: $(libdl)

>  $(objpfx)tst-nss-files-alias-leak.out: $(objpfx)/libnss_files.so

> +$(objpfx)tst-nss-files-alias-truncated: $(objpfx)/libnss_files.so

> diff --git a/nss/nss_files/files-alias.c b/nss/nss_files/files-alias.c

> index 540eb9c678..34d7aa84cc 100644

> --- a/nss/nss_files/files-alias.c

> +++ b/nss/nss_files/files-alias.c

> @@ -333,6 +333,16 @@ get_next_alias (FILE *stream, const char *match, struct aliasent *result,

>  		     can be ignored.  */

>  		  first_unused[room_left - 1] = '\xff';

>  		  line = fgets_unlocked (first_unused, room_left, stream);

> +		  if (line == NULL)

> +		    {

> +		      /* Continuation line without any data and

> +			 without a newline at the end.  Treat it as an

> +			 empty line and retry, reaching EOF once

> +			 more.  */

> +		      line = first_unused;

> +		      *line = '\0';

> +		      continue;

> +		    }


OK, so this allows us to process the previously read line as a whole
instead of throwing it away because of a truncated continuation. Setting
line to first_unused and terminating is OK.

>  		  if (first_unused[room_left - 1] != '\xff')

>  		    goto no_more_room;

>  		  cp = strpbrk (line, "#\n");

> diff --git a/nss/tst-nss-files-alias-truncated.c b/nss/tst-nss-files-alias-truncated.c

> new file mode 100644

> index 0000000000..2d6aba3c0e

> --- /dev/null

> +++ b/nss/tst-nss-files-alias-truncated.c

> @@ -0,0 +1,66 @@

> +/* Check handling of missing end-of-line at end of /etc/aliases (bug 24059).


OK. My preference would be a test-container test that doesn't have all of this
boiler plate, but I leave that choice up to you.

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

> +#include <nss.h>

> +#include <stddef.h>

> +#include <support/check.h>

> +#include <support/namespace.h>

> +#include <support/test-driver.h>

> +#include <support/xunistd.h>

> +

> +static void

> +in_chroot (void *closure)

> +{

> +  struct support_chroot *chroot_env = closure;

> +  xchroot (chroot_env->path_chroot);

> +

> +  struct aliasent *e = getaliasbyname ("user1");

> +  TEST_VERIFY_EXIT (e != NULL);

> +  TEST_COMPARE_STRING (e->alias_name, "user1");

> +  TEST_COMPARE (e->alias_members_len, 1);

> +  TEST_VERIFY_EXIT (e->alias_members != NULL);

> +  TEST_COMPARE_STRING (e->alias_members[0], "alias1");

> +  TEST_VERIFY (e->alias_local);


OK.

> +}

> +

> +static int

> +do_test (void)

> +{

> +  /* nss_files has already been loaded via DT_NEEDED, outside the

> +     chroot.  */

> +  __nss_configure_lookup ("aliases", "files");

> +

> +  support_become_root ();

> +  if (!support_can_chroot ())

> +    return EXIT_UNSUPPORTED;

> +

> +  struct support_chroot *chroot_env = support_chroot_create

> +    ((struct support_chroot_configuration)

> +     {

> +       .aliases = "user1: alias1,\n"

> +        " "              /* Continuation line, but no \n.  */


OK.

> +     });

> +

> +  support_isolate_in_subprocess (in_chroot, chroot_env);

> +

> +  support_chroot_free (chroot_env);

> +  return 0;

> +}

> +

> +#include <support/test-driver.c>

> diff --git a/support/namespace.h b/support/namespace.h

> index f93754934f..4770cf9e90 100644

> --- a/support/namespace.h

> +++ b/support/namespace.h

> @@ -74,6 +74,7 @@ struct support_chroot_configuration

>    const char *resolv_conf;      /* /etc/resolv.conf.  */

>    const char *hosts;            /* /etc/hosts.  */

>    const char *host_conf;        /* /etc/host.conf.  */

> +  const char *aliases;          /* /etc/aliases.  */


OK.

>  };

>  

>  /* The result of the creation of a chroot.  */

> @@ -90,6 +91,7 @@ struct support_chroot

>    char *path_resolv_conf;       /* /etc/resolv.conf.  */

>    char *path_hosts;             /* /etc/hosts.  */

>    char *path_host_conf;         /* /etc/host.conf.  */

> +  char *path_aliases;           /* /etc/aliases.  */


OK.

>  };

>  

>  /* Create a chroot environment.  The returned data should be freed

> diff --git a/support/support_chroot.c b/support/support_chroot.c

> index b866b89a18..412ec33f53 100644

> --- a/support/support_chroot.c

> +++ b/support/support_chroot.c

> @@ -56,6 +56,7 @@ support_chroot_create (struct support_chroot_configuration conf)

>                &chroot->path_resolv_conf);

>    write_file (path_etc, "hosts", conf.hosts, &chroot->path_hosts);

>    write_file (path_etc, "host.conf", conf.host_conf, &chroot->path_host_conf);

> +  write_file (path_etc, "aliases", conf.aliases, &chroot->path_aliases);


OK.

>  

>    free (path_etc);

>  

> @@ -77,5 +78,6 @@ support_chroot_free (struct support_chroot *chroot)

>    free (chroot->path_resolv_conf);

>    free (chroot->path_hosts);

>    free (chroot->path_host_conf);

> +  free (chroot->path_aliases);


OK.

>    free (chroot);

>  }

> 



-- 
Cheers,
Carlos.
Szabolcs Nagy Feb. 1, 2019, 2:40 p.m. | #2
On 15/01/2019 15:54, Florian Weimer wrote:
> If /etc/aliases ends with a continuation line (a line that starts

> with whitespace) which does not have a training newline character,

> the file parser would crash due to a null pointer dereference.

> 

> 2019-01-15  Florian Weimer  <fweimer@redhat.com>

> 

> 	[BZ #24059]

> 	* nss/nss_files/files-alias.c (get_next_alias): Handle

> 	continuation line without newline at the end.

> 	* nss/tst-nss-files-alias-truncated.c: New file.

> 	* nss/Makefile [$(build-shared)] (tests): Add

> 	tst-nss-files-alias-truncated.

> 	(tst-nss-files-alias-truncated): Link with libnss_files.so.

> 	* support/namespace.h (struct support_chroot_configuration): Add

> 	aliases member.

> 	(struct support_chroot): Add path_aliases member.

> 	* support/support_chroot.c (support_chroot_create): Handle

> 	aliases.

> 	(support_chroot_free): Free path_aliases.


aarch64 buildbot consistently fails with

FAIL: nss/tst-nss-files-alias-truncated

error: tst-nss-files-alias-truncated.c:34: not true: e != NULL
error: support_isolate_in_subprocess.c:37: child process exited with status 256
error: 2 test failures

i'm not yet sure why (passes with ./testrun.sh)
Florian Weimer Feb. 1, 2019, 3:15 p.m. | #3
* Szabolcs Nagy:

> On 15/01/2019 15:54, Florian Weimer wrote:

>> If /etc/aliases ends with a continuation line (a line that starts

>> with whitespace) which does not have a training newline character,

>> the file parser would crash due to a null pointer dereference.

>> 

>> 2019-01-15  Florian Weimer  <fweimer@redhat.com>

>> 

>> 	[BZ #24059]

>> 	* nss/nss_files/files-alias.c (get_next_alias): Handle

>> 	continuation line without newline at the end.

>> 	* nss/tst-nss-files-alias-truncated.c: New file.

>> 	* nss/Makefile [$(build-shared)] (tests): Add

>> 	tst-nss-files-alias-truncated.

>> 	(tst-nss-files-alias-truncated): Link with libnss_files.so.

>> 	* support/namespace.h (struct support_chroot_configuration): Add

>> 	aliases member.

>> 	(struct support_chroot): Add path_aliases member.

>> 	* support/support_chroot.c (support_chroot_create): Handle

>> 	aliases.

>> 	(support_chroot_free): Free path_aliases.

>

> aarch64 buildbot consistently fails with

>

> FAIL: nss/tst-nss-files-alias-truncated

>

> error: tst-nss-files-alias-truncated.c:34: not true: e != NULL

> error: support_isolate_in_subprocess.c:37: child process exited with status 256

> error: 2 test failures

>

> i'm not yet sure why (passes with ./testrun.sh)


I haven't seen that, and I don't know what could be causing it.  I fear
you have to debug it in the context of the build bot, sorry.

Thanks,
Florian
Szabolcs Nagy Feb. 1, 2019, 5:21 p.m. | #4
On 01/02/2019 15:15, Florian Weimer wrote:
> * Szabolcs Nagy:

> 

>> On 15/01/2019 15:54, Florian Weimer wrote:

>>> If /etc/aliases ends with a continuation line (a line that starts

>>> with whitespace) which does not have a training newline character,

>>> the file parser would crash due to a null pointer dereference.

>>>

>>> 2019-01-15  Florian Weimer  <fweimer@redhat.com>

>>>

>>> 	[BZ #24059]

>>> 	* nss/nss_files/files-alias.c (get_next_alias): Handle

>>> 	continuation line without newline at the end.

>>> 	* nss/tst-nss-files-alias-truncated.c: New file.

>>> 	* nss/Makefile [$(build-shared)] (tests): Add

>>> 	tst-nss-files-alias-truncated.

>>> 	(tst-nss-files-alias-truncated): Link with libnss_files.so.

>>> 	* support/namespace.h (struct support_chroot_configuration): Add

>>> 	aliases member.

>>> 	(struct support_chroot): Add path_aliases member.

>>> 	* support/support_chroot.c (support_chroot_create): Handle

>>> 	aliases.

>>> 	(support_chroot_free): Free path_aliases.

>>

>> aarch64 buildbot consistently fails with

>>

>> FAIL: nss/tst-nss-files-alias-truncated

>>

>> error: tst-nss-files-alias-truncated.c:34: not true: e != NULL

>> error: support_isolate_in_subprocess.c:37: child process exited with status 256

>> error: 2 test failures

>>

>> i'm not yet sure why (passes with ./testrun.sh)

> 

> I haven't seen that, and I don't know what could be causing it.  I fear

> you have to debug it in the context of the build bot, sorry.


libnss_files.so.2 is not listed as explicit dependency
for some reason so it is not loaded at program startup,
but after chroot is entered, but it's not available in
the chroot so the first api call fails.

(the buildbot used a gcc-5 toolchain so now that we require
gcc-6 the aarch64 buildbot is dead anyway.)
Florian Weimer Feb. 1, 2019, 5:32 p.m. | #5
* Szabolcs Nagy:

> On 01/02/2019 15:15, Florian Weimer wrote:

>> * Szabolcs Nagy:

>> 

>>> On 15/01/2019 15:54, Florian Weimer wrote:

>>>> If /etc/aliases ends with a continuation line (a line that starts

>>>> with whitespace) which does not have a training newline character,

>>>> the file parser would crash due to a null pointer dereference.

>>>>

>>>> 2019-01-15  Florian Weimer  <fweimer@redhat.com>

>>>>

>>>> 	[BZ #24059]

>>>> 	* nss/nss_files/files-alias.c (get_next_alias): Handle

>>>> 	continuation line without newline at the end.

>>>> 	* nss/tst-nss-files-alias-truncated.c: New file.

>>>> 	* nss/Makefile [$(build-shared)] (tests): Add

>>>> 	tst-nss-files-alias-truncated.

>>>> 	(tst-nss-files-alias-truncated): Link with libnss_files.so.

>>>> 	* support/namespace.h (struct support_chroot_configuration): Add

>>>> 	aliases member.

>>>> 	(struct support_chroot): Add path_aliases member.

>>>> 	* support/support_chroot.c (support_chroot_create): Handle

>>>> 	aliases.

>>>> 	(support_chroot_free): Free path_aliases.

>>>

>>> aarch64 buildbot consistently fails with

>>>

>>> FAIL: nss/tst-nss-files-alias-truncated

>>>

>>> error: tst-nss-files-alias-truncated.c:34: not true: e != NULL

>>> error: support_isolate_in_subprocess.c:37: child process exited with status 256

>>> error: 2 test failures

>>>

>>> i'm not yet sure why (passes with ./testrun.sh)

>> 

>> I haven't seen that, and I don't know what could be causing it.  I fear

>> you have to debug it in the context of the build bot, sorry.

>

> libnss_files.so.2 is not listed as explicit dependency

> for some reason so it is not loaded at program startup,

> but after chroot is entered, but it's not available in

> the chroot so the first api call fails.


There's already this in nss/Makefile:

$(objpfx)tst-nss-files-alias-truncated: $(objpfx)/libnss_files.so

I expected this causes a DT_NEEDED entry to be added to the executable,
so it is loaded upon startup, outside of the chroot.

Thanks,
Florian
Szabolcs Nagy Feb. 2, 2019, 4:01 p.m. | #6
* Florian Weimer <fweimer@redhat.com> [2019-02-01 18:32:23 +0100]:
> * Szabolcs Nagy:

> >

> > libnss_files.so.2 is not listed as explicit dependency

> > for some reason so it is not loaded at program startup,

> > but after chroot is entered, but it's not available in

> > the chroot so the first api call fails.

> 

> There's already this in nss/Makefile:

> 

> $(objpfx)tst-nss-files-alias-truncated: $(objpfx)/libnss_files.so

> 

> I expected this causes a DT_NEEDED entry to be added to the executable,

> so it is loaded upon startup, outside of the chroot.


it seems debian/ubuntu gcc always passes --as-needed to the linker
https://wiki.debian.org/ToolChain/DSOLinking

so either a reference is needed to the lib or
 -Wl,--no-as-needed lib -Wl,--as-needed
ldflag to force a DT_NEEDED.

..or copy the lib to the chroot.
Florian Weimer Feb. 2, 2019, 5:03 p.m. | #7
* Szabolcs Nagy:

> * Florian Weimer <fweimer@redhat.com> [2019-02-01 18:32:23 +0100]:

>> * Szabolcs Nagy:

>> >

>> > libnss_files.so.2 is not listed as explicit dependency

>> > for some reason so it is not loaded at program startup,

>> > but after chroot is entered, but it's not available in

>> > the chroot so the first api call fails.

>> 

>> There's already this in nss/Makefile:

>> 

>> $(objpfx)tst-nss-files-alias-truncated: $(objpfx)/libnss_files.so

>> 

>> I expected this causes a DT_NEEDED entry to be added to the executable,

>> so it is loaded upon startup, outside of the chroot.

>

> it seems debian/ubuntu gcc always passes --as-needed to the linker

> https://wiki.debian.org/ToolChain/DSOLinking


Nice find.  Yes, that completely explains it.

> so either a reference is needed to the lib or

>  -Wl,--no-as-needed lib -Wl,--as-needed

> ldflag to force a DT_NEEDED.


Hmm.  Should we put this on the general linker command line for tests?
Are there any cases where we actually want --as-needed behavior for
objects linked into tests?

> ..or copy the lib to the chroot.


The test-in-container framework does not work on our currrent reference
machines, even as root. 8-(

In other tests, I have used dlopen before the chroot.  I could adjust
this test accordingly, but it adds needless complexity in my opinion.
Florian Weimer March 13, 2019, 4:36 p.m. | #8
* Szabolcs Nagy:

> * Florian Weimer <fweimer@redhat.com> [2019-02-01 18:32:23 +0100]:

>> * Szabolcs Nagy:

>> >

>> > libnss_files.so.2 is not listed as explicit dependency

>> > for some reason so it is not loaded at program startup,

>> > but after chroot is entered, but it's not available in

>> > the chroot so the first api call fails.

>> 

>> There's already this in nss/Makefile:

>> 

>> $(objpfx)tst-nss-files-alias-truncated: $(objpfx)/libnss_files.so

>> 

>> I expected this causes a DT_NEEDED entry to be added to the executable,

>> so it is loaded upon startup, outside of the chroot.

>

> it seems debian/ubuntu gcc always passes --as-needed to the linker

> https://wiki.debian.org/ToolChain/DSOLinking

>

> so either a reference is needed to the lib or

>  -Wl,--no-as-needed lib -Wl,--as-needed

> ldflag to force a DT_NEEDED.

>

> ..or copy the lib to the chroot.


So what should we do here?

Should we disable --as-needed across the board?  Or should we fix the
test?

Thanks,
Florian
Adhemerval Zanella March 13, 2019, 5:48 p.m. | #9
On 02/02/2019 15:03, Florian Weimer wrote:
> * Szabolcs Nagy:

> 

>> * Florian Weimer <fweimer@redhat.com> [2019-02-01 18:32:23 +0100]:

>>> * Szabolcs Nagy:

>>>>

>>>> libnss_files.so.2 is not listed as explicit dependency

>>>> for some reason so it is not loaded at program startup,

>>>> but after chroot is entered, but it's not available in

>>>> the chroot so the first api call fails.

>>>

>>> There's already this in nss/Makefile:

>>>

>>> $(objpfx)tst-nss-files-alias-truncated: $(objpfx)/libnss_files.so

>>>

>>> I expected this causes a DT_NEEDED entry to be added to the executable,

>>> so it is loaded upon startup, outside of the chroot.

>>

>> it seems debian/ubuntu gcc always passes --as-needed to the linker

>> https://wiki.debian.org/ToolChain/DSOLinking

> 

> Nice find.  Yes, that completely explains it.

> 

>> so either a reference is needed to the lib or

>>  -Wl,--no-as-needed lib -Wl,--as-needed

>> ldflag to force a DT_NEEDED.

> 

> Hmm.  Should we put this on the general linker command line for tests?

> Are there any cases where we actually want --as-needed behavior for

> objects linked into tests?

> 

>> ..or copy the lib to the chroot.

> 

> The test-in-container framework does not work on our currrent reference

> machines, even as root. 8-(

> 

> In other tests, I have used dlopen before the chroot.  I could adjust

> this test accordingly, but it adds needless complexity in my opinion.

> 


What is preventing the tests to work with test-in-container by copying
the lib in the chroot?
Szabolcs Nagy March 13, 2019, 6:36 p.m. | #10
On 13/03/2019 16:36, Florian Weimer wrote:
> * Szabolcs Nagy:

> 

>> * Florian Weimer <fweimer@redhat.com> [2019-02-01 18:32:23 +0100]:

>>> * Szabolcs Nagy:

>>>>

>>>> libnss_files.so.2 is not listed as explicit dependency

>>>> for some reason so it is not loaded at program startup,

>>>> but after chroot is entered, but it's not available in

>>>> the chroot so the first api call fails.

>>>

>>> There's already this in nss/Makefile:

>>>

>>> $(objpfx)tst-nss-files-alias-truncated: $(objpfx)/libnss_files.so

>>>

>>> I expected this causes a DT_NEEDED entry to be added to the executable,

>>> so it is loaded upon startup, outside of the chroot.

>>

>> it seems debian/ubuntu gcc always passes --as-needed to the linker

>> https://wiki.debian.org/ToolChain/DSOLinking

>>

>> so either a reference is needed to the lib or

>>  -Wl,--no-as-needed lib -Wl,--as-needed

>> ldflag to force a DT_NEEDED.

>>

>> ..or copy the lib to the chroot.

> 

> So what should we do here?

> 

> Should we disable --as-needed across the board?  Or should we fix the

> test?


ideally the chroot should have all the runtime libs that
may be loaded, if that's too big hassle, then i'd fix
the test (to enforce DT_NEEDED or use dlopen before chroot).

(this issue affects the aarch64 buildbot, but that's not
running since the minimum gcc requirement got increased,
i'll ask for a buildbot server restart)
Florian Weimer March 14, 2019, 9:10 a.m. | #11
* Adhemerval Zanella:

> What is preventing the tests to work with test-in-container by copying

> the lib in the chroot?


For test-in-container, I'm still trying to figure out how to reliably
re-run a test after a test update, without having to wait five minutes
or more, until a rebuild from nothing completes.

Thanks,
Florian
Florian Weimer March 14, 2019, 2:04 p.m. | #12
* Szabolcs Nagy:

> On 13/03/2019 16:36, Florian Weimer wrote:

>> * Szabolcs Nagy:

>> 

>>> * Florian Weimer <fweimer@redhat.com> [2019-02-01 18:32:23 +0100]:

>>>> * Szabolcs Nagy:

>>>>>

>>>>> libnss_files.so.2 is not listed as explicit dependency

>>>>> for some reason so it is not loaded at program startup,

>>>>> but after chroot is entered, but it's not available in

>>>>> the chroot so the first api call fails.

>>>>

>>>> There's already this in nss/Makefile:

>>>>

>>>> $(objpfx)tst-nss-files-alias-truncated: $(objpfx)/libnss_files.so

>>>>

>>>> I expected this causes a DT_NEEDED entry to be added to the executable,

>>>> so it is loaded upon startup, outside of the chroot.

>>>

>>> it seems debian/ubuntu gcc always passes --as-needed to the linker

>>> https://wiki.debian.org/ToolChain/DSOLinking

>>>

>>> so either a reference is needed to the lib or

>>>  -Wl,--no-as-needed lib -Wl,--as-needed

>>> ldflag to force a DT_NEEDED.

>>>

>>> ..or copy the lib to the chroot.

>> 

>> So what should we do here?

>> 

>> Should we disable --as-needed across the board?  Or should we fix the

>> test?

>

> ideally the chroot should have all the runtime libs that

> may be loaded, if that's too big hassle, then i'd fix

> the test (to enforce DT_NEEDED or use dlopen before chroot).


Let's do it via dlopen.  See below.

Thanks,
Florian

nss: Fix tst-nss-files-alias-truncated for default --as-needed linking

Linking to the NSS module directly does not work if the linker defaults
to --as-needed because it will remove the apparently unused DSO
reference and not generate a DT_NEEDED entry.  Use an explicit dlopen
call, like in the other chroot tests involving NSS modules.

2019-03-14  Florian Weimer  <fweimer@redhat.com>

	* nss/tst-nss-files-alias-truncated.c (do_test): Load
	libnss_files.
	* nss/Makefile (tst-nss-files-alias-truncated): Link with -ldl,
	but not with libnss_files.
	(tst-nss-files-alias-truncated.out): Depend on libnss_files.

diff --git a/nss/Makefile b/nss/Makefile
index a8caa8af38..95081bddc5 100644
--- a/nss/Makefile
+++ b/nss/Makefile
@@ -178,4 +178,5 @@ $(objpfx)tst-nss-files-hosts-multi: $(libdl)
 $(objpfx)tst-nss-files-hosts-getent: $(libdl)
 $(objpfx)tst-nss-files-alias-leak: $(libdl)
 $(objpfx)tst-nss-files-alias-leak.out: $(objpfx)/libnss_files.so
-$(objpfx)tst-nss-files-alias-truncated: $(objpfx)/libnss_files.so
+$(objpfx)tst-nss-files-alias-truncated: $(libdl)
+$(objpfx)tst-nss-files-alias-truncated.out: $(objpfx)/libnss_files.so
diff --git a/nss/tst-nss-files-alias-truncated.c b/nss/tst-nss-files-alias-truncated.c
index 2d6aba3c0e..029ae6a2a7 100644
--- a/nss/tst-nss-files-alias-truncated.c
+++ b/nss/tst-nss-files-alias-truncated.c
@@ -17,11 +17,13 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <aliases.h>
+#include <gnu/lib-names.h>
 #include <nss.h>
 #include <stddef.h>
 #include <support/check.h>
 #include <support/namespace.h>
 #include <support/test-driver.h>
+#include <support/xdlfcn.h>
 #include <support/xunistd.h>
 
 static void
@@ -42,8 +44,9 @@ in_chroot (void *closure)
 static int
 do_test (void)
 {
-  /* nss_files has already been loaded via DT_NEEDED, outside the
-     chroot.  */
+  /* Make sure we don't try to load the module in the chroot.  */
+  xdlopen (LIBNSS_FILES_SO, RTLD_NOW);
+
   __nss_configure_lookup ("aliases", "files");
 
   support_become_root ();
Szabolcs Nagy March 14, 2019, 2:09 p.m. | #13
On 14/03/2019 14:04, Florian Weimer wrote:
> * Szabolcs Nagy:

>> ideally the chroot should have all the runtime libs that

>> may be loaded, if that's too big hassle, then i'd fix

>> the test (to enforce DT_NEEDED or use dlopen before chroot).

> 

> Let's do it via dlopen.  See below.

> 

> Thanks,

> Florian

> 

> nss: Fix tst-nss-files-alias-truncated for default --as-needed linking

> 

> Linking to the NSS module directly does not work if the linker defaults

> to --as-needed because it will remove the apparently unused DSO

> reference and not generate a DT_NEEDED entry.  Use an explicit dlopen

> call, like in the other chroot tests involving NSS modules.

> 

> 2019-03-14  Florian Weimer  <fweimer@redhat.com>

> 

> 	* nss/tst-nss-files-alias-truncated.c (do_test): Load

> 	libnss_files.

> 	* nss/Makefile (tst-nss-files-alias-truncated): Link with -ldl,

> 	but not with libnss_files.

> 	(tst-nss-files-alias-truncated.out): Depend on libnss_files.


looks reasonable.

thanks.

Patch

diff --git a/nss/Makefile b/nss/Makefile
index 0fa28f0c5e..e66e01d45a 100644
--- a/nss/Makefile
+++ b/nss/Makefile
@@ -68,6 +68,7 @@  tests += tst-nss-files-hosts-erange
 tests += tst-nss-files-hosts-multi
 tests += tst-nss-files-hosts-getent
 tests += tst-nss-files-alias-leak
+tests += tst-nss-files-alias-truncated
 endif
 
 # If we have a thread library then we can test cancellation against
@@ -176,3 +177,4 @@  $(objpfx)tst-nss-files-hosts-multi: $(libdl)
 $(objpfx)tst-nss-files-hosts-getent: $(libdl)
 $(objpfx)tst-nss-files-alias-leak: $(libdl)
 $(objpfx)tst-nss-files-alias-leak.out: $(objpfx)/libnss_files.so
+$(objpfx)tst-nss-files-alias-truncated: $(objpfx)/libnss_files.so
diff --git a/nss/nss_files/files-alias.c b/nss/nss_files/files-alias.c
index 540eb9c678..34d7aa84cc 100644
--- a/nss/nss_files/files-alias.c
+++ b/nss/nss_files/files-alias.c
@@ -333,6 +333,16 @@  get_next_alias (FILE *stream, const char *match, struct aliasent *result,
 		     can be ignored.  */
 		  first_unused[room_left - 1] = '\xff';
 		  line = fgets_unlocked (first_unused, room_left, stream);
+		  if (line == NULL)
+		    {
+		      /* Continuation line without any data and
+			 without a newline at the end.  Treat it as an
+			 empty line and retry, reaching EOF once
+			 more.  */
+		      line = first_unused;
+		      *line = '\0';
+		      continue;
+		    }
 		  if (first_unused[room_left - 1] != '\xff')
 		    goto no_more_room;
 		  cp = strpbrk (line, "#\n");
diff --git a/nss/tst-nss-files-alias-truncated.c b/nss/tst-nss-files-alias-truncated.c
new file mode 100644
index 0000000000..2d6aba3c0e
--- /dev/null
+++ b/nss/tst-nss-files-alias-truncated.c
@@ -0,0 +1,66 @@ 
+/* Check handling of missing end-of-line at end of /etc/aliases (bug 24059).
+   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 <aliases.h>
+#include <nss.h>
+#include <stddef.h>
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/test-driver.h>
+#include <support/xunistd.h>
+
+static void
+in_chroot (void *closure)
+{
+  struct support_chroot *chroot_env = closure;
+  xchroot (chroot_env->path_chroot);
+
+  struct aliasent *e = getaliasbyname ("user1");
+  TEST_VERIFY_EXIT (e != NULL);
+  TEST_COMPARE_STRING (e->alias_name, "user1");
+  TEST_COMPARE (e->alias_members_len, 1);
+  TEST_VERIFY_EXIT (e->alias_members != NULL);
+  TEST_COMPARE_STRING (e->alias_members[0], "alias1");
+  TEST_VERIFY (e->alias_local);
+}
+
+static int
+do_test (void)
+{
+  /* nss_files has already been loaded via DT_NEEDED, outside the
+     chroot.  */
+  __nss_configure_lookup ("aliases", "files");
+
+  support_become_root ();
+  if (!support_can_chroot ())
+    return EXIT_UNSUPPORTED;
+
+  struct support_chroot *chroot_env = support_chroot_create
+    ((struct support_chroot_configuration)
+     {
+       .aliases = "user1: alias1,\n"
+        " "              /* Continuation line, but no \n.  */
+     });
+
+  support_isolate_in_subprocess (in_chroot, chroot_env);
+
+  support_chroot_free (chroot_env);
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/support/namespace.h b/support/namespace.h
index f93754934f..4770cf9e90 100644
--- a/support/namespace.h
+++ b/support/namespace.h
@@ -74,6 +74,7 @@  struct support_chroot_configuration
   const char *resolv_conf;      /* /etc/resolv.conf.  */
   const char *hosts;            /* /etc/hosts.  */
   const char *host_conf;        /* /etc/host.conf.  */
+  const char *aliases;          /* /etc/aliases.  */
 };
 
 /* The result of the creation of a chroot.  */
@@ -90,6 +91,7 @@  struct support_chroot
   char *path_resolv_conf;       /* /etc/resolv.conf.  */
   char *path_hosts;             /* /etc/hosts.  */
   char *path_host_conf;         /* /etc/host.conf.  */
+  char *path_aliases;           /* /etc/aliases.  */
 };
 
 /* Create a chroot environment.  The returned data should be freed
diff --git a/support/support_chroot.c b/support/support_chroot.c
index b866b89a18..412ec33f53 100644
--- a/support/support_chroot.c
+++ b/support/support_chroot.c
@@ -56,6 +56,7 @@  support_chroot_create (struct support_chroot_configuration conf)
               &chroot->path_resolv_conf);
   write_file (path_etc, "hosts", conf.hosts, &chroot->path_hosts);
   write_file (path_etc, "host.conf", conf.host_conf, &chroot->path_host_conf);
+  write_file (path_etc, "aliases", conf.aliases, &chroot->path_aliases);
 
   free (path_etc);
 
@@ -77,5 +78,6 @@  support_chroot_free (struct support_chroot *chroot)
   free (chroot->path_resolv_conf);
   free (chroot->path_hosts);
   free (chroot->path_host_conf);
+  free (chroot->path_aliases);
   free (chroot);
 }