[PATCH 0/3] Portability improvements

[PATCH 0/3] Portability improvements

[Patrick Steinhardt]

Hi,

This is a small batch of portability fixes. It removes calls to
the non-portable getpwent_r function, problems with hardened
toolchains defining FORTIFY_SOURCE and removes usage of
__BEGIN_DECLS. These patches are all aiming for compatibility
with musl on hardened Gentoo.

While I guess that the first two commits of removing
__BEGIN_DECLS and the patch for FORTIFY_SOURCE are
uncontroversial, I can imagine the removal of getpwent_r being a
bit more so due to the alternative being non-reentrant. But to
the best of my knowledge the code in question is not called in a
threaded context, so I actually doubt it matters much. Given that
I'm new to this code base I could very likely be wrong, though.

Patrick


Patrick Steinhardt (3):
  libsepol: replace non-standard use of __BEGIN_DECLS
  libselinux: fix error when FORTIFY_SOURCE is already set
  genhomedircon: avoid use of non-standard `getpwent_r`

 libselinux/src/Makefile                   |  3 ++-
 libselinux/utils/Makefile                 |  3 ++-
 libsemanage/src/genhomedircon.c           | 22 +++++-----------------
 libsepol/include/sepol/ibendport_record.h | 10 +++++++---
 libsepol/include/sepol/ibendports.h       | 11 ++++++++---
 libsepol/include/sepol/ibpkey_record.h    | 11 ++++++++---
 libsepol/include/sepol/ibpkeys.h          | 12 +++++++++---
 7 files changed, 41 insertions(+), 31 deletions(-)

-- 
2.13.1

[PATCH 1/3] libsepol: replace non-standard use of __BEGIN_DECLS

[Patrick Steinhardt]

While most header files already use the common pattern of `extern "C"`
declarations to enable compiling in a C++ project, some header files in
libsepol instead use the macros `__BEGIN_DECLS` and `__END_DECLS`. These
macros are defined in the "sys/cdefs.h" header file, which provides
some non-standard extensions for glibc.

Convert usage of these declarations with the standard `extern "C"`
pattern. This improves compatibility with other libc implementations,
e.g. musl libc.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 libsepol/include/sepol/ibendport_record.h | 10 +++++++---
 libsepol/include/sepol/ibendports.h       | 11 ++++++++---
 libsepol/include/sepol/ibpkey_record.h    | 11 ++++++++---
 libsepol/include/sepol/ibpkeys.h          | 12 +++++++++---
 4 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/libsepol/include/sepol/ibendport_record.h b/libsepol/include/sepol/ibendport_record.h
index e30b252d..2a37ec63 100644
--- a/libsepol/include/sepol/ibendport_record.h
+++ b/libsepol/include/sepol/ibendport_record.h
@@ -4,9 +4,10 @@
 #include <stddef.h>
 #include <sepol/context_record.h>
 #include <sepol/handle.h>
-#include <sys/cdefs.h>
 
