* [PATCH 1/2] nss: add assert to DB_LOOKUP_FCT (BZ #28752) @ 2022-05-26 0:20 Sam James 2022-05-26 0:20 ` [PATCH 2/2] nss: handle stat failure in check_reload_and_get " Sam James 2022-05-27 17:07 ` [PATCH 1/2] nss: add assert to DB_LOOKUP_FCT " Adhemerval Zanella 0 siblings, 2 replies; 17+ messages in thread From: Sam James @ 2022-05-26 0:20 UTC (permalink / raw) To: libc-alpha It's interesting if we have a null action list, so an assert is worthwhile. Suggested-by: DJ Delorie <dj@redhat.com> Signed-off-by: Sam James <sam@gentoo.org> --- nss/XXX-lookup.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/nss/XXX-lookup.c b/nss/XXX-lookup.c index db95937674..793dde976e 100644 --- a/nss/XXX-lookup.c +++ b/nss/XXX-lookup.c @@ -15,6 +15,7 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ +#include <assert.h> #include "nsswitch.h" /*******************************************************************\ @@ -54,6 +55,10 @@ DB_LOOKUP_FCT (nss_action_list *ni, const char *fct_name, const char *fct2_name, *ni = DATABASE_NAME_SYMBOL; + // We want to know about it if we've somehow got a NULL action list; + // in the past, we had bad state if seccomp interfered with setup. + assert(*ni != NULL); + return __nss_lookup (ni, fct_name, fct2_name, fctp); } libc_hidden_def (DB_LOOKUP_FCT) -- 2.35.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752) 2022-05-26 0:20 [PATCH 1/2] nss: add assert to DB_LOOKUP_FCT (BZ #28752) Sam James @ 2022-05-26 0:20 ` Sam James 2022-05-26 6:49 ` Sam James 2022-05-27 17:04 ` Adhemerval Zanella 2022-05-27 17:07 ` [PATCH 1/2] nss: add assert to DB_LOOKUP_FCT " Adhemerval Zanella 1 sibling, 2 replies; 17+ messages in thread From: Sam James @ 2022-05-26 0:20 UTC (permalink / raw) To: libc-alpha Skip the chroot test if the database isn't loaded correctly (because the chroot test uses some existing DB state). The __stat64_time64 -> fstatat call can fail if running under an (aggressive) seccomp filter, like Firefox seems to use. This manifested in a crash when using glib built with FAM support with such a Firefox build. Suggested-by: DJ Delorie <dj@redhat.com> Signed-off-by: Sam James <sam@gentoo.org> --- nss/nss_database.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/nss/nss_database.c b/nss/nss_database.c index d56c5b798d..6c9b440a98 100644 --- a/nss/nss_database.c +++ b/nss/nss_database.c @@ -420,21 +420,24 @@ nss_database_check_reload_and_get (struct nss_database_state *local, return true; } - /* Before we reload, verify that "/" hasn't changed. We assume that - errors here are very unlikely, but the chance that we're entering - a container is also very unlikely, so we err on the side of both - very unlikely things not happening at the same time. */ - if (__stat64_time64 ("/", &str) != 0 - || (local->root_ino != 0 - && (str.st_ino != local->root_ino - || str.st_dev != local->root_dev))) - { - /* Change detected; disable reloading and return current state. */ - atomic_store_release (&local->data.reload_disabled, 1); - *result = local->data.services[database_index]; - __libc_lock_unlock (local->lock); - return true; - } + if (local->data.services[database_index] != NULL) { + /* Before we reload, verify that "/" hasn't changed. We assume that + errors here are very unlikely, but the chance that we're entering + a container is also very unlikely, so we err on the side of both + very unlikely things not happening at the same time. */ + if (__stat64_time64 ("/", &str) != 0 + || (local->root_ino != 0 + && (str.st_ino != local->root_ino + || str.st_dev != local->root_dev))) + { + /* Change detected; disable reloading and return current state. */ + atomic_store_release (&local->data.reload_disabled, 1); + *result = local->data.services[database_index]; + __libc_lock_unlock (local->lock); + return true; + } + } + local->root_ino = str.st_ino; local->root_dev = str.st_dev; __libc_lock_unlock (local->lock); -- 2.35.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752) 2022-05-26 0:20 ` [PATCH 2/2] nss: handle stat failure in check_reload_and_get " Sam James @ 2022-05-26 6:49 ` Sam James 2022-06-04 3:26 ` DJ Delorie 2022-05-27 17:04 ` Adhemerval Zanella 1 sibling, 1 reply; 17+ messages in thread From: Sam James @ 2022-05-26 6:49 UTC (permalink / raw) To: libc-alpha [-- Attachment #1: Type: text/plain, Size: 262 bytes --] > On 26 May 2022, at 01:20, Sam James <sam@gentoo.org> wrote: > > Skip the chroot test if the database isn't loaded > correctly (because the chroot test uses some > existing DB state). It looks like there's a test failure with the trybot. I'll look into it. [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 618 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752) 2022-05-26 6:49 ` Sam James @ 2022-06-04 3:26 ` DJ Delorie 2022-06-04 3:48 ` Sam James 2022-06-04 6:30 ` Sam James 0 siblings, 2 replies; 17+ messages in thread From: DJ Delorie @ 2022-06-04 3:26 UTC (permalink / raw) To: Sam James; +Cc: libc-alpha Sam James <sam@gentoo.org> writes: >> On 26 May 2022, at 01:20, Sam James <sam@gentoo.org> wrote: >> >> Skip the chroot test if the database isn't loaded >> correctly (because the chroot test uses some >> existing DB state). > > It looks like there's a test failure with the trybot. I'll look into it. I think we need to move the local->* settings inside the new block as otherwise the str.* we're referencing are undefined. diff --git a/nss/nss_database.c b/nss/nss_database.c index d56c5b798d..72883d9b42 100644 --- a/nss/nss_database.c +++ b/nss/nss_database.c @@ -420,23 +420,26 @@ nss_database_check_reload_and_get (struct nss_database_state *local, return true; } - /* Before we reload, verify that "/" hasn't changed. We assume that - errors here are very unlikely, but the chance that we're entering - a container is also very unlikely, so we err on the side of both - very unlikely things not happening at the same time. */ - if (__stat64_time64 ("/", &str) != 0 - || (local->root_ino != 0 - && (str.st_ino != local->root_ino - || str.st_dev != local->root_dev))) - { - /* Change detected; disable reloading and return current state. */ - atomic_store_release (&local->data.reload_disabled, 1); - *result = local->data.services[database_index]; - __libc_lock_unlock (local->lock); - return true; - } - local->root_ino = str.st_ino; - local->root_dev = str.st_dev; + if (local->data.services[database_index] != NULL) { + /* Before we reload, verify that "/" hasn't changed. We assume that + errors here are very unlikely, but the chance that we're entering + a container is also very unlikely, so we err on the side of both + very unlikely things not happening at the same time. */ + if (__stat64_time64 ("/", &str) != 0 + || (local->root_ino != 0 + && (str.st_ino != local->root_ino + || str.st_dev != local->root_dev))) + { + /* Change detected; disable reloading and return current state. */ + atomic_store_release (&local->data.reload_disabled, 1); + *result = local->data.services[database_index]; + __libc_lock_unlock (local->lock); + return true; + } + local->root_ino = str.st_ino; + local->root_dev = str.st_dev; + } + __libc_lock_unlock (local->lock); /* Avoid overwriting the global configuration until we have loaded ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752) 2022-06-04 3:26 ` DJ Delorie @ 2022-06-04 3:48 ` Sam James 2022-06-04 6:30 ` Sam James 1 sibling, 0 replies; 17+ messages in thread From: Sam James @ 2022-06-04 3:48 UTC (permalink / raw) To: DJ Delorie; +Cc: libc-alpha [-- Attachment #1: Type: text/plain, Size: 567 bytes --] > On 4 Jun 2022, at 04:26, DJ Delorie via Libc-alpha <libc-alpha@sourceware.org> wrote: > > Sam James <sam@gentoo.org> writes: >>> On 26 May 2022, at 01:20, Sam James <sam@gentoo.org> wrote: >>> >>> Skip the chroot test if the database isn't loaded >>> correctly (because the chroot test uses some >>> existing DB state). >> >> It looks like there's a test failure with the trybot. I'll look into it. > > I think we need to move the local->* settings inside the new block as > otherwise the str.* we're referencing are undefined. Duh. Thanks. [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 358 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752) 2022-06-04 3:26 ` DJ Delorie 2022-06-04 3:48 ` Sam James @ 2022-06-04 6:30 ` Sam James 2022-06-04 20:51 ` DJ Delorie 1 sibling, 1 reply; 17+ messages in thread From: Sam James @ 2022-06-04 6:30 UTC (permalink / raw) To: DJ Delorie; +Cc: libc-alpha [-- Attachment #1: Type: text/plain, Size: 1612 bytes --] > On 4 Jun 2022, at 04:26, DJ Delorie <dj@redhat.com> wrote: > > Sam James <sam@gentoo.org> writes: >>> On 26 May 2022, at 01:20, Sam James <sam@gentoo.org> wrote: >>> >>> Skip the chroot test if the database isn't loaded >>> correctly (because the chroot test uses some >>> existing DB state). >> >> It looks like there's a test failure with the trybot. I'll look into it. > > I think we need to move the local->* settings inside the new block as > otherwise the str.* we're referencing are undefined. > Huh, it still fails, but makes a bit more sense. It's upset because now we apparently we return a result despite not being started up: ``` FAIL: nss/tst-reload2 original exit status 1 error: tst-reload2.c:132: not true: pw->pw_uid != 2468 tst-reload2.c:138: numeric comparison failure left: 5 (0x5); from: pw->pw_uid right: 1234 (0x4d2); from: 1234 error: tst-reload2.c:140: not true: gr != NULL error: tst-reload2.c:148: not true: he != NULL error: 4 test failures ``` If local->data.services[database_index] is null, we go ahead and reload anyway even if we're in a chroot, because we assume that it's unlikely to have both cases. But it's null .. even if we are just entering a chroot, but then we skip the test? Are we back to always wanting to do the chroot test, because otherwise we're going to accidentally end up loading from the chroot again? i.e. if it's null, we never change local->root_{into,dev}, so they continue to point into the chroot. But I can't always do a statx call because then we're back at square 1. And now I've talked myself into a corner and I'm confused :) [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 358 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752) 2022-06-04 6:30 ` Sam James @ 2022-06-04 20:51 ` DJ Delorie 2022-06-04 21:29 ` Sam James 0 siblings, 1 reply; 17+ messages in thread From: DJ Delorie @ 2022-06-04 20:51 UTC (permalink / raw) To: Sam James; +Cc: libc-alpha Sam James <sam@gentoo.org> writes: > Huh, it still fails, but makes a bit more sense. It's upset because > now we apparently we return a result despite not being started > up: So back to basics... This routine is called when we have a real getXbyY() call and need a config. Our code happens after we've checked for nsswitch.conf changing. action: FALSE = error returned to user action: TRUE = old configuration re-used action: CONT = continue to load new configuration services[db] inode.changed action NULL NO CONT - we need some config NULL YES CONT - we need some config non-null NO CONT - we need to reload non-null YES TRUE - use old config So I think the patch is correct. I'm seeing tst-reload2 fail also (sigh, only tested tst-reload1) It looks like we need to initialize local->root_* even if [services] is NULL. I.e. we need to run that stat always, despite deferring its check. This is a little messier but I think the logic is all in the right order. We might need to move the local-> if{} to before the NULL check, although we must load the config at least once so it should be OK. diff --git a/nss/nss_database.c b/nss/nss_database.c index d56c5b798d..f2ed2f2c25 100644 --- a/nss/nss_database.c +++ b/nss/nss_database.c @@ -420,23 +420,32 @@ nss_database_check_reload_and_get (struct nss_database_state *local, return true; } - /* Before we reload, verify that "/" hasn't changed. We assume that - errors here are very unlikely, but the chance that we're entering - a container is also very unlikely, so we err on the side of both - very unlikely things not happening at the same time. */ - if (__stat64_time64 ("/", &str) != 0 - || (local->root_ino != 0 - && (str.st_ino != local->root_ino - || str.st_dev != local->root_dev))) + int stat_rv = __stat64_time64 ("/", &str); + + if (local->data.services[database_index] != NULL) { - /* Change detected; disable reloading and return current state. */ - atomic_store_release (&local->data.reload_disabled, 1); - *result = local->data.services[database_index]; - __libc_lock_unlock (local->lock); - return true; + /* Before we reload, verify that "/" hasn't changed. We assume that + errors here are very unlikely, but the chance that we're entering + a container is also very unlikely, so we err on the side of both + very unlikely things not happening at the same time. */ + if (stat_rv != 0 + || (local->root_ino != 0 + && (str.st_ino != local->root_ino + || str.st_dev != local->root_dev))) + { + /* Change detected; disable reloading and return current state. */ + atomic_store_release (&local->data.reload_disabled, 1); + *result = local->data.services[database_index]; + __libc_lock_unlock (local->lock); + return true; + } + } + if (stat_rv == 0) + { + local->root_ino = str.st_ino; + local->root_dev = str.st_dev; } - local->root_ino = str.st_ino; - local->root_dev = str.st_dev; + __libc_lock_unlock (local->lock); /* Avoid overwriting the global configuration until we have loaded ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752) 2022-06-04 20:51 ` DJ Delorie @ 2022-06-04 21:29 ` Sam James 2022-06-04 22:11 ` Sam James 0 siblings, 1 reply; 17+ messages in thread From: Sam James @ 2022-06-04 21:29 UTC (permalink / raw) To: DJ Delorie; +Cc: libc-alpha [-- Attachment #1: Type: text/plain, Size: 3773 bytes --] > On 4 Jun 2022, at 21:51, DJ Delorie <dj@redhat.com> wrote: > > Sam James <sam@gentoo.org> writes: >> Huh, it still fails, but makes a bit more sense. It's upset because >> now we apparently we return a result despite not being started >> up: > > So back to basics... > > This routine is called when we have a real getXbyY() call and need > a config. > > Our code happens after we've checked for nsswitch.conf changing. > > action: FALSE = error returned to user > action: TRUE = old configuration re-used > action: CONT = continue to load new configuration > > services[db] inode.changed action > > NULL NO CONT - we need some config > NULL YES CONT - we need some config > non-null NO CONT - we need to reload > non-null YES TRUE - use old config > > So I think the patch is correct. I'm seeing tst-reload2 fail also > (sigh, only tested tst-reload1) > > It looks like we need to initialize local->root_* even if [services] is > NULL. I.e. we need to run that stat always, despite deferring its > check. > > This is a little messier but I think the logic is all in the right > order. We might need to move the local-> if{} to before the NULL check, > although we must load the config at least once so it should be OK. > I'm glad I nearly got there -- I was worried about making the stat call at all, but the issue is referencing the result, of course, if it's not safe to do so. Let me give this a whack! > diff --git a/nss/nss_database.c b/nss/nss_database.c > index d56c5b798d..f2ed2f2c25 100644 > --- a/nss/nss_database.c > +++ b/nss/nss_database.c > @@ -420,23 +420,32 @@ nss_database_check_reload_and_get (struct nss_database_state *local, > return true; > } > > - /* Before we reload, verify that "/" hasn't changed. We assume that > - errors here are very unlikely, but the chance that we're entering > - a container is also very unlikely, so we err on the side of both > - very unlikely things not happening at the same time. */ > - if (__stat64_time64 ("/", &str) != 0 > - || (local->root_ino != 0 > - && (str.st_ino != local->root_ino > - || str.st_dev != local->root_dev))) > + int stat_rv = __stat64_time64 ("/", &str); > + > + if (local->data.services[database_index] != NULL) > { > - /* Change detected; disable reloading and return current state. */ > - atomic_store_release (&local->data.reload_disabled, 1); > - *result = local->data.services[database_index]; > - __libc_lock_unlock (local->lock); > - return true; > + /* Before we reload, verify that "/" hasn't changed. We assume that > + errors here are very unlikely, but the chance that we're entering > + a container is also very unlikely, so we err on the side of both > + very unlikely things not happening at the same time. */ > + if (stat_rv != 0 > + || (local->root_ino != 0 > + && (str.st_ino != local->root_ino > + || str.st_dev != local->root_dev))) > + { > + /* Change detected; disable reloading and return current state. */ > + atomic_store_release (&local->data.reload_disabled, 1); > + *result = local->data.services[database_index]; > + __libc_lock_unlock (local->lock); > + return true; > + } > + } > + if (stat_rv == 0) > + { > + local->root_ino = str.st_ino; > + local->root_dev = str.st_dev; > } > - local->root_ino = str.st_ino; > - local->root_dev = str.st_dev; > + > __libc_lock_unlock (local->lock); > > /* Avoid overwriting the global configuration until we have loaded > > Best, sam [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 358 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752) 2022-06-04 21:29 ` Sam James @ 2022-06-04 22:11 ` Sam James 2022-06-04 22:13 ` Sam James 2022-06-05 0:55 ` DJ Delorie 0 siblings, 2 replies; 17+ messages in thread From: Sam James @ 2022-06-04 22:11 UTC (permalink / raw) To: Sam James; +Cc: DJ Delorie, libc-alpha [-- Attachment #1: Type: text/plain, Size: 2785 bytes --] > On 4 Jun 2022, at 22:29, Sam James via Libc-alpha <libc-alpha@sourceware.org> wrote: > > > >> On 4 Jun 2022, at 21:51, DJ Delorie <dj@redhat.com> wrote: >> >> Sam James <sam@gentoo.org> writes: >>> Huh, it still fails, but makes a bit more sense. It's upset because >>> now we apparently we return a result despite not being started >>> up: >> >> So back to basics... >> >> This routine is called when we have a real getXbyY() call and need >> a config. >> >> Our code happens after we've checked for nsswitch.conf changing. >> >> action: FALSE = error returned to user >> action: TRUE = old configuration re-used >> action: CONT = continue to load new configuration >> >> services[db] inode.changed action >> >> NULL NO CONT - we need some config >> NULL YES CONT - we need some config >> non-null NO CONT - we need to reload >> non-null YES TRUE - use old config >> >> So I think the patch is correct. I'm seeing tst-reload2 fail also >> (sigh, only tested tst-reload1) >> >> It looks like we need to initialize local->root_* even if [services] is >> NULL. I.e. we need to run that stat always, despite deferring its >> check. >> >> This is a little messier but I think the logic is all in the right >> order. We might need to move the local-> if{} to before the NULL check, >> although we must load the config at least once so it should be OK. >> > > I'm glad I nearly got there -- I was worried about making the stat > call at all, but the issue is referencing the result, of course, if > it's not safe to do so. > > Let me give this a whack! >> Okay, it still fails (I put the check before and after, no difference, although I think after makes more sense), but bear with me: ``` FAIL: nss/tst-reload2 original exit status 1 error: tst-reload2.c:132: not true: pw->pw_uid != 2468 tst-reload2.c:138: numeric comparison failure left: 5 (0x5); from: pw->pw_uid right: 1234 (0x4d2); from: 1234 error: tst-reload2.c:140: not true: gr != NULL error: tst-reload2.c:148: not true: he != NULL error: 4 test failures make[1]: Leaving directory '/home/sam/git/glibc' ``` Are we sure tst-reload2 is correct? At line 140, shouldn't it be null (I mean, it is when the test fails, but isn't that okay?), because we don't want to load the inner config? Especially given on line 142, we want the getgrnam() call to return null. And Group ID 5 is only defined in group_table_data2. Now, where I'm less sure is the failure on line 148 (gethostbyname). test2 is in subdir/etc/nsswitch.conf and nothing about test2 is available in the outer configuration. *BUT* the comment above it says "... can still load the files DSO.", so I'm less confident I'm understanding that one. > > Best, > sam [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 358 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752) 2022-06-04 22:11 ` Sam James @ 2022-06-04 22:13 ` Sam James 2022-06-05 0:55 ` DJ Delorie 1 sibling, 0 replies; 17+ messages in thread From: Sam James @ 2022-06-04 22:13 UTC (permalink / raw) To: Sam James; +Cc: DJ Delorie, libc-alpha [-- Attachment #1: Type: text/plain, Size: 3088 bytes --] > On 4 Jun 2022, at 23:11, Sam James <sam@gentoo.org> wrote: > > > >> On 4 Jun 2022, at 22:29, Sam James via Libc-alpha <libc-alpha@sourceware.org> wrote: >> >> >> >>> On 4 Jun 2022, at 21:51, DJ Delorie <dj@redhat.com> wrote: >>> >>> Sam James <sam@gentoo.org> writes: >>>> Huh, it still fails, but makes a bit more sense. It's upset because >>>> now we apparently we return a result despite not being started >>>> up: >>> >>> So back to basics... >>> >>> This routine is called when we have a real getXbyY() call and need >>> a config. >>> >>> Our code happens after we've checked for nsswitch.conf changing. >>> >>> action: FALSE = error returned to user >>> action: TRUE = old configuration re-used >>> action: CONT = continue to load new configuration >>> >>> services[db] inode.changed action >>> >>> NULL NO CONT - we need some config >>> NULL YES CONT - we need some config >>> non-null NO CONT - we need to reload >>> non-null YES TRUE - use old config >>> >>> So I think the patch is correct. I'm seeing tst-reload2 fail also >>> (sigh, only tested tst-reload1) >>> >>> It looks like we need to initialize local->root_* even if [services] is >>> NULL. I.e. we need to run that stat always, despite deferring its >>> check. >>> >>> This is a little messier but I think the logic is all in the right >>> order. We might need to move the local-> if{} to before the NULL check, >>> although we must load the config at least once so it should be OK. >>> >> >> I'm glad I nearly got there -- I was worried about making the stat >> call at all, but the issue is referencing the result, of course, if >> it's not safe to do so. >> >> Let me give this a whack! >>> > > Okay, it still fails (I put the check before and after, no difference, although I think after makes more sense), but bear with me: > ``` > FAIL: nss/tst-reload2 > original exit status 1 > error: tst-reload2.c:132: not true: pw->pw_uid != 2468 > tst-reload2.c:138: numeric comparison failure > left: 5 (0x5); from: pw->pw_uid > right: 1234 (0x4d2); from: 1234 > error: tst-reload2.c:140: not true: gr != NULL > error: tst-reload2.c:148: not true: he != NULL > error: 4 test failures > make[1]: Leaving directory '/home/sam/git/glibc' > ``` > > Are we sure tst-reload2 is correct? > > At line 140, shouldn't it be null (I mean, it is when the test fails, but isn't that okay?), because we don't want to load the inner config? > > Especially given on line 142, we want the getgrnam() call to return null. And Group ID 5 is only defined in group_table_data2. > > Now, where I'm less sure is the failure on line 148 (gethostbyname). test2 is in subdir/etc/nsswitch.conf and nothing about test2 is available in the outer configuration. *BUT* the comment above it says "... can still load the files DSO.", so I'm less confident I'm understanding that one. > (This doesn't address all of the failures, so it might be I'm just misunderstanding, but line 140 vs line 142 definitely seems odd?) >> >> Best, >> sam [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 358 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752) 2022-06-04 22:11 ` Sam James 2022-06-04 22:13 ` Sam James @ 2022-06-05 0:55 ` DJ Delorie 2022-06-05 3:56 ` Sam James 2022-06-05 6:23 ` Andreas Schwab 1 sibling, 2 replies; 17+ messages in thread From: DJ Delorie @ 2022-06-05 0:55 UTC (permalink / raw) To: Sam James; +Cc: libc-alpha Weird, it works for me, both reload1 and reload2 inside and outside of the trybot's build container. Note: the testroot is NOT automatically updated each time you type "make", because that's a lot of overhead that's rarely needed. If you modify the sources and want to see the results in containerized tests: $ rm -rf $BUILD/testroot.* $ make -j $BUILD/testroot.pristine/install.stamp ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752) 2022-06-05 0:55 ` DJ Delorie @ 2022-06-05 3:56 ` Sam James 2022-06-05 6:23 ` Andreas Schwab 1 sibling, 0 replies; 17+ messages in thread From: Sam James @ 2022-06-05 3:56 UTC (permalink / raw) To: DJ Delorie; +Cc: libc-alpha [-- Attachment #1: Type: text/plain, Size: 635 bytes --] > On 5 Jun 2022, at 01:55, DJ Delorie <dj@redhat.com> wrote: > > > Weird, it works for me, both reload1 and reload2 inside and outside of > the trybot's build container. > > Note: the testroot is NOT automatically updated each time you type > "make", because that's a lot of overhead that's rarely needed. If you > modify the sources and want to see the results in containerized tests: > > $ rm -rf $BUILD/testroot.* > $ make -j $BUILD/testroot.pristine/install.stamp > Ah, thanks, that did it! I hadn't realised about the testroot part. (Then got bit by https://sourceware.org/bugzilla/show_bug.cgi?id=25836). Passing! \o/ [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 358 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752) 2022-06-05 0:55 ` DJ Delorie 2022-06-05 3:56 ` Sam James @ 2022-06-05 6:23 ` Andreas Schwab 2022-06-06 16:14 ` DJ Delorie 1 sibling, 1 reply; 17+ messages in thread From: Andreas Schwab @ 2022-06-05 6:23 UTC (permalink / raw) To: DJ Delorie via Libc-alpha On Jun 04 2022, DJ Delorie via Libc-alpha wrote: > $ rm -rf $BUILD/testroot.* > $ make -j $BUILD/testroot.pristine/install.stamp There should be a makefile target to do that. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different." ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752) 2022-06-05 6:23 ` Andreas Schwab @ 2022-06-06 16:14 ` DJ Delorie 0 siblings, 0 replies; 17+ messages in thread From: DJ Delorie @ 2022-06-06 16:14 UTC (permalink / raw) To: Andreas Schwab; +Cc: libc-alpha, sam Andreas Schwab <schwab@linux-m68k.org> writes: >> $ rm -rf $BUILD/testroot.* >> $ make -j $BUILD/testroot.pristine/install.stamp > > There should be a makefile target to do that. I'm not opposed, but: 1. Bikeshed a clever name, and 2. How would anyone learn of this new option when they need it? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] nss: handle stat failure in check_reload_and_get (BZ #28752) 2022-05-26 0:20 ` [PATCH 2/2] nss: handle stat failure in check_reload_and_get " Sam James 2022-05-26 6:49 ` Sam James @ 2022-05-27 17:04 ` Adhemerval Zanella 1 sibling, 0 replies; 17+ messages in thread From: Adhemerval Zanella @ 2022-05-27 17:04 UTC (permalink / raw) To: libc-alpha On 25/05/2022 21:20, Sam James via Libc-alpha wrote: > Skip the chroot test if the database isn't loaded > correctly (because the chroot test uses some > existing DB state). > > The __stat64_time64 -> fstatat call can fail if > running under an (aggressive) seccomp filter, > like Firefox seems to use. > > This manifested in a crash when using glib built > with FAM support with such a Firefox build. > > Suggested-by: DJ Delorie <dj@redhat.com> > Signed-off-by: Sam James <sam@gentoo.org> > --- > nss/nss_database.c | 33 ++++++++++++++++++--------------- > 1 file changed, 18 insertions(+), 15 deletions(-) > > diff --git a/nss/nss_database.c b/nss/nss_database.c > index d56c5b798d..6c9b440a98 100644 > --- a/nss/nss_database.c > +++ b/nss/nss_database.c > @@ -420,21 +420,24 @@ nss_database_check_reload_and_get (struct nss_database_state *local, > return true; > } > > - /* Before we reload, verify that "/" hasn't changed. We assume that > - errors here are very unlikely, but the chance that we're entering > - a container is also very unlikely, so we err on the side of both > - very unlikely things not happening at the same time. */ > - if (__stat64_time64 ("/", &str) != 0 > - || (local->root_ino != 0 > - && (str.st_ino != local->root_ino > - || str.st_dev != local->root_dev))) > - { > - /* Change detected; disable reloading and return current state. */ > - atomic_store_release (&local->data.reload_disabled, 1); > - *result = local->data.services[database_index]; > - __libc_lock_unlock (local->lock); > - return true; > - } > + if (local->data.services[database_index] != NULL) { > + /* Before we reload, verify that "/" hasn't changed. We assume that > + errors here are very unlikely, but the chance that we're entering > + a container is also very unlikely, so we err on the side of both > + very unlikely things not happening at the same time. */ > + if (__stat64_time64 ("/", &str) != 0 > + || (local->root_ino != 0 > + && (str.st_ino != local->root_ino > + || str.st_dev != local->root_dev))) > + { > + /* Change detected; disable reloading and return current state. */ > + atomic_store_release (&local->data.reload_disabled, 1); > + *result = local->data.services[database_index]; > + __libc_lock_unlock (local->lock); > + return true; > + } > + } > + > local->root_ino = str.st_ino; > local->root_dev = str.st_dev; > __libc_lock_unlock (local->lock); Besides the buildbot issue [1] you already noted, please use the expected GNU indentation. We have now a clang-format script (.clang-format) to help the format. [1] https://www.delorie.com/trybots/32bit/9696/nss-tst-reload1.out ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] nss: add assert to DB_LOOKUP_FCT (BZ #28752) 2022-05-26 0:20 [PATCH 1/2] nss: add assert to DB_LOOKUP_FCT (BZ #28752) Sam James 2022-05-26 0:20 ` [PATCH 2/2] nss: handle stat failure in check_reload_and_get " Sam James @ 2022-05-27 17:07 ` Adhemerval Zanella 2022-06-04 3:48 ` Sam James 1 sibling, 1 reply; 17+ messages in thread From: Adhemerval Zanella @ 2022-05-27 17:07 UTC (permalink / raw) To: Sam James, libc-alpha On 25/05/2022 21:20, Sam James via Libc-alpha wrote: > It's interesting if we have a null action list, > so an assert is worthwhile. > > Suggested-by: DJ Delorie <dj@redhat.com> > Signed-off-by: Sam James <sam@gentoo.org> > --- > nss/XXX-lookup.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/nss/XXX-lookup.c b/nss/XXX-lookup.c > index db95937674..793dde976e 100644 > --- a/nss/XXX-lookup.c > +++ b/nss/XXX-lookup.c > @@ -15,6 +15,7 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > +#include <assert.h> > #include "nsswitch.h" > > /*******************************************************************\ > @@ -54,6 +55,10 @@ DB_LOOKUP_FCT (nss_action_list *ni, const char *fct_name, const char *fct2_name, > > *ni = DATABASE_NAME_SYMBOL; > > + // We want to know about it if we've somehow got a NULL action list; > + // in the past, we had bad state if seccomp interfered with setup. I think the expected comment format is the usual C90 one (/* ... */). > + assert(*ni != NULL); > + > return __nss_lookup (ni, fct_name, fct2_name, fctp); > } > libc_hidden_def (DB_LOOKUP_FCT) Are you trying to assure that DATABASE_NAME_SYMBOL is not NULL here? If so I think it would be simpler to just use a _Static_assert. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] nss: add assert to DB_LOOKUP_FCT (BZ #28752) 2022-05-27 17:07 ` [PATCH 1/2] nss: add assert to DB_LOOKUP_FCT " Adhemerval Zanella @ 2022-06-04 3:48 ` Sam James 0 siblings, 0 replies; 17+ messages in thread From: Sam James @ 2022-06-04 3:48 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: libc-alpha [-- Attachment #1: Type: text/plain, Size: 1733 bytes --] > On 27 May 2022, at 18:07, Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> wrote: > > > > On 25/05/2022 21:20, Sam James via Libc-alpha wrote: >> It's interesting if we have a null action list, >> so an assert is worthwhile. >> >> Suggested-by: DJ Delorie <dj@redhat.com> >> Signed-off-by: Sam James <sam@gentoo.org> >> --- >> nss/XXX-lookup.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/nss/XXX-lookup.c b/nss/XXX-lookup.c >> index db95937674..793dde976e 100644 >> --- a/nss/XXX-lookup.c >> +++ b/nss/XXX-lookup.c >> @@ -15,6 +15,7 @@ >> License along with the GNU C Library; if not, see >> <https://www.gnu.org/licenses/>. */ >> >> +#include <assert.h> >> #include "nsswitch.h" >> >> /*******************************************************************\ >> @@ -54,6 +55,10 @@ DB_LOOKUP_FCT (nss_action_list *ni, const char *fct_name, const char *fct2_name, >> >> *ni = DATABASE_NAME_SYMBOL; >> >> + // We want to know about it if we've somehow got a NULL action list; >> + // in the past, we had bad state if seccomp interfered with setup. > > I think the expected comment format is the usual C90 one (/* ... */). Right. > >> + assert(*ni != NULL); >> + >> return __nss_lookup (ni, fct_name, fct2_name, fctp); >> } >> libc_hidden_def (DB_LOOKUP_FCT) > > Are you trying to assure that DATABASE_NAME_SYMBOL is not NULL here? If > so I think it would be simpler to just use a _Static_assert. I'm sorry, I'm not sure what the expression ought to be in that case. I get that some of it is figured out at build-time given we're using some macros, but I don't see what to test against to check it's not NULL statically? Best, sam [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 358 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-06-06 16:14 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-26 0:20 [PATCH 1/2] nss: add assert to DB_LOOKUP_FCT (BZ #28752) Sam James 2022-05-26 0:20 ` [PATCH 2/2] nss: handle stat failure in check_reload_and_get " Sam James 2022-05-26 6:49 ` Sam James 2022-06-04 3:26 ` DJ Delorie 2022-06-04 3:48 ` Sam James 2022-06-04 6:30 ` Sam James 2022-06-04 20:51 ` DJ Delorie 2022-06-04 21:29 ` Sam James 2022-06-04 22:11 ` Sam James 2022-06-04 22:13 ` Sam James 2022-06-05 0:55 ` DJ Delorie 2022-06-05 3:56 ` Sam James 2022-06-05 6:23 ` Andreas Schwab 2022-06-06 16:14 ` DJ Delorie 2022-05-27 17:04 ` Adhemerval Zanella 2022-05-27 17:07 ` [PATCH 1/2] nss: add assert to DB_LOOKUP_FCT " Adhemerval Zanella 2022-06-04 3:48 ` Sam James
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).