On Sat, 16 Oct 2021 at 20:43, Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > On Fri, Oct 15, 2021 at 2:31 PM Christian Göttsche > <cgzones@xxxxxxxxxxxxxx> wrote: > > > > Glibc 2.34 added an access function attribute to pthread_setspecific(3). > > This leads to the following GCC warnings: > > > > In file included from matchpathcon.c:5: > > matchpathcon.c: In function ‘matchpathcon_init_prefix’: > > selinux_internal.h:38:25: error: ‘pthread_setspecific’ expecting 1 byte in a region of size 0 [-Werror=stringop-overread] > > 38 | pthread_setspecific(KEY, VALUE); \ > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > matchpathcon.c:359:9: note: in expansion of macro ‘__selinux_setspecific’ > > 359 | __selinux_setspecific(destructor_key, (void *)1); > > | ^~~~~~~~~~~~~~~~~~~~~ > > In file included from selinux_internal.h:2, > > from matchpathcon.c:5: > > /usr/include/pthread.h:1167:12: note: in a call to function ‘pthread_setspecific’ declared with attribute ‘access (none, 2)’ > > 1167 | extern int pthread_setspecific (pthread_key_t __key, > > | ^~~~~~~~~~~~~~~~~~~ > > > > The actual value and the validity of the passed pointer is irrelevant, > > since it does not gets accessed internally by glibc and > > pthread_getspecific(3) is not used. > > Use a pointer to a (temporary) valid object to please GCC. > > > > Closes: https://github.com/SELinuxProject/selinux/issues/311 > > Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx> > > --- > > libselinux/src/matchpathcon.c | 4 +++- > > libselinux/src/procattr.c | 4 +++- > > libselinux/src/setrans_client.c | 4 +++- > > 3 files changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/libselinux/src/matchpathcon.c b/libselinux/src/matchpathcon.c > > index 1e7f8890..b0ec1e8b 100644 > > --- a/libselinux/src/matchpathcon.c > > +++ b/libselinux/src/matchpathcon.c > > @@ -352,11 +352,13 @@ static void matchpathcon_init_once(void) > > > > int matchpathcon_init_prefix(const char *path, const char *subset) > > { > > + int dummy; > > + > > if (!mycanoncon) > > mycanoncon = default_canoncon; > > > > __selinux_once(once, matchpathcon_init_once); > > - __selinux_setspecific(destructor_key, (void *)1); > > + __selinux_setspecific(destructor_key, &dummy); > > > > options[SELABEL_OPT_SUBSET].type = SELABEL_OPT_SUBSET; > > options[SELABEL_OPT_SUBSET].value = subset; > > diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c > > index 6552ee01..24cee323 100644 > > --- a/libselinux/src/procattr.c > > +++ b/libselinux/src/procattr.c > > @@ -68,7 +68,9 @@ void __attribute__((destructor)) procattr_destructor(void) > > static inline void init_thread_destructor(void) > > { > > if (destructor_initialized == 0) { > > - __selinux_setspecific(destructor_key, (void *)1); > > + int dummy; > > + > > + __selinux_setspecific(destructor_key, &dummy); > > destructor_initialized = 1; > > } > > } > > diff --git a/libselinux/src/setrans_client.c b/libselinux/src/setrans_client.c > > index 52a8ba78..515d2d4d 100644 > > --- a/libselinux/src/setrans_client.c > > +++ b/libselinux/src/setrans_client.c > > @@ -272,7 +272,9 @@ static inline void init_thread_destructor(void) > > if (!has_setrans) > > return; > > if (destructor_initialized == 0) { > > - __selinux_setspecific(destructor_key, (void *)1); > > + int dummy; > > + > > + __selinux_setspecific(destructor_key, &dummy); > > destructor_initialized = 1; > > } > > } > > -- > > 2.33.0 > > > > Hello, > Thanks for working on this. While this patch fixes the precise > warning, I fear that future improvements on the analysis performed by > compilers (or by static analysis tools) will lead to future warnings, > as we are still abusing the pthread_setspecific interface after your > patch: "dummy" is not initialized (so a tool could report a "use of > uninitialized variable"), and is destroyed after the scope of the > functions (so a tool could report a "use after free" or a "use of a > stack pointer beyond its scope"). > > These issues can be fixed by making dummy "static char dummy;" for > example. What do you think? > I don't quite like introducing 3 static variables, although their footprint is probably negligible. What about using the address of a global variable, e.g.: __selinux_setspecific(destructor_key, /* use some valid address to please GCC */ &selinux_mnt); > Thanks, > Nicolas >
