| Message ID | 20220526002045.1872855-1-sam@gentoo.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] nss: add assert to DB_LOOKUP_FCT (BZ #28752) | expand |
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.
> 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
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)
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(+)