-__BEGIN_DECLS
+#ifdef __cplusplus
+extern "C" {
+#endif
 
 struct sepol_ibendport;
 struct sepol_ibendport_key;
@@ -64,5 +65,8 @@ extern int sepol_ibendport_clone(sepol_handle_t *handle,
 
 extern void sepol_ibendport_free(sepol_ibendport_t *ibendport);
 
-__END_DECLS
+#ifdef __cplusplus
+}
+#endif
+
 #endif
diff --git a/libsepol/include/sepol/ibendports.h b/libsepol/include/sepol/ibendports.h
index 4a89e0ca..4ad77a12 100644
--- a/libsepol/include/sepol/ibendports.h
+++ b/libsepol/include/sepol/ibendports.h
@@ -4,9 +4,10 @@
 #include <sepol/handle.h>
 #include <sepol/policydb.h>
 #include <sepol/ibendport_record.h>
-#include <sys/cdefs.h>
 
-__BEGIN_DECLS
+#ifdef __cplusplus
+extern "C" {
+#endif
 
 /* Return the number of ibendports */
 extern int sepol_ibendport_count(sepol_handle_t *handle,
@@ -41,5 +42,9 @@ extern int sepol_ibendport_iterate(sepol_handle_t *handle,
 				   int (*fn)(const sepol_ibendport_t *ibendport,
 					     void *fn_arg), void *arg);
 
-__END_DECLS
+
+#ifdef __cplusplus
+}
+#endif
+
 #endif
diff --git a/libsepol/include/sepol/ibpkey_record.h b/libsepol/include/sepol/ibpkey_record.h
index ab68147c..1511785d 100644
--- a/libsepol/include/sepol/ibpkey_record.h
+++ b/libsepol/include/sepol/ibpkey_record.h
@@ -5,11 +5,12 @@
 #include <stdint.h>
 #include <sepol/context_record.h>
 #include <sepol/handle.h>
-#include <sys/cdefs.h>
 
 #define INET6_ADDRLEN 16
 
-__BEGIN_DECLS
+#ifdef __cplusplus
+extern "C" {
+#endif
 
 struct sepol_ibpkey;
 struct sepol_ibpkey_key;
@@ -71,5 +72,9 @@ extern int sepol_ibpkey_clone(sepol_handle_t *handle,
 
 extern void sepol_ibpkey_free(sepol_ibpkey_t *ibpkey);
 
-__END_DECLS
+
+#ifdef __cplusplus
+}
+#endif
+
 #endif
diff --git a/libsepol/include/sepol/ibpkeys.h b/libsepol/include/sepol/ibpkeys.h
index 4ab0a8a6..4b69d1e9 100644
--- a/libsepol/include/sepol/ibpkeys.h
+++ b/libsepol/include/sepol/ibpkeys.h
@@ -4,9 +4,11 @@
 #include <sepol/handle.h>
 #include <sepol/policydb.h>
 #include <sepol/ibpkey_record.h>
-#include <sys/cdefs.h>
 
-__BEGIN_DECLS
+
+#ifdef __cplusplus
+extern "C" {
+#endif
 
 /* Return the number of ibpkeys */
 extern int sepol_ibpkey_count(sepol_handle_t *handle,
@@ -40,5 +42,9 @@ extern int sepol_ibpkey_iterate(sepol_handle_t *handle,
 				int (*fn)(const sepol_ibpkey_t *ibpkey,
 					  void *fn_arg), void *arg);
 
-__END_DECLS
+
+#ifdef __cplusplus
+}
+#endif
+
 #endif
-- 
2.13.1

[PATCH 2/3] libselinux: fix error when FORTIFY_SOURCE is already set

[Patrick Steinhardt]

Two makefiles of ours pass `-D_FORTIFY_SOURCE` directly to the
preprocessor. While this does not pose any problems when the value has
not been previously set, it can break the build if it is part of the
standard build flags.

Fix the issue by undefining the flag with `-Wp,-U_FORTIFY_SOURCE` right
before redefining the value. This fixes builds with some hardened
compiler chains.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 libselinux/src/Makefile   | 3 ++-
 libselinux/utils/Makefile | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
index 4306dd0e..010b7ffe 100644
--- a/libselinux/src/Makefile
+++ b/libselinux/src/Makefile
@@ -59,7 +59,8 @@ ifeq ($(COMPILER), gcc)
 EXTRA_CFLAGS = -fipa-pure-const -Wlogical-op -Wpacked-bitfield-compat -Wsync-nand \
 	-Wcoverage-mismatch -Wcpp -Wformat-contains-nul -Wnormalized=nfc -Wsuggest-attribute=const \
 	-Wsuggest-attribute=noreturn -Wsuggest-attribute=pure -Wtrampolines -Wjump-misses-init \
-	-Wno-suggest-attribute=pure -Wno-suggest-attribute=const -Wp,-D_FORTIFY_SOURCE=2
+	-Wno-suggest-attribute=pure -Wno-suggest-attribute=const \
+	-Wp,-U_FORTIFY_SOURCE -Wp,-D_FORTIFY_SOURCE=2
 else
 EXTRA_CFLAGS = -Wunused-command-line-argument
 endif
diff --git a/libselinux/utils/Makefile b/libselinux/utils/Makefile
index 474ee95b..c94d7cf2 100644
--- a/libselinux/utils/Makefile
+++ b/libselinux/utils/Makefile
@@ -32,7 +32,8 @@ CFLAGS ?= -O -Wall -W -Wundef -Wformat-y2k -Wformat-security -Winit-self -Wmissi
           -Wformat-extra-args -Wformat-zero-length -Wformat=2 -Wmultichar \
           -Woverflow -Wpointer-to-int-cast -Wpragmas \
           -Wno-missing-field-initializers -Wno-sign-compare \
-          -Wno-format-nonliteral -Wframe-larger-than=$(MAX_STACK_SIZE) -Wp,-D_FORTIFY_SOURCE=2 \
+          -Wno-format-nonliteral -Wframe-larger-than=$(MAX_STACK_SIZE) \
+          -Wp,-U_FORTIFY_SOURCE -Wp,-D_FORTIFY_SOURCE=2 \
           -fstack-protector-all --param=ssp-buffer-size=4 -fexceptions \
           -fasynchronous-unwind-tables -fdiagnostics-show-option -funit-at-a-time \
           -Werror -Wno-aggregate-return -Wno-redundant-decls \
-- 
2.13.1

[PATCH 3/3] genhomedircon: avoid use of non-standard `getpwent_r`

[Patrick Steinhardt]

The `getpwent_r` function is a non-standard but re-entrant version of
the sdardardized `getpwent` function. Next to the benefit of being
re-entrant, though, it avoids compilation with libc implementations that
do not provide this glibc-specific extension, for example musl libc.

As the code is usually not run in a threaded environment, it is save to
use the non-reentrant version, though. As such, convert code to use
`getpwent` instead to fix building with other libc implementations.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 libsemanage/src/genhomedircon.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
index e8c95ee4..f58c17ce 100644
--- a/libsemanage/src/genhomedircon.c
+++ b/libsemanage/src/genhomedircon.c
@@ -295,9 +295,8 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 	long rbuflen;
 	uid_t temp, minuid = 500, maxuid = 60000;
 	int minuid_set = 0;
-	struct passwd pwstorage, *pwbuf;
+	struct passwd *pwbuf;
 	struct stat buf;
-	int retval;
 
 	path = semanage_findval(PATH_ETC_USERADD, "HOME", "=");
 	if (path && *path) {
@@ -369,7 +368,7 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 	if (rbuf == NULL)
 		goto fail;
 	setpwent();
-	while ((retval = getpwent_r(&pwstorage, rbuf, rbuflen, &pwbuf)) == 0) {
+	while ((pwbuf = getpwent()) != NULL) {
 		if (pwbuf->pw_uid < minuid || pwbuf->pw_uid > maxuid)
 			continue;
 		if (!semanage_list_find(shells, pwbuf->pw_shell))
@@ -413,7 +412,7 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 		path = NULL;
 	}
 
-	if (retval && retval != ENOENT) {
+	if (errno && errno != ENOENT) {
 		WARN(s->h_semanage, "Error while fetching users.  "
 		     "Returning list so far.");
 	}
@@ -1064,10 +1063,7 @@ static int get_group_users(genhomedircon_settings_t * s,
 	long grbuflen;
 	char *grbuf = NULL;
 	struct group grstorage, *group = NULL;
-
-	long prbuflen;
-	char *pwbuf = NULL;
-	struct passwd pwstorage, *pw = NULL;
+	struct passwd *pw = NULL;
 
 	grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX);
 	if (grbuflen <= 0)
@@ -1104,15 +1100,8 @@ static int get_group_users(genhomedircon_settings_t * s,
 		}
 	}
 
-	prbuflen = sysconf(_SC_GETPW_R_SIZE_MAX);
-	if (prbuflen <= 0)
-		goto cleanup;
-	pwbuf = malloc(prbuflen);
-	if (pwbuf == NULL)
-		goto cleanup;
-
 	setpwent();
-	while ((retval = getpwent_r(&pwstorage, pwbuf, prbuflen, &pw)) == 0) {
+	while ((pw = getpwent()) != NULL) {
 		// skip users who also have this group as their
 		// primary group
 		if (lfind(pw->pw_name, group->gr_mem, &nmembers,
@@ -1131,7 +1120,6 @@ static int get_group_users(genhomedircon_settings_t * s,
 	retval = STATUS_SUCCESS;
 cleanup:
 	endpwent();
-	free(pwbuf);
 	free(grbuf);
 
 	return retval;
-- 
2.13.1

Re: [PATCH 1/3] libsepol: replace non-standard use of __BEGIN_DECLS

[Stephen Smalley]

On Tue, 2017-06-20 at 16:07 +0200, Patrick Steinhardt wrote:
> While most header files already use the common pattern of `extern
> "C"`
> declarations to enable compiling in a C++ project, some header files
> in
> libsepol instead use the macros `__BEGIN_DECLS` and `__END_DECLS`.
> These
> macros are defined in the "sys/cdefs.h" header file, which provides
> some non-standard extensions for glibc.
> 
> Convert usage of these declarations with the standard `extern "C"`
> pattern. This improves compatibility with other libc implementations,
> e.g. musl libc.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Thanks, applied.

> ---
>  libsepol/include/sepol/ibendport_record.h | 10 +++++++---
>  libsepol/include/sepol/ibendports.h       | 11 ++++++++---
>  libsepol/include/sepol/ibpkey_record.h    | 11 ++++++++---
>  libsepol/include/sepol/ibpkeys.h          | 12 +++++++++---
>  4 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/libsepol/include/sepol/ibendport_record.h
> b/libsepol/include/sepol/ibendport_record.h
> index e30b252d..2a37ec63 100644
> --- a/libsepol/include/sepol/ibendport_record.h
> +++ b/libsepol/include/sepol/ibendport_record.h
> @@ -4,9 +4,10 @@
>  #include <stddef.h>
>  #include <sepol/context_record.h>
>  #include <sepol/handle.h>
> -#include <sys/cdefs.h>
>  
> -__BEGIN_DECLS
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
>  
>  struct sepol_ibendport;
>  struct sepol_ibendport_key;
> @@ -64,5 +65,8 @@ extern int sepol_ibendport_clone(sepol_handle_t
> *handle,
>  
>  extern void sepol_ibendport_free(sepol_ibendport_t *ibendport);
>  
> -__END_DECLS
> +#ifdef __cplusplus
> +}
> +#endif
> +
>  #endif
> diff --git a/libsepol/include/sepol/ibendports.h
> b/libsepol/include/sepol/ibendports.h
> index 4a89e0ca..4ad77a12 100644
> --- a/libsepol/include/sepol/ibendports.h
> +++ b/libsepol/include/sepol/ibendports.h
> @@ -4,9 +4,10 @@
>  #include <sepol/handle.h>
>  #include <sepol/policydb.h>
>  #include <sepol/ibendport_record.h>
> -#include <sys/cdefs.h>
>  
> -__BEGIN_DECLS
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
>  
>  /* Return the number of ibendports */
>  extern int sepol_ibendport_count(sepol_handle_t *handle,
> @@ -41,5 +42,9 @@ extern int sepol_ibendport_iterate(sepol_handle_t
> *handle,
>  				   int (*fn)(const sepol_ibendport_t
> *ibendport,
>  					     void *fn_arg), void
> *arg);
>  
> -__END_DECLS
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
>  #endif
> diff --git a/libsepol/include/sepol/ibpkey_record.h
> b/libsepol/include/sepol/ibpkey_record.h
> index ab68147c..1511785d 100644
> --- a/libsepol/include/sepol/ibpkey_record.h
> +++ b/libsepol/include/sepol/ibpkey_record.h
> @@ -5,11 +5,12 @@
>  #include <stdint.h>
>  #include <sepol/context_record.h>
>  #include <sepol/handle.h>
> -#include <sys/cdefs.h>
>  
>  #define INET6_ADDRLEN 16
>  
> -__BEGIN_DECLS
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
>  
>  struct sepol_ibpkey;
>  struct sepol_ibpkey_key;
> @@ -71,5 +72,9 @@ extern int sepol_ibpkey_clone(sepol_handle_t
> *handle,
>  
>  extern void sepol_ibpkey_free(sepol_ibpkey_t *ibpkey);
>  
> -__END_DECLS
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
>  #endif
> diff --git a/libsepol/include/sepol/ibpkeys.h
> b/libsepol/include/sepol/ibpkeys.h
> index 4ab0a8a6..4b69d1e9 100644
> --- a/libsepol/include/sepol/ibpkeys.h
> +++ b/libsepol/include/sepol/ibpkeys.h
> @@ -4,9 +4,11 @@
>  #include <sepol/handle.h>
>  #include <sepol/policydb.h>
>  #include <sepol/ibpkey_record.h>
> -#include <sys/cdefs.h>
>  
> -__BEGIN_DECLS
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
>  
>  /* Return the number of ibpkeys */
>  extern int sepol_ibpkey_count(sepol_handle_t *handle,
> @@ -40,5 +42,9 @@ extern int sepol_ibpkey_iterate(sepol_handle_t
> *handle,
>  				int (*fn)(const sepol_ibpkey_t
> *ibpkey,
>  					  void *fn_arg), void *arg);
>  
> -__END_DECLS
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
>  #endif

Re: [PATCH 2/3] libselinux: fix error when FORTIFY_SOURCE is already set

[Stephen Smalley]

On Tue, 2017-06-20 at 16:07 +0200, Patrick Steinhardt wrote:
> Two makefiles of ours pass `-D_FORTIFY_SOURCE` directly to the
> preprocessor. While this does not pose any problems when the value
> has
> not been previously set, it can break the build if it is part of the
> standard build flags.
> 
> Fix the issue by undefining the flag with `-Wp,-U_FORTIFY_SOURCE`
> right
> before redefining the value. This fixes builds with some hardened
> compiler chains.

I don't quite understand why the caller can't just specify their own
CFLAGS, in which case we won't use the ones in libselinux/src/Makefile
(except for the override CFLAGS, which don't include the FORTIFY
options), ala:
# We turn up security all the way to 11!
make CFLAGS="-O -Wp,-D_FORTIFY_SOURCE=11"

The problem I see with your solution is that it means that if your
hardened toolchain wants to select a higher _FORTIFY_SOURCE value in
the future (if such a thing were to exist), we'll just undefine it and
redefine to the lower value in our Makefiles.  If you can't just set
CFLAGS in the caller, then perhaps the libselinux Makefile should only
add FORTIFY options if they aren't already defined.

> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  libselinux/src/Makefile   | 3 ++-
>  libselinux/utils/Makefile | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
> index 4306dd0e..010b7ffe 100644
> --- a/libselinux/src/Makefile
> +++ b/libselinux/src/Makefile
> @@ -59,7 +59,8 @@ ifeq ($(COMPILER), gcc)
>  EXTRA_CFLAGS = -fipa-pure-const -Wlogical-op -Wpacked-bitfield-
> compat -Wsync-nand \
>  	-Wcoverage-mismatch -Wcpp -Wformat-contains-nul
> -Wnormalized=nfc -Wsuggest-attribute=const \
>  	-Wsuggest-attribute=noreturn -Wsuggest-attribute=pure
> -Wtrampolines -Wjump-misses-init \
> -	-Wno-suggest-attribute=pure -Wno-suggest-attribute=const
> -Wp,-D_FORTIFY_SOURCE=2
> +	-Wno-suggest-attribute=pure -Wno-suggest-attribute=const \
> +	-Wp,-U_FORTIFY_SOURCE -Wp,-D_FORTIFY_SOURCE=2
>  else
>  EXTRA_CFLAGS = -Wunused-command-line-argument
>  endif
> diff --git a/libselinux/utils/Makefile b/libselinux/utils/Makefile
> index 474ee95b..c94d7cf2 100644
> --- a/libselinux/utils/Makefile
> +++ b/libselinux/utils/Makefile
> @@ -32,7 +32,8 @@ CFLAGS ?= -O -Wall -W -Wundef -Wformat-y2k
> -Wformat-security -Winit-self -Wmissi
>            -Wformat-extra-args -Wformat-zero-length -Wformat=2
> -Wmultichar \
>            -Woverflow -Wpointer-to-int-cast -Wpragmas \
>            -Wno-missing-field-initializers -Wno-sign-compare \
> -          -Wno-format-nonliteral -Wframe-larger-
> than=$(MAX_STACK_SIZE) -Wp,-D_FORTIFY_SOURCE=2 \
> +          -Wno-format-nonliteral -Wframe-larger-
> than=$(MAX_STACK_SIZE) \
> +          -Wp,-U_FORTIFY_SOURCE -Wp,-D_FORTIFY_SOURCE=2 \
>            -fstack-protector-all --param=ssp-buffer-size=4
> -fexceptions \
>            -fasynchronous-unwind-tables -fdiagnostics-show-option
> -funit-at-a-time \
>            -Werror -Wno-aggregate-return -Wno-redundant-decls \

Re: [PATCH 2/3] libselinux: fix error when FORTIFY_SOURCE is already set

[Jason Zaman]

On Tue, Jun 20, 2017 at 11:14:33AM -0400, Stephen Smalley wrote:
> On Tue, 2017-06-20 at 16:07 +0200, Patrick Steinhardt wrote:
> > Two makefiles of ours pass `-D_FORTIFY_SOURCE` directly to the
> > preprocessor. While this does not pose any problems when the value
> > has
> > not been previously set, it can break the build if it is part of the
> > standard build flags.
> > 
> > Fix the issue by undefining the flag with `-Wp,-U_FORTIFY_SOURCE`
> > right
> > before redefining the value. This fixes builds with some hardened
> > compiler chains.
> 
> I don't quite understand why the caller can't just specify their own
> CFLAGS, in which case we won't use the ones in libselinux/src/Makefile
> (except for the override CFLAGS, which don't include the FORTIFY
> options), ala:
> # We turn up security all the way to 11!
> make CFLAGS="-O -Wp,-D_FORTIFY_SOURCE=11"
> 
> The problem I see with your solution is that it means that if your
> hardened toolchain wants to select a higher _FORTIFY_SOURCE value in
> the future (if such a thing were to exist), we'll just undefine it and
> redefine to the lower value in our Makefiles.  If you can't just set
> CFLAGS in the caller, then perhaps the libselinux Makefile should only
> add FORTIFY options if they aren't already defined.

He said this is on gentoo in which case portage will pretty much always
have CFLAGS set so this is never a problem.

Its only a problem for me if i build manually outside of portage in. I
normally just sed'd out the fortify bit. But i'd agree with the best way
would be to test it, something like cc-option from the kernel
CFLAGS ?= $(call cc-option -Werror -Wl,-D_FORTIFY_SOURCE=2)

-- Jason
> 
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  libselinux/src/Makefile   | 3 ++-
> >  libselinux/utils/Makefile | 3 ++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
> > index 4306dd0e..010b7ffe 100644
> > --- a/libselinux/src/Makefile
> > +++ b/libselinux/src/Makefile
> > @@ -59,7 +59,8 @@ ifeq ($(COMPILER), gcc)
> >  EXTRA_CFLAGS = -fipa-pure-const -Wlogical-op -Wpacked-bitfield-
> > compat -Wsync-nand \
> >  	-Wcoverage-mismatch -Wcpp -Wformat-contains-nul
> > -Wnormalized=nfc -Wsuggest-attribute=const \
> >  	-Wsuggest-attribute=noreturn -Wsuggest-attribute=pure
> > -Wtrampolines -Wjump-misses-init \
> > -	-Wno-suggest-attribute=pure -Wno-suggest-attribute=const
> > -Wp,-D_FORTIFY_SOURCE=2
> > +	-Wno-suggest-attribute=pure -Wno-suggest-attribute=const \
> > +	-Wp,-U_FORTIFY_SOURCE -Wp,-D_FORTIFY_SOURCE=2
> >  else
> >  EXTRA_CFLAGS = -Wunused-command-line-argument
> >  endif
> > diff --git a/libselinux/utils/Makefile b/libselinux/utils/Makefile
> > index 474ee95b..c94d7cf2 100644
> > --- a/libselinux/utils/Makefile
> > +++ b/libselinux/utils/Makefile
> > @@ -32,7 +32,8 @@ CFLAGS ?= -O -Wall -W -Wundef -Wformat-y2k
> > -Wformat-security -Winit-self -Wmissi
> >            -Wformat-extra-args -Wformat-zero-length -Wformat=2
> > -Wmultichar \
> >            -Woverflow -Wpointer-to-int-cast -Wpragmas \
> >            -Wno-missing-field-initializers -Wno-sign-compare \
> > -          -Wno-format-nonliteral -Wframe-larger-
> > than=$(MAX_STACK_SIZE) -Wp,-D_FORTIFY_SOURCE=2 \
> > +          -Wno-format-nonliteral -Wframe-larger-
> > than=$(MAX_STACK_SIZE) \
> > +          -Wp,-U_FORTIFY_SOURCE -Wp,-D_FORTIFY_SOURCE=2 \
> >            -fstack-protector-all --param=ssp-buffer-size=4
> > -fexceptions \
> >            -fasynchronous-unwind-tables -fdiagnostics-show-option
> > -funit-at-a-time \
> >            -Werror -Wno-aggregate-return -Wno-redundant-decls \

Re: [PATCH 3/3] genhomedircon: avoid use of non-standard `getpwent_r`

[Stephen Smalley]

On Tue, 2017-06-20 at 16:07 +0200, Patrick Steinhardt wrote:
> The `getpwent_r` function is a non-standard but re-entrant version of
> the sdardardized `getpwent` function. Next to the benefit of being
> re-entrant, though, it avoids compilation with libc implementations
> that
> do not provide this glibc-specific extension, for example musl libc.
> 
> As the code is usually not run in a threaded environment, it is save
> to
> use the non-reentrant version, though. As such, convert code to use
> `getpwent` instead to fix building with other libc implementations.

Not sure we are guaranteed that, since libsemanage has external users
too, e.g. sssd among others.  OTOH, getpwent_r man page says that it is
not really reentrant either due to shared reading position in the
stream, so maybe this doesn't matter.

> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  libsemanage/src/genhomedircon.c | 22 +++++-----------------
>  1 file changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/libsemanage/src/genhomedircon.c
> b/libsemanage/src/genhomedircon.c
> index e8c95ee4..f58c17ce 100644
> --- a/libsemanage/src/genhomedircon.c
> +++ b/libsemanage/src/genhomedircon.c
> @@ -295,9 +295,8 @@ static semanage_list_t
> *get_home_dirs(genhomedircon_settings_t * s)
>  	long rbuflen;
>  	uid_t temp, minuid = 500, maxuid = 60000;
>  	int minuid_set = 0;
> -	struct passwd pwstorage, *pwbuf;
> +	struct passwd *pwbuf;
>  	struct stat buf;
> -	int retval;
>  
>  	path = semanage_findval(PATH_ETC_USERADD, "HOME", "=");
>  	if (path && *path) {
> @@ -369,7 +368,7 @@ static semanage_list_t
> *get_home_dirs(genhomedircon_settings_t * s)
>  	if (rbuf == NULL)
>  		goto fail;
>  	setpwent();
> -	while ((retval = getpwent_r(&pwstorage, rbuf, rbuflen,
> &pwbuf)) == 0) {
> +	while ((pwbuf = getpwent()) != NULL) {

Shouldn't you have removed rbuflen and rbuf too?  They are no longer
used (except to allocate and free, but never used otherwise) after this
change.

>  		if (pwbuf->pw_uid < minuid || pwbuf->pw_uid >
> maxuid)
>  			continue;
>  		if (!semanage_list_find(shells, pwbuf->pw_shell))
> @@ -413,7 +412,7 @@ static semanage_list_t
> *get_home_dirs(genhomedircon_settings_t * s)
>  		path = NULL;
>  	}
>  
> -	if (retval && retval != ENOENT) {
> +	if (errno && errno != ENOENT) {

Does getpwent() set errno to ENOENT? Not mentioned in its man page,
unlike for getpwent_r().

>  		WARN(s->h_semanage, "Error while fetching users.  "
>  		     "Returning list so far.");
>  	}
> @@ -1064,10 +1063,7 @@ static int
> get_group_users(genhomedircon_settings_t * s,
>  	long grbuflen;
>  	char *grbuf = NULL;
>  	struct group grstorage, *group = NULL;
> -
> -	long prbuflen;
> -	char *pwbuf = NULL;
> -	struct passwd pwstorage, *pw = NULL;
> +	struct passwd *pw = NULL;
>  
>  	grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX);
>  	if (grbuflen <= 0)
> @@ -1104,15 +1100,8 @@ static int
> get_group_users(genhomedircon_settings_t * s,
>  		}
>  	}
>  
> -	prbuflen = sysconf(_SC_GETPW_R_SIZE_MAX);
> -	if (prbuflen <= 0)
> -		goto cleanup;
> -	pwbuf = malloc(prbuflen);
> -	if (pwbuf == NULL)
> -		goto cleanup;
> -
>  	setpwent();
> -	while ((retval = getpwent_r(&pwstorage, pwbuf, prbuflen,
> &pw)) == 0) {
> +	while ((pw = getpwent()) != NULL) {
>  		// skip users who also have this group as their
>  		// primary group
>  		if (lfind(pw->pw_name, group->gr_mem, &nmembers,

Interestingly, this goes on to call add_user(), which calls
getpwnam_r().  So if that one was ever switched back to just
getpwnam(), we'd have a potential problem there.

> @@ -1131,7 +1120,6 @@ static int
> get_group_users(genhomedircon_settings_t * s,
>  	retval = STATUS_SUCCESS;
>  cleanup:
>  	endpwent();
> -	free(pwbuf);
>  	free(grbuf);
>  
>  	return retval;

Re: [PATCH 2/3] libselinux: fix error when FORTIFY_SOURCE is already set

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3464 bytes --]

On Tue, Jun 20, 2017 at 11:14:33AM -0400, Stephen Smalley wrote:
> On Tue, 2017-06-20 at 16:07 +0200, Patrick Steinhardt wrote:
> > Two makefiles of ours pass `-D_FORTIFY_SOURCE` directly to the
> > preprocessor. While this does not pose any problems when the value
> > has
> > not been previously set, it can break the build if it is part of the
> > standard build flags.
> > 
> > Fix the issue by undefining the flag with `-Wp,-U_FORTIFY_SOURCE`
> > right
> > before redefining the value. This fixes builds with some hardened
> > compiler chains.
> 
> I don't quite understand why the caller can't just specify their own
> CFLAGS, in which case we won't use the ones in libselinux/src/Makefile
> (except for the override CFLAGS, which don't include the FORTIFY
> options), ala:
> # We turn up security all the way to 11!
> make CFLAGS="-O -Wp,-D_FORTIFY_SOURCE=11"
> 
> The problem I see with your solution is that it means that if your
> hardened toolchain wants to select a higher _FORTIFY_SOURCE value in
> the future (if such a thing were to exist), we'll just undefine it and
> redefine to the lower value in our Makefiles.  If you can't just set
> CFLAGS in the caller, then perhaps the libselinux Makefile should only
> add FORTIFY options if they aren't already defined.

This does sound more reasonable than my approach, agreed. Will
change as proposed in version 2, thanks.

Patrick

> 
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  libselinux/src/Makefile   | 3 ++-
> >  libselinux/utils/Makefile | 3 ++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
> > index 4306dd0e..010b7ffe 100644
> > --- a/libselinux/src/Makefile
> > +++ b/libselinux/src/Makefile
> > @@ -59,7 +59,8 @@ ifeq ($(COMPILER), gcc)
> >  EXTRA_CFLAGS = -fipa-pure-const -Wlogical-op -Wpacked-bitfield-
> > compat -Wsync-nand \
> >  	-Wcoverage-mismatch -Wcpp -Wformat-contains-nul
> > -Wnormalized=nfc -Wsuggest-attribute=const \
> >  	-Wsuggest-attribute=noreturn -Wsuggest-attribute=pure
> > -Wtrampolines -Wjump-misses-init \
> > -	-Wno-suggest-attribute=pure -Wno-suggest-attribute=const
> > -Wp,-D_FORTIFY_SOURCE=2
> > +	-Wno-suggest-attribute=pure -Wno-suggest-attribute=const \
> > +	-Wp,-U_FORTIFY_SOURCE -Wp,-D_FORTIFY_SOURCE=2
> >  else
> >  EXTRA_CFLAGS = -Wunused-command-line-argument
> >  endif
> > diff --git a/libselinux/utils/Makefile b/libselinux/utils/Makefile
> > index 474ee95b..c94d7cf2 100644
> > --- a/libselinux/utils/Makefile
> > +++ b/libselinux/utils/Makefile
> > @@ -32,7 +32,8 @@ CFLAGS ?= -O -Wall -W -Wundef -Wformat-y2k
> > -Wformat-security -Winit-self -Wmissi
> >            -Wformat-extra-args -Wformat-zero-length -Wformat=2
> > -Wmultichar \
> >            -Woverflow -Wpointer-to-int-cast -Wpragmas \
> >            -Wno-missing-field-initializers -Wno-sign-compare \
> > -          -Wno-format-nonliteral -Wframe-larger-
> > than=$(MAX_STACK_SIZE) -Wp,-D_FORTIFY_SOURCE=2 \
> > +          -Wno-format-nonliteral -Wframe-larger-
> > than=$(MAX_STACK_SIZE) \
> > +          -Wp,-U_FORTIFY_SOURCE -Wp,-D_FORTIFY_SOURCE=2 \
> >            -fstack-protector-all --param=ssp-buffer-size=4
> > -fexceptions \
> >            -fasynchronous-unwind-tables -fdiagnostics-show-option
> > -funit-at-a-time \
> >            -Werror -Wno-aggregate-return -Wno-redundant-decls \

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] genhomedircon: avoid use of non-standard `getpwent_r`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4847 bytes --]

On Tue, Jun 20, 2017 at 11:42:59AM -0400, Stephen Smalley wrote:
> On Tue, 2017-06-20 at 16:07 +0200, Patrick Steinhardt wrote:
> > The `getpwent_r` function is a non-standard but re-entrant version of
> > the sdardardized `getpwent` function. Next to the benefit of being
> > re-entrant, though, it avoids compilation with libc implementations
> > that
> > do not provide this glibc-specific extension, for example musl libc.
> > 
> > As the code is usually not run in a threaded environment, it is save
> > to
> > use the non-reentrant version, though. As such, convert code to use
> > `getpwent` instead to fix building with other libc implementations.
> 
> Not sure we are guaranteed that, since libsemanage has external users
> too, e.g. sssd among others.  OTOH, getpwent_r man page says that it is
> not really reentrant either due to shared reading position in the
> stream, so maybe this doesn't matter.

Hum, okay. man-pages differ here in their wording across systems,
but I guess the part you're referring to is

```
The functions getpwent_r() and fgetpwent_r() are the reentrant
versions of getpwent(3) and fgetpwent(3). The former reads the
next passwd entry from the stream initialized by setpwent(3). The
latter reads the next passwd entry from the stream fp.
```

While it indeed seems to imply that the stream by `setpwent` is
used, they also pretend to be reentrant here. Puzzled.

> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  libsemanage/src/genhomedircon.c | 22 +++++-----------------
> >  1 file changed, 5 insertions(+), 17 deletions(-)
> > 
> > diff --git a/libsemanage/src/genhomedircon.c
> > b/libsemanage/src/genhomedircon.c
> > index e8c95ee4..f58c17ce 100644
> > --- a/libsemanage/src/genhomedircon.c
> > +++ b/libsemanage/src/genhomedircon.c
> > @@ -295,9 +295,8 @@ static semanage_list_t
> > *get_home_dirs(genhomedircon_settings_t * s)
> >  	long rbuflen;
> >  	uid_t temp, minuid = 500, maxuid = 60000;
> >  	int minuid_set = 0;
> > -	struct passwd pwstorage, *pwbuf;
> > +	struct passwd *pwbuf;
> >  	struct stat buf;
> > -	int retval;
> >  
> >  	path = semanage_findval(PATH_ETC_USERADD, "HOME", "=");
> >  	if (path && *path) {
> > @@ -369,7 +368,7 @@ static semanage_list_t
> > *get_home_dirs(genhomedircon_settings_t * s)
> >  	if (rbuf == NULL)
> >  		goto fail;
> >  	setpwent();
> > -	while ((retval = getpwent_r(&pwstorage, rbuf, rbuflen,
> > &pwbuf)) == 0) {
> > +	while ((pwbuf = getpwent()) != NULL) {
> 
> Shouldn't you have removed rbuflen and rbuf too?  They are no longer
> used (except to allocate and free, but never used otherwise) after this
> change.

That was an oversight, yeah. Fixed in v2.

> >  		if (pwbuf->pw_uid < minuid || pwbuf->pw_uid >
> > maxuid)
> >  			continue;
> >  		if (!semanage_list_find(shells, pwbuf->pw_shell))
> > @@ -413,7 +412,7 @@ static semanage_list_t
> > *get_home_dirs(genhomedircon_settings_t * s)
> >  		path = NULL;
> >  	}
> >  
> > -	if (retval && retval != ENOENT) {
> > +	if (errno && errno != ENOENT) {
> 
> Does getpwent() set errno to ENOENT? Not mentioned in its man page,
> unlike for getpwent_r().

You're right, it does not. Will remove this check.

> >  		WARN(s->h_semanage, "Error while fetching users.  "
> >  		     "Returning list so far.");
> >  	}
> > @@ -1064,10 +1063,7 @@ static int
> > get_group_users(genhomedircon_settings_t * s,
> >  	long grbuflen;
> >  	char *grbuf = NULL;
> >  	struct group grstorage, *group = NULL;
> > -
> > -	long prbuflen;
> > -	char *pwbuf = NULL;
> > -	struct passwd pwstorage, *pw = NULL;
> > +	struct passwd *pw = NULL;
> >  
> >  	grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX);
> >  	if (grbuflen <= 0)
> > @@ -1104,15 +1100,8 @@ static int
> > get_group_users(genhomedircon_settings_t * s,
> >  		}
> >  	}
> >  
> > -	prbuflen = sysconf(_SC_GETPW_R_SIZE_MAX);
> > -	if (prbuflen <= 0)
> > -		goto cleanup;
> > -	pwbuf = malloc(prbuflen);
> > -	if (pwbuf == NULL)
> > -		goto cleanup;
> > -
> >  	setpwent();
> > -	while ((retval = getpwent_r(&pwstorage, pwbuf, prbuflen,
> > &pw)) == 0) {
> > +	while ((pw = getpwent()) != NULL) {
> >  		// skip users who also have this group as their
> >  		// primary group
> >  		if (lfind(pw->pw_name, group->gr_mem, &nmembers,
> 
> Interestingly, this goes on to call add_user(), which calls
> getpwnam_r().  So if that one was ever switched back to just
> getpwnam(), we'd have a potential problem there.

Agreed. But in fact, `getpwnam_r` is part of POSIX, so there is
no need to do so here.

> > @@ -1131,7 +1120,6 @@ static int
> > get_group_users(genhomedircon_settings_t * s,
> >  	retval = STATUS_SUCCESS;
> >  cleanup:
> >  	endpwent();
> > -	free(pwbuf);
> >  	free(grbuf);
> >  
> >  	return retval;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 0/2] Portability improvements

[Patrick Steinhardt]

Hi,

this is the second version of my portability fixes. Changes
include applying proposed changes (thanks Stephen and Jason) as
well as improved commit messages. The interdiff is attached
below.

I've dropped the first patch as it's already been applied.

Patrick

Patrick Steinhardt (2):
  libselinux: avoid redefining _FORTIFY_SOURCE
  genhomedircon: avoid use of non-standard `getpwent_r`

 libselinux/src/Makefile         |  2 +-
 libselinux/utils/Makefile       |  2 +-
 libsemanage/src/genhomedircon.c | 34 +++++++---------------------------
 3 files changed, 9 insertions(+), 29 deletions(-)

-- 
2.13.1

diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
index 010b7ffe..ea912609 100644
--- a/libselinux/src/Makefile
+++ b/libselinux/src/Makefile
@@ -59,8 +59,7 @@ ifeq ($(COMPILER), gcc)
 EXTRA_CFLAGS = -fipa-pure-const -Wlogical-op -Wpacked-bitfield-compat -Wsync-nand \
 	-Wcoverage-mismatch -Wcpp -Wformat-contains-nul -Wnormalized=nfc -Wsuggest-attribute=const \
 	-Wsuggest-attribute=noreturn -Wsuggest-attribute=pure -Wtrampolines -Wjump-misses-init \
-	-Wno-suggest-attribute=pure -Wno-suggest-attribute=const \
-	-Wp,-U_FORTIFY_SOURCE -Wp,-D_FORTIFY_SOURCE=2
+	-Wno-suggest-attribute=pure -Wno-suggest-attribute=const -Wp,-D_FORTIFY_SOURCE
 else
 EXTRA_CFLAGS = -Wunused-command-line-argument
 endif
diff --git a/libselinux/utils/Makefile b/libselinux/utils/Makefile
index eb28120d..eb4851a9 100644
--- a/libselinux/utils/Makefile
+++ b/libselinux/utils/Makefile
@@ -32,8 +32,7 @@ CFLAGS ?= -O -Wall -W -Wundef -Wformat-y2k -Wformat-security -Winit-self -Wmissi
           -Wformat-extra-args -Wformat-zero-length -Wformat=2 -Wmultichar \
           -Woverflow -Wpointer-to-int-cast -Wpragmas \
           -Wno-missing-field-initializers -Wno-sign-compare \
-          -Wno-format-nonliteral -Wframe-larger-than=$(MAX_STACK_SIZE) \
-          -Wp,-U_FORTIFY_SOURCE -Wp,-D_FORTIFY_SOURCE=2 \
+          -Wno-format-nonliteral -Wframe-larger-than=$(MAX_STACK_SIZE) -Wp,-D_FORTIFY_SOURCE \
           -fstack-protector-all --param=ssp-buffer-size=4 -fexceptions \
           -fasynchronous-unwind-tables -fdiagnostics-show-option -funit-at-a-time \
           -Werror -Wno-aggregate-return -Wno-redundant-decls \
diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
index f58c17ce..b9a74b73 100644
--- a/libsemanage/src/genhomedircon.c
+++ b/libsemanage/src/genhomedircon.c
@@ -290,9 +290,7 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 	semanage_list_t *homedir_list = NULL;
 	semanage_list_t *shells = NULL;
 	fc_match_handle_t hand;
-	char *rbuf = NULL;
 	char *path = NULL;
-	long rbuflen;
 	uid_t temp, minuid = 500, maxuid = 60000;
 	int minuid_set = 0;
 	struct passwd *pwbuf;
@@ -361,12 +359,7 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 	free(path);
 	path = NULL;
 
-	rbuflen = sysconf(_SC_GETPW_R_SIZE_MAX);
-	if (rbuflen <= 0)
-		goto fail;
-	rbuf = malloc(rbuflen);
-	if (rbuf == NULL)
-		goto fail;
+	errno = 0;
 	setpwent();
 	while ((pwbuf = getpwent()) != NULL) {
 		if (pwbuf->pw_uid < minuid || pwbuf->pw_uid > maxuid)
@@ -410,9 +403,10 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 		}
 		free(path);
 		path = NULL;
+		errno = 0;
 	}
 
-	if (errno && errno != ENOENT) {
+	if (errno) {
 		WARN(s->h_semanage, "Error while fetching users.  "
 		     "Returning list so far.");
 	}
@@ -421,14 +415,12 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 		goto fail;
 
 	endpwent();
-	free(rbuf);
 	semanage_list_destroy(&shells);
 
 	return homedir_list;
 
       fail:
 	endpwent();
-	free(rbuf);
 	free(path);
 	semanage_list_destroy(&homedir_list);
 	semanage_list_destroy(&shells);

[PATCH v2 1/2] libselinux: avoid redefining _FORTIFY_SOURCE

[Patrick Steinhardt]

Two makefiles of ours pass `-D_FORTIFY_SOURCE=2` directly to the
preprocessor. While this does not pose any problems when the value has
not been previously set, it can break the build if it is part of the
standard build flags.

The issue can easily be fixed by instead defining `_FORTIFY_SOURCE`
without specifying a concrete value. In this case, gcc will not error
out and simply keep using the previously defined value. On the other
hand, if no value has been defined, we will now compile with
`_FORTIFY_SOURCE=1`. From feature_test_macros(7):

    If _FORTIFY_SOURCE is set to 1, with compiler optimization level 1
    (gcc -O1) and above, checks that shouldn't change the behavior of
    conforming programs are performed.  With _FORTIFY_SOURCE set to 2,
    some more checking is added, but some conforming programs might
    fail.

While this leaves us with less checks for buffer overflows, it will only
enable checks that should not change behaviour of conforming programs.
With _FORTIFY_SOURCE=2, the compiler may even unintentionally change
behaviour of conforming programs. So in fact, one could even argue that
we should only be setting the value to 1 anyway to avoid surprising side
effects.

So this patch changes our CFLAGS to only pass `-D_FORTIFY_SOURCE`
without any concrete value, fixing the build issue.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 libselinux/src/Makefile   | 2 +-
 libselinux/utils/Makefile | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
index 4306dd0e..ea912609 100644
--- a/libselinux/src/Makefile
+++ b/libselinux/src/Makefile
@@ -59,7 +59,7 @@ ifeq ($(COMPILER), gcc)
 EXTRA_CFLAGS = -fipa-pure-const -Wlogical-op -Wpacked-bitfield-compat -Wsync-nand \
 	-Wcoverage-mismatch -Wcpp -Wformat-contains-nul -Wnormalized=nfc -Wsuggest-attribute=const \
 	-Wsuggest-attribute=noreturn -Wsuggest-attribute=pure -Wtrampolines -Wjump-misses-init \
-	-Wno-suggest-attribute=pure -Wno-suggest-attribute=const -Wp,-D_FORTIFY_SOURCE=2
+	-Wno-suggest-attribute=pure -Wno-suggest-attribute=const -Wp,-D_FORTIFY_SOURCE
 else
 EXTRA_CFLAGS = -Wunused-command-line-argument
 endif
diff --git a/libselinux/utils/Makefile b/libselinux/utils/Makefile
index 843b0e7c..eb4851a9 100644
--- a/libselinux/utils/Makefile
+++ b/libselinux/utils/Makefile
@@ -32,7 +32,7 @@ CFLAGS ?= -O -Wall -W -Wundef -Wformat-y2k -Wformat-security -Winit-self -Wmissi
           -Wformat-extra-args -Wformat-zero-length -Wformat=2 -Wmultichar \
           -Woverflow -Wpointer-to-int-cast -Wpragmas \
           -Wno-missing-field-initializers -Wno-sign-compare \
-          -Wno-format-nonliteral -Wframe-larger-than=$(MAX_STACK_SIZE) -Wp,-D_FORTIFY_SOURCE=2 \
+          -Wno-format-nonliteral -Wframe-larger-than=$(MAX_STACK_SIZE) -Wp,-D_FORTIFY_SOURCE \
           -fstack-protector-all --param=ssp-buffer-size=4 -fexceptions \
           -fasynchronous-unwind-tables -fdiagnostics-show-option -funit-at-a-time \
           -Werror -Wno-aggregate-return -Wno-redundant-decls \
-- 
2.13.1

[PATCH v2 2/2] genhomedircon: avoid use of non-standard `getpwent_r`

[Patrick Steinhardt]

The `getpwent_r` function is a non-standard but reentrant version of the
POSIX-defined `getpwent` function. While it should provide the benefit
of being safe to use in multi-threaded environments, it disallows us
from compiling with libc implementations which stick to the POSIX
standard more closely.

As libsemanage may be used in a multi-threaded environment, being
reentrant may in fact be quite important to us. As such, simply
switching out `getpwent_r` against its non-reentrant function can prove
quite dangerous. But interestingly enough, the glibc implementation of
`getpwent_r` does not even guarantee being reentrant. Quoting from
getpwent_r(7):

    NOTES

    The function getpwent_r() is not really reentrant since it shares
    the reading position in the stream with all other threads.

As such, it is non-reentrant in the same sense as its simple `getpwent`
brother and can simply be switched out without losing any guarantees
here.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 libsemanage/src/genhomedircon.c | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

diff --git a/libsemanage/src/genhomedircon.c b/libsemanage/src/genhomedircon.c
index e8c95ee4..b9a74b73 100644
--- a/libsemanage/src/genhomedircon.c
+++ b/libsemanage/src/genhomedircon.c
@@ -290,14 +290,11 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 	semanage_list_t *homedir_list = NULL;
 	semanage_list_t *shells = NULL;
 	fc_match_handle_t hand;
-	char *rbuf = NULL;
 	char *path = NULL;
-	long rbuflen;
 	uid_t temp, minuid = 500, maxuid = 60000;
 	int minuid_set = 0;
-	struct passwd pwstorage, *pwbuf;
+	struct passwd *pwbuf;
 	struct stat buf;
-	int retval;
 
 	path = semanage_findval(PATH_ETC_USERADD, "HOME", "=");
 	if (path && *path) {
@@ -362,14 +359,9 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 	free(path);
 	path = NULL;
 
-	rbuflen = sysconf(_SC_GETPW_R_SIZE_MAX);
-	if (rbuflen <= 0)
-		goto fail;
-	rbuf = malloc(rbuflen);
-	if (rbuf == NULL)
-		goto fail;
+	errno = 0;
 	setpwent();
-	while ((retval = getpwent_r(&pwstorage, rbuf, rbuflen, &pwbuf)) == 0) {
+	while ((pwbuf = getpwent()) != NULL) {
 		if (pwbuf->pw_uid < minuid || pwbuf->pw_uid > maxuid)
 			continue;
 		if (!semanage_list_find(shells, pwbuf->pw_shell))
@@ -411,9 +403,10 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 		}
 		free(path);
 		path = NULL;
+		errno = 0;
 	}
 
-	if (retval && retval != ENOENT) {
+	if (errno) {
 		WARN(s->h_semanage, "Error while fetching users.  "
 		     "Returning list so far.");
 	}
@@ -422,14 +415,12 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
 		goto fail;
 
 	endpwent();
-	free(rbuf);
 	semanage_list_destroy(&shells);
 
 	return homedir_list;
 
       fail:
 	endpwent();
-	free(rbuf);
 	free(path);
 	semanage_list_destroy(&homedir_list);
 	semanage_list_destroy(&shells);
@@ -1064,10 +1055,7 @@ static int get_group_users(genhomedircon_settings_t * s,
 	long grbuflen;
 	char *grbuf = NULL;
 	struct group grstorage, *group = NULL;
-
-	long prbuflen;
-	char *pwbuf = NULL;
-	struct passwd pwstorage, *pw = NULL;
+	struct passwd *pw = NULL;
 
 	grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX);
 	if (grbuflen <= 0)
@@ -1104,15 +1092,8 @@ static int get_group_users(genhomedircon_settings_t * s,
 		}
 	}
 
-	prbuflen = sysconf(_SC_GETPW_R_SIZE_MAX);
-	if (prbuflen <= 0)
-		goto cleanup;
-	pwbuf = malloc(prbuflen);
-	if (pwbuf == NULL)
-		goto cleanup;
-
 	setpwent();
-	while ((retval = getpwent_r(&pwstorage, pwbuf, prbuflen, &pw)) == 0) {
+	while ((pw = getpwent()) != NULL) {
 		// skip users who also have this group as their
 		// primary group
 		if (lfind(pw->pw_name, group->gr_mem, &nmembers,
@@ -1131,7 +1112,6 @@ static int get_group_users(genhomedircon_settings_t * s,
 	retval = STATUS_SUCCESS;
 cleanup:
 	endpwent();
-	free(pwbuf);
 	free(grbuf);
 
 	return retval;
-- 
2.13.1

Re: [PATCH v2 2/2] genhomedircon: avoid use of non-standard `getpwent_r`

[Stephen Smalley]

On Thu, 2017-06-22 at 11:45 +0200, Patrick Steinhardt wrote:
> The `getpwent_r` function is a non-standard but reentrant version of
> the
> POSIX-defined `getpwent` function. While it should provide the
> benefit
> of being safe to use in multi-threaded environments, it disallows us
> from compiling with libc implementations which stick to the POSIX
> standard more closely.
> 
> As libsemanage may be used in a multi-threaded environment, being
> reentrant may in fact be quite important to us. As such, simply
> switching out `getpwent_r` against its non-reentrant function can
> prove
> quite dangerous. But interestingly enough, the glibc implementation
> of
> `getpwent_r` does not even guarantee being reentrant. Quoting from
> getpwent_r(7):
> 
>     NOTES
> 
>     The function getpwent_r() is not really reentrant since it shares
>     the reading position in the stream with all other threads.
> 
> As such, it is non-reentrant in the same sense as its simple
> `getpwent`
> brother and can simply be switched out without losing any guarantees
> here.

Thanks, both patches applied.

> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  libsemanage/src/genhomedircon.c | 34 +++++++----------------------
> -----
>  1 file changed, 7 insertions(+), 27 deletions(-)
> 
> diff --git a/libsemanage/src/genhomedircon.c
> b/libsemanage/src/genhomedircon.c
> index e8c95ee4..b9a74b73 100644
> --- a/libsemanage/src/genhomedircon.c
> +++ b/libsemanage/src/genhomedircon.c
> @@ -290,14 +290,11 @@ static semanage_list_t
> *get_home_dirs(genhomedircon_settings_t * s)
>  	semanage_list_t *homedir_list = NULL;
>  	semanage_list_t *shells = NULL;
>  	fc_match_handle_t hand;
> -	char *rbuf = NULL;
>  	char *path = NULL;
> -	long rbuflen;
>  	uid_t temp, minuid = 500, maxuid = 60000;
>  	int minuid_set = 0;
> -	struct passwd pwstorage, *pwbuf;
> +	struct passwd *pwbuf;
>  	struct stat buf;
> -	int retval;
>  
>  	path = semanage_findval(PATH_ETC_USERADD, "HOME", "=");
>  	if (path && *path) {
> @@ -362,14 +359,9 @@ static semanage_list_t
> *get_home_dirs(genhomedircon_settings_t * s)
>  	free(path);
>  	path = NULL;
>  
> -	rbuflen = sysconf(_SC_GETPW_R_SIZE_MAX);
> -	if (rbuflen <= 0)
> -		goto fail;
> -	rbuf = malloc(rbuflen);
> -	if (rbuf == NULL)
> -		goto fail;
> +	errno = 0;
>  	setpwent();
> -	while ((retval = getpwent_r(&pwstorage, rbuf, rbuflen,
> &pwbuf)) == 0) {
> +	while ((pwbuf = getpwent()) != NULL) {
>  		if (pwbuf->pw_uid < minuid || pwbuf->pw_uid >
> maxuid)
>  			continue;
>  		if (!semanage_list_find(shells, pwbuf->pw_shell))
> @@ -411,9 +403,10 @@ static semanage_list_t
> *get_home_dirs(genhomedircon_settings_t * s)
>  		}
>  		free(path);
>  		path = NULL;
> +		errno = 0;
>  	}
>  
> -	if (retval && retval != ENOENT) {
> +	if (errno) {
>  		WARN(s->h_semanage, "Error while fetching users.  "
>  		     "Returning list so far.");
>  	}
> @@ -422,14 +415,12 @@ static semanage_list_t
> *get_home_dirs(genhomedircon_settings_t * s)
>  		goto fail;
>  
>  	endpwent();
> -	free(rbuf);
>  	semanage_list_destroy(&shells);
>  
>  	return homedir_list;
>  
>        fail:
>  	endpwent();
> -	free(rbuf);
>  	free(path);
>  	semanage_list_destroy(&homedir_list);
>  	semanage_list_destroy(&shells);
> @@ -1064,10 +1055,7 @@ static int
> get_group_users(genhomedircon_settings_t * s,
>  	long grbuflen;
>  	char *grbuf = NULL;
>  	struct group grstorage, *group = NULL;
> -
> -	long prbuflen;
> -	char *pwbuf = NULL;
> -	struct passwd pwstorage, *pw = NULL;
> +	struct passwd *pw = NULL;
>  
>  	grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX);
>  	if (grbuflen <= 0)
> @@ -1104,15 +1092,8 @@ static int
> get_group_users(genhomedircon_settings_t * s,
>  		}
>  	}
>  
> -	prbuflen = sysconf(_SC_GETPW_R_SIZE_MAX);
> -	if (prbuflen <= 0)
> -		goto cleanup;
> -	pwbuf = malloc(prbuflen);
> -	if (pwbuf == NULL)
> -		goto cleanup;
> -
>  	setpwent();
> -	while ((retval = getpwent_r(&pwstorage, pwbuf, prbuflen,
> &pw)) == 0) {
> +	while ((pw = getpwent()) != NULL) {
>  		// skip users who also have this group as their
>  		// primary group
>  		if (lfind(pw->pw_name, group->gr_mem, &nmembers,
> @@ -1131,7 +1112,6 @@ static int
> get_group_users(genhomedircon_settings_t * s,
>  	retval = STATUS_SUCCESS;
>  cleanup:
>  	endpwent();
> -	free(pwbuf);
>  	free(grbuf);
>  
>  	return retval;