| Message ID | 5664991E.4090105@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/06/2015 03:22 PM, Florian Weimer wrote: > The attached patch adds a test hook which allows us to select different > files (not those under /etc) for testing nss_files inside the build > tree. It contains a regression test for bug 17363 as an example. > > If the direction is acceptable, I'll add further tests. [snip] > There are also a few additional relocations (for the string constants), > but they should not matter. I have two concerns. (1) Security. What security implications are there in exposing this interface? (2) Test what we ship. We need to get away from build-tree testing and move to installed tree testing to verify that we are testing is what we are shipping. The testing would look like this: - Setup an installed tree. - Setup the test. - Run the test in some kind of isolation with configuration changes made to the sysroot that would otherwise be impossible on the host. - Return status. - Repeat for all tests that need a sysroot e.g. ldconfig, network, nss... That is not to say that whitebox hook-based testing is bad, but it deviates from (1) and (2) in ways which make me uncomfortable. A more appealing alternative would be to run the test under a systemtap script which did all the work of updating the paths to the databases without the hook changes. Thoughts? Cheers, Carlos.
On 12/07/2015 05:55 PM, Carlos O'Donell wrote: > I have two concerns. > > (1) Security. > > What security implications are there in exposing this interface? I added a function that has to be called instead of an environment variable precisely so that there are no security concerns. The function is prefixed with _nss_files, which is what we use for namespacing the NSS service modules. > (2) Test what we ship. > > We need to get away from build-tree testing and move to installed tree > testing to verify that we are testing is what we are shipping. My proposed tests do that, which is why there is a hook. An alternative would be to compile nss_files twice, with different settings. But then we aren't testing anymore what we are shipping. > The testing would look like this: > > - Setup an installed tree. > - Setup the test. > - Run the test in some kind of isolation with configuration changes > made to the sysroot that would otherwise be impossible on the host. > - Return status. > - Repeat for all tests that need a sysroot e.g. ldconfig, network, nss... I agree that installed-tree testing is better. At least the nss_files tests should be straightforward to migrate when installed-tree testing arrives. You just omit the path override, and copy the test files to /etc in the test environment. > A more appealing alternative would be to run the test under a systemtap > script which did all the work of updating the paths to the databases > without the hook changes. I think this would be far more brittle and difficult to implement because the existing path names are just string literals. Run-time patching also means that it's not really what we ship. At that point, we may be better off with something like cwrap, or an xtest with chroot. Florian
On 12/07/2015 01:34 PM, Florian Weimer wrote: > On 12/07/2015 05:55 PM, Carlos O'Donell wrote: > >> I have two concerns. >> >> (1) Security. >> >> What security implications are there in exposing this interface? > > I added a function that has to be called instead of an environment > variable precisely so that there are no security concerns. The function > is prefixed with _nss_files, which is what we use for namespacing the > NSS service modules. I agree that an environment variable would not have been acceptable. My question is: Have we thought through any security implications? We went from having a number of RO-data strings scattered about to having a single structure that allows access to all of them in RW-data? It's the same concerns I have at about tunables actually, but in this context it's a risk reward of "better testing" vs. "new way to change /etc/passwd lookup". Then again, you'd need to have enough control of the target process to make a call, so you might as well do other things? What I'm asking is: Have you given thought to the security implications? Your answer is: Yes. A function is the best we can do. Did I understand that right? Can we harden this global table in any way? >> (2) Test what we ship. >> >> We need to get away from build-tree testing and move to installed tree >> testing to verify that we are testing is what we are shipping. > > My proposed tests do that, which is why there is a hook. An alternative > would be to compile nss_files twice, with different settings. But then > we aren't testing anymore what we are shipping. In retrospect I see what you mean here. In that the interface for initialization is always present as a private API and never removed. I like that aspect of the testing. What I don't like is that the test program doesn't behave like a normal application that would be using the APIs in question. It calls a special hook and that's really the problem I have with whitebox testing. I'm not against the new function, I'm against the test program having to use it in a special way that makes it not behave like a regular application. >> The testing would look like this: >> >> - Setup an installed tree. >> - Setup the test. >> - Run the test in some kind of isolation with configuration changes >> made to the sysroot that would otherwise be impossible on the host. >> - Return status. >> - Repeat for all tests that need a sysroot e.g. ldconfig, network, nss... > > I agree that installed-tree testing is better. At least the nss_files > tests should be straightforward to migrate when installed-tree testing > arrives. You just omit the path override, and copy the test files to > /etc in the test environment. Agreed. >> A more appealing alternative would be to run the test under a systemtap >> script which did all the work of updating the paths to the databases >> without the hook changes. > > I think this would be far more brittle and difficult to implement > because the existing path names are just string literals. Run-time > patching also means that it's not really what we ship. At that point, > we may be better off with something like cwrap, or an xtest with chroot. I like both of those options better. If we are going to add hooks for testing like you propose it's going to be because we absolutely need the hooks and there is no other way to get at the information in question. It must be a last resort that we add hooks like this since they incur maintenance burden forever. I think xtest+chroot is really the ideal solution here and would help further containerized testing. To be honest I like 99% of what you did, in fact coalescing the db names into a single struct prepares us for a set of tunables that might allow you to run applications under alternate databases by tweaking those options. Cheers, Carlos.
On 12/07/2015 09:50 PM, Carlos O'Donell wrote: > What I'm asking is: Have you given thought to the security implications? > > Your answer is: Yes. A function is the best we can do. > > Did I understand that right? > > Can we harden this global table in any way? To be honest, I'm confused by this request. We do not usually take such issues into account when making changes to the library. I think a function is indeed the best way, yes. > I'm not against the new function, I'm against the test program having to use it > in a special way that makes it not behave like a regular application. I see, and to some extend, I share that concern. >>> A more appealing alternative would be to run the test under a systemtap >>> script which did all the work of updating the paths to the databases >>> without the hook changes. >> >> I think this would be far more brittle and difficult to implement >> because the existing path names are just string literals. Run-time >> patching also means that it's not really what we ship. At that point, >> we may be better off with something like cwrap, or an xtest with chroot. > > I like both of those options better. > > If we are going to add hooks for testing like you propose it's going > to be because we absolutely need the hooks and there is no other way > to get at the information in question. It must be a last resort that > we add hooks like this since they incur maintenance burden forever. > > I think xtest+chroot is really the ideal solution here and would help > further containerized testing. I'm not so sure. Setting up the chroot in a realistic way is quite difficult. You need to populat /dev and /dev/pts in some way, and arrange for the necessary GCC and platform bits being present, whatever they are. “make install” may not actually produce a tree layout that is used by any downstream distribution. And so on. The list of issues is quite long. I think we could package such tests that they rewrite an existing chroot (or live VM image) and then do the testing they need. But these tests still would not run as part of the build process. I'll keep my patch around for local nss_files testing, but it seems that we don't want it yet in glibc proper. Florian
On 12/10/2015 09:42 AM, Florian Weimer wrote: > On 12/07/2015 09:50 PM, Carlos O'Donell wrote: > >> What I'm asking is: Have you given thought to the security implications? >> >> Your answer is: Yes. A function is the best we can do. >> >> Did I understand that right? >> >> Can we harden this global table in any way? > > To be honest, I'm confused by this request. We do not usually take such > issues into account when making changes to the library. > > I think a function is indeed the best way, yes. Perfect. No further questions. >> I'm not against the new function, I'm against the test program having to use it >> in a special way that makes it not behave like a regular application. > > I see, and to some extend, I share that concern. Good. >>>> A more appealing alternative would be to run the test under a systemtap >>>> script which did all the work of updating the paths to the databases >>>> without the hook changes. >>> >>> I think this would be far more brittle and difficult to implement >>> because the existing path names are just string literals. Run-time >>> patching also means that it's not really what we ship. At that point, >>> we may be better off with something like cwrap, or an xtest with chroot. >> >> I like both of those options better. >> >> If we are going to add hooks for testing like you propose it's going >> to be because we absolutely need the hooks and there is no other way >> to get at the information in question. It must be a last resort that >> we add hooks like this since they incur maintenance burden forever. >> >> I think xtest+chroot is really the ideal solution here and would help >> further containerized testing. > > I'm not so sure. Setting up the chroot in a realistic way is quite > difficult. You need to populat /dev and /dev/pts in some way, and > arrange for the necessary GCC and platform bits being present, whatever > they are. “make install” may not actually produce a tree layout that is > used by any downstream distribution. And so on. The list of issues is > quite long. In which regard do you think this is not ideal? Should we rely on distro images? Let the user choose which images to use for their target? At the end of the day glibc has to test it's own functionality on what it considers to be a normalized system root. When the distributions file bugs we can compare their configuration to what we used in testing. I don't think it matters that our `make install` is never used by downstream, but it has to represent a reality that we are willing to support as glibc developers. Relying on downstream to test our work has very high latency and can result in a lot of lost productivity. We really need to know immediately if a change broke something. Lastly, we may have to rely on some basic building blocks for doing this kind of testing. Without those building blocks we mark the test UNSUPPORTED. It is entirely plausible that at the start the test only works on x86_64, requires docker, and runs slim Fedora container for testing. Other targets need to fill in the technology pieces that would allow them to launch a target system that can be configured for testing and which can be thrown away or broken if the test fails. Note: As a target test it will require quite a bit of resources on the target to run, including root, so it should continue to be xcheck. > I think we could package such tests that they rewrite an existing chroot > (or live VM image) and then do the testing they need. But these tests > still would not run as part of the build process. That is one solution and was discussed at GNU Cauldron, we would create a "libc-tests" project that is an add-on to glibc (using the add-on mechanism) which would allow you to add-on additional tests that are more complex. > I'll keep my patch around for local nss_files testing, but it seems that > we don't want it yet in glibc proper. I just don't think it's the right solution. Cheers, Carlos.
On 12/11/2015 07:51 AM, Carlos O'Donell wrote: >> I'm not so sure. Setting up the chroot in a realistic way is quite >> difficult. You need to populat /dev and /dev/pts in some way, and >> arrange for the necessary GCC and platform bits being present, whatever >> they are. “make install” may not actually produce a tree layout that is >> used by any downstream distribution. And so on. The list of issues is >> quite long. > > In which regard do you think this is not ideal? It's quite a bit of work and very tightly integrated with individual distributions. Test hooks and in-tree testing directly benefit everyone who looks at test results. > Should we rely on distro images? It's not just that, you'd also need to build glibc according to the distribution's needs, regarding file system layout and so on. So it's really difficult to see how this can be part of the upstream testing. > Lastly, we may have to rely on some basic building blocks for doing this kind > of testing. Without those building blocks we mark the test UNSUPPORTED. > It is entirely plausible that at the start the test only works on x86_64, > requires docker, and runs slim Fedora container for testing. But that still looks like it needs network access, which builders generally lack (and rightly so). Florian
On 12/11/2015 07:15 AM, Florian Weimer wrote: > On 12/11/2015 07:51 AM, Carlos O'Donell wrote: > >>> I'm not so sure. Setting up the chroot in a realistic way is quite >>> difficult. You need to populat /dev and /dev/pts in some way, and >>> arrange for the necessary GCC and platform bits being present, whatever >>> they are. “make install” may not actually produce a tree layout that is >>> used by any downstream distribution. And so on. The list of issues is >>> quite long. >> >> In which regard do you think this is not ideal? > > It's quite a bit of work and very tightly integrated with individual > distributions. Test hooks and in-tree testing directly benefit everyone > who looks at test results. You raise a good point here and that's perhaps the strongest argument I've heard in favour of white-box testing for this particular interface. Test hooks do result in immediate benefits, and I'm not against them for testing intermediate state that can't otherwise be accessed via whole system tests. We need to move developers beyond the immediate `make check` and get them doing broader testing upstream and earlier to accomodate the changes they are working on. >> Should we rely on distro images? > > It's not just that, you'd also need to build glibc according to the > distribution's needs, regarding file system layout and so on. So it's > really difficult to see how this can be part of the upstream testing. Agreed. However, I don't see how that means it can't be part of the upstream testing. There is no line in the sand that says upstream stops here and the distribution starts here. >> Lastly, we may have to rely on some basic building blocks for doing this kind >> of testing. Without those building blocks we mark the test UNSUPPORTED. >> It is entirely plausible that at the start the test only works on x86_64, >> requires docker, and runs slim Fedora container for testing. > > But that still looks like it needs network access, which builders > generally lack (and rightly so). You always need network access to clone the repository. Therefore this would be an extension of the checkout process that would require you to run enough initialization to be able to use the results to do the testing. Beyond the image download there won't be much else required. Cheers, Carlos.
On Fri, Dec 11, 2015 at 9:08 AM, Carlos O'Donell <carlos@redhat.com> wrote: > > On 12/11/2015 07:15 AM, Florian Weimer wrote: > > On 12/11/2015 07:51 AM, Carlos O'Donell wrote: > > > >>> I'm not so sure. Setting up the chroot in a realistic way is quite > >>> difficult. You need to populat /dev and /dev/pts in some way, and > >>> arrange for the necessary GCC and platform bits being present, whatever > >>> they are. “make install” may not actually produce a tree layout that is > >>> used by any downstream distribution. And so on. The list of issues is > >>> quite long. > >> > >> In which regard do you think this is not ideal? > > > > It's quite a bit of work and very tightly integrated with individual > > distributions. Test hooks and in-tree testing directly benefit everyone > > who looks at test results. > > You raise a good point here and that's perhaps the strongest argument > I've heard in favour of white-box testing for this particular interface. > One of the bits of religion I've gotten at Google is that one wants white-box hooks for unit testing because the tests may be running in exotic and/or highly-restricted contexts, and your chances of getting a code path executed are better if you can write a custom test executable that drives the library down that path. Stan
2015-12-06 Florian Weimer <fweimer@redhat.com> Implement test hook for nss_files database file selection. * nss/Makefile (tests): Add tst-nss-files. (libnss_files-routines): Add files-config. (LDLIBS-tst-nss-files): New variable. * nss/Versions (libnss_files): Export _nss_files_set_config. * nss/nss_db/db-XXX.c (DBFILE_): New macro. (DBFILE): Use it to stringify DATABASE. * nss/nss_files.h: New file. * nss/nss_files/files-XXX.c: Update comment: DATABASE is now an identifier. (DATAFILE): Adjust. * nss/nss_files/files-config.c: New file. * nss/nss_files/files-ethers.c (DATABASE): Adjust. * nss/nss_files/files-group.c (DATABASE): Likewise. * nss/nss_files/files-hosts.c (DATABASE): Likewise. * nss/nss_files/files-key.c (DATABASE): Likewise. * nss/nss_files/files-netgrp.c (DATABASE): Likewise. * nss/nss_files/files-network.c (DATABASE): Likewise. * nss/nss_files/files-parse.c: Update comment: DATABASE is now an identifier. * nss/nss_files/files-proto.c (DATABASE): Adjust. * nss/nss_files/files-pwd.c (DATABASE): Likewise. * nss/nss_files/files-rpc.c (DATABASE): Likewise. * nss/nss_files/files-services.c (DATABASE): Likewise. * nss/nss_files/files-sgrp.c (DATABASE): Likewise. * nss/nss_files/files-spwd.c (DATABASE): Likewise. * nss/test-data/files/netgroup: New file. * nss/test-data/files/passwd: Likewise. * nss/tst-nss-files.c: Likewise. diff --git a/nss/Makefile b/nss/Makefile index bbbad85..e666ec6 100644 --- a/nss/Makefile +++ b/nss/Makefile @@ -50,7 +50,7 @@ extra-objs += $(makedb-modules:=.o) tests-static = tst-field tests = test-netdb tst-nss-test1 test-digits-dots \ - tst-nss-getpwent bug17079 \ + tst-nss-getpwent bug17079 tst-nss-files \ $(tests-static) xtests = bug-erange @@ -68,7 +68,8 @@ vpath %.c $(subdir-dirs) ../locale/programs ../intl libnss_files-routines := $(addprefix files-,$(databases)) \ - files-initgroups files-have_o_cloexec files-init + files-initgroups files-have_o_cloexec files-init \ + files-config libnss_db-dbs := $(addprefix db-,\ $(filter-out hosts network key alias,\ @@ -124,3 +125,5 @@ $(objpfx)/libnss_test1.so$(libnss_test1.so-version): $(objpfx)/libnss_test1.so $(make-link) endif $(objpfx)tst-nss-test1.out: $(objpfx)/libnss_test1.so$(libnss_test1.so-version) + +LDLIBS-tst-nss-files = -ldl diff --git a/nss/Versions b/nss/Versions index f8ababc..ff7dd29 100644 --- a/nss/Versions +++ b/nss/Versions @@ -100,6 +100,7 @@ libnss_files { _nss_files_initgroups_dyn; _nss_files_init; + _nss_files_set_config; } } diff --git a/nss/nss_db/db-XXX.c b/nss/nss_db/db-XXX.c index 314edc9..5db2711 100644 --- a/nss/nss_db/db-XXX.c +++ b/nss/nss_db/db-XXX.c @@ -39,7 +39,8 @@ #define ENTNAME_r CONCAT(ENTNAME,_r) #include <paths.h> -#define DBFILE _PATH_VARDB DATABASE ".db" +#define DBFILE_(name) _PATH_VARDB #name ".db" +#define DBFILE DBFILE_(name) #ifdef NEED_H_ERRNO # define H_ERRNO_PROTO , int *herrnop diff --git a/nss/nss_files.h b/nss/nss_files.h new file mode 100644 index 0000000..f700c22 --- /dev/null +++ b/nss/nss_files.h @@ -0,0 +1,51 @@ +/* Internal interfaces of the nss_files service module. + 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/>. */ + +#ifndef NSS_FILES_H +#define NSS_FILES_H + +struct nss_files_config +{ + const char *path_ethers; + const char *path_group; + const char *path_gshadow; + const char *path_hosts; + const char *path_netgroup; + const char *path_networks; + const char *path_passwd; + const char *path_protocols; + const char *path_publickey; + const char *path_rpc; + const char *path_services; + const char *path_shadow; +}; + +/* Internal configuration setting. Only available within the DSO. */ +extern struct nss_files_config _nss_files_config attribute_hidden; + +/* Internal access to the file name based on the database name. */ +#define NSS_FILES_DATABASE_(name) _nss_files_config.path_##name +#define NSS_FILES_DATABASE(name) NSS_FILES_DATABASE_ (name) + +/* Internal function to set _nss_files_config. Only copies the + struct, but not the value of the strings. Outside the DSO, this + function has to be looked up with dlsym. */ +void _nss_files_set_config (const struct nss_files_config *) + internal_function; + +#endif /* NSS_FILES_H */ diff --git a/nss/nss_files/files-XXX.c b/nss/nss_files/files-XXX.c index cca4322..63b0c02 100644 --- a/nss/nss_files/files-XXX.c +++ b/nss/nss_files/files-XXX.c @@ -22,6 +22,7 @@ #include <fcntl.h> #include <libc-lock.h> #include "nsswitch.h" +#include <nss_files.h> #include <kernel-features.h> @@ -29,7 +30,7 @@ ENTNAME -- database name of the structure and functions (hostent, pwent). STRUCTURE -- struct name, define only if not ENTNAME (passwd, group). - DATABASE -- string of the database file's name ("hosts", "passwd"). + DATABASE -- name of the database file's name ("hosts", "passwd"). NEED_H_ERRNO - defined iff an arg `int *herrnop' is used. @@ -38,7 +39,7 @@ #define ENTNAME_r CONCAT(ENTNAME,_r) -#define DATAFILE "/etc/" DATABASE +#define DATAFILE NSS_FILES_DATABASE (DATABASE) #ifdef NEED_H_ERRNO # include <netdb.h> diff --git a/nss/nss_files/files-config.c b/nss/nss_files/files-config.c new file mode 100644 index 0000000..68f9907 --- /dev/null +++ b/nss/nss_files/files-config.c @@ -0,0 +1,42 @@ +/* Files configuration for nss_files. + 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_files.h> + +struct nss_files_config _nss_files_config = + { + .path_ethers = "/etc/ethers", + .path_group = "/etc/group", + .path_gshadow = "/etc/gshadow", + .path_hosts = "/etc/hosts", + .path_netgroup = "/etc/netgroup", + .path_networks = "/etc/networks", + .path_passwd = "/etc/passwd", + .path_protocols = "/etc/protocols", + .path_publickey = "/etc/publickey", + .path_rpc = "/etc/rpc", + .path_services = "/etc/services", + .path_shadow = "/etc/shadow", + }; + +void +internal_function +_nss_files_set_config (const struct nss_files_config *config) +{ + _nss_files_config = *config; +} diff --git a/nss/nss_files/files-ethers.c b/nss/nss_files/files-ethers.c index 47fa784..ca96799 100644 --- a/nss/nss_files/files-ethers.c +++ b/nss/nss_files/files-ethers.c @@ -22,7 +22,7 @@ struct etherent_data {}; #define ENTNAME etherent -#define DATABASE "ethers" +#define DATABASE ethers #include "files-parse.c" LINE_PARSER ("#", diff --git a/nss/nss_files/files-grp.c b/nss/nss_files/files-grp.c index 42155bc..6a631ef 100644 --- a/nss/nss_files/files-grp.c +++ b/nss/nss_files/files-grp.c @@ -20,7 +20,7 @@ #define STRUCTURE group #define ENTNAME grent -#define DATABASE "group" +#define DATABASE group struct grent_data {}; /* Our parser function is already defined in fgetgrent.c, so use that. diff --git a/nss/nss_files/files-hosts.c b/nss/nss_files/files-hosts.c index 4117458..aad8d90 100644 --- a/nss/nss_files/files-hosts.c +++ b/nss/nss_files/files-hosts.c @@ -30,7 +30,7 @@ #define ENTNAME hostent -#define DATABASE "hosts" +#define DATABASE hosts #define NEED_H_ERRNO #define EXTRA_ARGS , af, flags diff --git a/nss/nss_files/files-key.c b/nss/nss_files/files-key.c index 2d64744..60fb0f4 100644 --- a/nss/nss_files/files-key.c +++ b/nss/nss_files/files-key.c @@ -23,8 +23,9 @@ #include <rpc/key_prot.h> #include <rpc/des_crypt.h> #include "nsswitch.h" +#include <nss_files.h> -#define DATAFILE "/etc/publickey" +#define DATAFILE _nss_files_config.path_publickey static enum nss_status diff --git a/nss/nss_files/files-netgrp.c b/nss/nss_files/files-netgrp.c index 8886c26..c1c3f00 100644 --- a/nss/nss_files/files-netgrp.c +++ b/nss/nss_files/files-netgrp.c @@ -26,8 +26,9 @@ #include <string.h> #include "nsswitch.h" #include "netgroup.h" +#include <nss_files.h> -#define DATAFILE "/etc/netgroup" +#define DATAFILE _nss_files_config.path_netgroup libnss_files_hidden_proto (_nss_files_endnetgrent) diff --git a/nss/nss_files/files-network.c b/nss/nss_files/files-network.c index 46e9945..d9868c4 100644 --- a/nss/nss_files/files-network.c +++ b/nss/nss_files/files-network.c @@ -22,7 +22,7 @@ #include <stdint.h> #define ENTNAME netent -#define DATABASE "networks" +#define DATABASE networks #define NEED_H_ERRNO struct netent_data {}; diff --git a/nss/nss_files/files-parse.c b/nss/nss_files/files-parse.c index 9911633..836a0c9 100644 --- a/nss/nss_files/files-parse.c +++ b/nss/nss_files/files-parse.c @@ -26,7 +26,7 @@ ENTNAME -- database name of the structure and functions (hostent, pwent). STRUCTURE -- struct name, define only if not ENTNAME (passwd, group). - DATABASE -- string of the database file's name ("hosts", "passwd"). + DATABASE -- name of the database (hosts, passwd). ENTDATA -- if defined, `struct ENTDATA' is used by the parser to store things pointed to by the resultant `struct STRUCTURE'. diff --git a/nss/nss_files/files-proto.c b/nss/nss_files/files-proto.c index 370266a..e2099ef 100644 --- a/nss/nss_files/files-proto.c +++ b/nss/nss_files/files-proto.c @@ -20,7 +20,7 @@ #define ENTNAME protoent -#define DATABASE "protocols" +#define DATABASE protocols struct protoent_data {}; diff --git a/nss/nss_files/files-pwd.c b/nss/nss_files/files-pwd.c index 84ce5bb..4936624 100644 --- a/nss/nss_files/files-pwd.c +++ b/nss/nss_files/files-pwd.c @@ -20,7 +20,7 @@ #define STRUCTURE passwd #define ENTNAME pwent -#define DATABASE "passwd" +#define DATABASE passwd struct pwent_data {}; /* Our parser function is already defined in fgetpwent_r.c, so use that diff --git a/nss/nss_files/files-rpc.c b/nss/nss_files/files-rpc.c index 34f6aec..3415ca6 100644 --- a/nss/nss_files/files-rpc.c +++ b/nss/nss_files/files-rpc.c @@ -20,7 +20,7 @@ #define ENTNAME rpcent -#define DATABASE "rpc" +#define DATABASE rpc struct rpcent_data {}; diff --git a/nss/nss_files/files-service.c b/nss/nss_files/files-service.c index 7fccbdb..0bbf046 100644 --- a/nss/nss_files/files-service.c +++ b/nss/nss_files/files-service.c @@ -21,7 +21,7 @@ #define ENTNAME servent -#define DATABASE "services" +#define DATABASE services struct servent_data {}; diff --git a/nss/nss_files/files-sgrp.c b/nss/nss_files/files-sgrp.c index ac74324..538e417 100644 --- a/nss/nss_files/files-sgrp.c +++ b/nss/nss_files/files-sgrp.c @@ -20,7 +20,7 @@ #define STRUCTURE sgrp #define ENTNAME sgent -#define DATABASE "gshadow" +#define DATABASE gshadow struct sgent_data {}; /* Our parser function is already defined in sgetspent_r.c, so use that diff --git a/nss/nss_files/files-spwd.c b/nss/nss_files/files-spwd.c index a255454..6436ee1 100644 --- a/nss/nss_files/files-spwd.c +++ b/nss/nss_files/files-spwd.c @@ -20,7 +20,7 @@ #define STRUCTURE spwd #define ENTNAME spent -#define DATABASE "shadow" +#define DATABASE shadow struct spent_data {}; /* Our parser function is already defined in sgetspent_r.c, so use that diff --git a/nss/test-data/files/netgroup b/nss/test-data/files/netgroup new file mode 100644 index 0000000..b367576 --- /dev/null +++ b/nss/test-data/files/netgroup @@ -0,0 +1,6 @@ +netgr gr1 gr2 gr3 +gr1 (,u1,) +gr2 (,u2,) +gr3 gr5 gr6 +gr5 +gr6 (,u3,) diff --git a/nss/test-data/files/passwd b/nss/test-data/files/passwd new file mode 100644 index 0000000..873152a --- /dev/null +++ b/nss/test-data/files/passwd @@ -0,0 +1 @@ +root:x:0:0:glibc root test user:/root:/bin/bash diff --git a/nss/tst-nss-files.c b/nss/tst-nss-files.c new file mode 100644 index 0000000..8d3cb75 --- /dev/null +++ b/nss/tst-nss-files.c @@ -0,0 +1,211 @@ +/* Test nss_files parsing. + 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 <dlfcn.h> +#include <errno.h> +#include <gnu/lib-names.h> +#include <netdb.h> +#include <nss_files.h> +#include <pwd.h> +#include <stdbool.h> +#include <stdio.h> +#include <stdlib.h> + +static void +configure_nss (void) +{ + __nss_configure_lookup ("passwd", "files"); +} + +static char * +file_name_for_database (const char *name) +{ + char *curdir = realpath (".", NULL); + if (curdir == NULL) + { + printf ("error: realpath (\".\", NULL): %m\n"); + exit (1); + } + + char *result; + int ret = asprintf (&result, "%s/test-data/files/%s", curdir, name); + if (ret < 0) + { + printf ("error: asprintf: %m\n"); + exit (1); + } + return result; +} + +static struct nss_files_config test_config; + +static void +init_test_config (void) +{ + test_config = (struct nss_files_config) + { + .path_ethers = file_name_for_database ("ethers"), + .path_group = file_name_for_database ("group"), + .path_gshadow = file_name_for_database ("gshadow"), + .path_hosts = file_name_for_database ("hosts"), + .path_netgroup = file_name_for_database ("netgroup"), + .path_networks = file_name_for_database ("networks"), + .path_passwd = file_name_for_database ("passwd"), + .path_protocols = file_name_for_database ("protocols"), + .path_publickey = file_name_for_database ("publickey"), + .path_rpc = file_name_for_database ("rpc"), + .path_services = file_name_for_database ("services"), + .path_shadow = file_name_for_database ("shadow"), + }; +} + +static bool errors; + +static int +sort_strings (const void *a, const void *b) +{ + return strcmp (*(const char **) a, *(const char **) b); +} + + +/* Test case from bug 17363: getnetgrent does not return the complete + list of entries if one of the netgroups in its definition tree is + empty. */ +static void +test_bug17363 (void) +{ + int ret = setnetgrent ("netgr"); + if (ret != 1) + { + printf ("error: setnetgrent (\"netgr\"): %m\n"); + errors = true; + } + +#define COUNT 3 + static const char *const expected[COUNT] = + { + ",u1,", + ",u2,", + ",u3,", + }; + + char *actual[COUNT] = {}; + + for (size_t i = 0; ; ++i) + { + char *host; + char *user; + char *domain; + ret = getnetgrent (&host, &user, &domain); + if (ret != 1) + { + if (i == COUNT) + break; + printf ("error: getnetgrent (%zu)\n", i); + errors = true; + break; + } + if (i == COUNT) + { + printf ("error: getnetgrent (%zu): unexpected success\n", i); + errors = true; + break; + } + + if (host == NULL) + host = (char *) ""; + if (user == NULL) + user = (char *) ""; + if (domain == NULL) + domain = (char *) ""; + + ret = asprintf (actual + i, "%s,%s,%s", host, user, domain); + if (ret < 0) + { + printf ("error: asprintf: %m\n"); + exit (1); + } + } + + endnetgrent (); + + if (!errors) + { + qsort (actual, COUNT, sizeof (actual[0]), sort_strings); + for (size_t i = 0; i < COUNT; ++i) + { + if (strcmp (actual[i], expected[i]) != 0) + { + printf ("error: getnetgrent (%zu): \"%s\" instead of \"%s\"\n", + i, actual[i], expected[i]); + errors = true; + } + free (actual[i]); + } + } +#undef COUNT +} + +static int +do_test (void) +{ + configure_nss (); + + void *handle = dlopen (LIBNSS_FILES_SO, RTLD_LAZY); + if (handle == NULL) + { + printf ("error: cannot open " LIBNSS_FILES_SO ": %s\n", + dlerror ()); + return 1; + } + + __typeof__ (&_nss_files_set_config) config_function = + dlsym (handle, "_nss_files_set_config"); + if (config_function == NULL) + { + printf ("error: cannot find configuration function symbol: %s\n", + dlerror ()); + return 1; + } + + init_test_config (); + config_function (&test_config); + + /* Check that the reconfiguration to the test files was + successful. */ + const struct passwd *pw = getpwuid (0); + if (pw == NULL) + { + printf ("error: root user lookup failed: %m\n"); + return 1; + } + if (strcmp (pw->pw_gecos, "glibc root test user") != 0) + { + printf ("error: incorrect GECOS field for root user: \"%s\n\"", + pw->pw_gecos); + return 1; + } + + test_bug17363 (); + dlclose (handle); + + return errors; +} + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c"