[PATCH] libsemanage: define basename macro for non-glibc systems

[PATCH] libsemanage: define basename macro for non-glibc systems

[Rahul Sandhu]

Passing a const char *path to basename(3) is a glibc specific
extension.

Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
---
 libsemanage/src/conf-parse.y | 3 +++
 libsemanage/src/direct_api.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf-parse.y
index 6cb8a598..97cc5438 100644
--- a/libsemanage/src/conf-parse.y
+++ b/libsemanage/src/conf-parse.y
@@ -50,6 +50,9 @@ static external_prog_t *new_external;
 static int parse_errors;
 
 #define PASSIGN(p1,p2) { free(p1); p1 = p2; }
+#if !defined(__GLIBC__)
+#define basename(src) (strrchr(src, '/') ? strrchr(src, '/') + 1 : src)
+#endif
 
 %}
 
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index 99cba7f7..4459a7d7 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -63,6 +63,9 @@
 #define PIPE_READ 0
 #define PIPE_WRITE 1
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#if !defined(__GLIBC__)
+#define basename(src) (strrchr(src, '/') ? strrchr(src, '/') + 1 : src)
+#endif
 
 static void semanage_direct_destroy(semanage_handle_t * sh);
 static int semanage_direct_disconnect(semanage_handle_t * sh);
-- 
2.48.1

Re: [PATCH] libsemanage: define basename macro for non-glibc systems

[William Roberts]

On Thu, Feb 20, 2025 at 3:13 PM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> Passing a const char *path to basename(3) is a glibc specific
> extension.
>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> ---
>  libsemanage/src/conf-parse.y | 3 +++
>  libsemanage/src/direct_api.c | 3 +++
>  2 files changed, 6 insertions(+)
>
> diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf-parse.y
> index 6cb8a598..97cc5438 100644
> --- a/libsemanage/src/conf-parse.y
> +++ b/libsemanage/src/conf-parse.y
> @@ -50,6 +50,9 @@ static external_prog_t *new_external;
>  static int parse_errors;
>
>  #define PASSIGN(p1,p2) { free(p1); p1 = p2; }
> +#if !defined(__GLIBC__)
> +#define basename(src) (strrchr(src, '/') ? strrchr(src, '/') + 1 : src)
> +#endif
>
>  %}
>
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index 99cba7f7..4459a7d7 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -63,6 +63,9 @@
>  #define PIPE_READ 0
>  #define PIPE_WRITE 1
>  #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +#if !defined(__GLIBC__)
> +#define basename(src) (strrchr(src, '/') ? strrchr(src, '/') + 1 : src)
> +#endif
>
>  static void semanage_direct_destroy(semanage_handle_t * sh);
>  static int semanage_direct_disconnect(semanage_handle_t * sh);
> --
> 2.48.1
>

What system are you on where you run libsemange without glibc, just curious?

I am not opposed to adding an implementation for basename where the
input can be read only for non-glibc, but this patch doesn't work:
Per the man page, https://man7.org/linux/man-pages/man3/basename.3.html,
basename("/") should return "/", this one return "\0"
basename("/usr/"); should return "usr", this returns "\0".

There are two ways you could approach this:
1. If you wanted to do an implementation, I would add it to
utilities.c/h and call it something other than basename so we don't
get any odd issues with it choosing the one from the headers over the
macro. Perhaps libsemange_basename(). On glibc systems this would just
call basename directly and on non-glibc systems do the implementation
with your own logic.
2. Just copy the path into a modifiable buffer on non-glibc systems

I would do both approaches. Create a utility routine that calls
basename for glibc and for non-glibc just copies it to a modifiable
buffer and then calls basename over rolling our own and the bugs
associated with it, also add a unit test for this. This would keep the
logic in one place and be dirt simple.

Bill

Re: [PATCH] libsemanage: define basename macro for non-glibc systems

[William Roberts]

On Thu, Feb 20, 2025 at 6:16 PM William Roberts
<bill.c.roberts@gmail.com> wrote:
>
> On Thu, Feb 20, 2025 at 3:13 PM Rahul Sandhu <nvraxn@gmail.com> wrote:
> >
> > Passing a const char *path to basename(3) is a glibc specific
> > extension.
> >
> > Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
> > ---
> >  libsemanage/src/conf-parse.y | 3 +++
> >  libsemanage/src/direct_api.c | 3 +++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf-parse.y
> > index 6cb8a598..97cc5438 100644
> > --- a/libsemanage/src/conf-parse.y
> > +++ b/libsemanage/src/conf-parse.y
> > @@ -50,6 +50,9 @@ static external_prog_t *new_external;
> >  static int parse_errors;
> >
> >  #define PASSIGN(p1,p2) { free(p1); p1 = p2; }
> > +#if !defined(__GLIBC__)
> > +#define basename(src) (strrchr(src, '/') ? strrchr(src, '/') + 1 : src)
> > +#endif
> >
> >  %}
> >
> > diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> > index 99cba7f7..4459a7d7 100644
> > --- a/libsemanage/src/direct_api.c
> > +++ b/libsemanage/src/direct_api.c
> > @@ -63,6 +63,9 @@
> >  #define PIPE_READ 0
> >  #define PIPE_WRITE 1
> >  #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > +#if !defined(__GLIBC__)
> > +#define basename(src) (strrchr(src, '/') ? strrchr(src, '/') + 1 : src)
> > +#endif
> >
> >  static void semanage_direct_destroy(semanage_handle_t * sh);
> >  static int semanage_direct_disconnect(semanage_handle_t * sh);
> > --
> > 2.48.1
> >
>
> What system are you on where you run libsemange without glibc, just curious?
>
> I am not opposed to adding an implementation for basename where the
> input can be read only for non-glibc, but this patch doesn't work:
> Per the man page, https://man7.org/linux/man-pages/man3/basename.3.html,
> basename("/") should return "/", this one return "\0"
> basename("/usr/"); should return "usr", this returns "\0".
>
> There are two ways you could approach this:
> 1. If you wanted to do an implementation, I would add it to
> utilities.c/h and call it something other than basename so we don't
> get any odd issues with it choosing the one from the headers over the
> macro. Perhaps libsemange_basename(). On glibc systems this would just
> call basename directly and on non-glibc systems do the implementation
> with your own logic.
> 2. Just copy the path into a modifiable buffer on non-glibc systems
>
> I would do both approaches. Create a utility routine that calls
> basename for glibc and for non-glibc just copies it to a modifiable
> buffer and then calls basename over rolling our own and the bugs
> associated with it, also add a unit test for this. This would keep the
> logic in one place and be dirt simple.
>

Looking even more closely it's only the conf-parse.y usage where
selinux_policy_root() returns a const char *,
the usage in direct_api.c path is a char *, so we only need one spot
changed and that can just be a simple
copy to an intermediate buffer or am I missing something else here
you're pointing out?

> Bill

Re: [PATCH] libsemanage: define basename macro for non-glibc systems

[Rahul Sandhu]

> What system are you on where you run libsemange without glibc, just curious?

All Gentoo machines, Gentoo musl. :)

> I am not opposed to adding an implementation for basename where the
> input can be read only for non-glibc, but this patch doesn't work:
> Per the man page, https://man7.org/linux/man-pages/man3/basename.3.html,
> basename("/") should return "/", this one return "\0"
> basename("/usr/"); should return "usr", this returns "\0".

> There are two ways you could approach this:
> 1. If you wanted to do an implementation, I would add it to
> utilities.c/h and call it something other than basename so we don't
> get any odd issues with it choosing the one from the headers over the
> macro. Perhaps libsemange_basename(). On glibc systems this would just
> call basename directly and on non-glibc systems do the implementation
> with your own logic.
> 2. Just copy the path into a modifiable buffer on non-glibc systems
>
> I would do both approaches. Create a utility routine that calls
> basename for glibc and for non-glibc just copies it to a modifiable
> buffer and then calls basename over rolling our own and the bugs
> associated with it, also add a unit test for this. This would keep the
> logic in one place and be dirt simple.

FWIW, glibc's basename appears to be really trivial:

char *
__basename (const char *filename)
{
  char *p = strrchr (filename, '/');
  return p ? p + 1 : (char *) filename;
}

> selinux_policy_root() returns a const char *,
> the usage in direct_api.c path is a char *, so we only need one spot
> changed and that can just be a simple
> copy to an intermediate buffer or am I missing something else here
> you're pointing out?

Oh good point, we're just missing a header (libgen.h).

I suppose then it is just a simple copy to an intermediate buffer, I'll
send an updated patch shortly.

Thanks,
Rahul

Re: [PATCH] libsemanage: define basename macro for non-glibc systems

[Rahul Sandhu]

In regards to this:

> There are two ways you could approach this:
> 1. If you wanted to do an implementation, I would add it to
> utilities.c/h and call it something other than basename so we don't
> get any odd issues with it choosing the one from the headers over the
> macro. Perhaps libsemange_basename(). On glibc systems this would just
> call basename directly and on non-glibc systems do the implementation
> with your own logic.
> 2. Just copy the path into a modifiable buffer on non-glibc systems
>
> I would do both approaches. Create a utility routine that calls
> basename for glibc and for non-glibc just copies it to a modifiable
> buffer and then calls basename over rolling our own and the bugs
> associated with it, also add a unit test for this. This would keep the
> logic in one place and be dirt simple.

It appears that glibc's basename(3) has a different behaviour to the
version of basename(3) defined in posix. From the man page:

> The GNU version never modifies its argument, and returns the empty
> string when path has a trailing slash, and in particular also when it
> is "/".

So I think it might be best to just define an semanage_basename based
off the GNU behaviour as it is fairly trivial (being 2 LOC).

I'll send a patch to do this shortly.

Thanks,
Rahul

[PATCH v2] libsemanage: create semanage_basename to ensure posix compliance

[Rahul Sandhu]

Passing a const char * to basename(3) is a glibc-specific extension, so
create our own basename implementation. As it's a trivial 2 LOC, always
use our implementation of basename even if glibc is available to avoid
the complications of attaining the non-posix glibc implementation of
basename(3) as _GNU_SOURCE needs to be defined, but libgen.h also needs
to have not been included.

Also fix a missing check for selinux_policy_root(3). From the man page:
On failure, selinux_policy_root returns NULL.

As the glibc basename(3) (unlike posix basename(3)) does not support
having a nullptr passed to it, only pass the policy_root to basename(3)
if it is non-null.

Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
---
 libsemanage/src/conf-parse.y       | 13 ++++++++++---
 libsemanage/src/direct_api.c       |  1 +
 libsemanage/src/utilities.c        |  9 +++++++++
 libsemanage/src/utilities.h        | 13 +++++++++++++
 libsemanage/tests/test_utilities.c | 26 ++++++++++++++++++++++++++
 5 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf-parse.y
index 6cb8a598..d3ca5f1f 100644
--- a/libsemanage/src/conf-parse.y
+++ b/libsemanage/src/conf-parse.y
@@ -21,6 +21,7 @@
 %{
 
 #include "semanage_conf.h"
+#include "utilities.h"
 
 #include <sepol/policydb.h>
 #include <selinux/selinux.h>
@@ -382,7 +383,10 @@ external_opt:   PROG_PATH '=' ARG  { PASSIGN(new_external->path, $3); }
 static int semanage_conf_init(semanage_conf_t * conf)
 {
 	conf->store_type = SEMANAGE_CON_DIRECT;
-	conf->store_path = strdup(basename(selinux_policy_root()));
+	const char *policy_root = selinux_policy_root();
+	if (policy_root != NULL) {
+		conf->store_path = strdup(semanage_basename(policy_root));
+	}
 	conf->ignoredirs = NULL;
 	conf->store_root_path = strdup("/var/lib/selinux");
 	conf->compiler_directory_path = strdup("/usr/libexec/selinux/hll");
@@ -544,8 +548,11 @@ static int parse_module_store(char *arg)
 	free(current_conf->store_path);
 	if (strcmp(arg, "direct") == 0) {
 		current_conf->store_type = SEMANAGE_CON_DIRECT;
-		current_conf->store_path =
-		    strdup(basename(selinux_policy_root()));
+		const char *policy_root = selinux_policy_root();
+		if (policy_root != NULL) {
+			current_conf->store_path =
+			    strdup(semanage_basename(policy_root));
+		}
 		current_conf->server_port = -1;
 	} else if (*arg == '/') {
 		current_conf->store_type = SEMANAGE_CON_POLSERV_LOCAL;
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index 99cba7f7..ce12ccaf 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -26,6 +26,7 @@
 
 #include <assert.h>
 #include <fcntl.h>
+#include <libgen.h>
 #include <stdio.h>
 #include <stdio_ext.h>
 #include <stdlib.h>
diff --git a/libsemanage/src/utilities.c b/libsemanage/src/utilities.c
index 70b5b677..004ffb62 100644
--- a/libsemanage/src/utilities.c
+++ b/libsemanage/src/utilities.c
@@ -349,3 +349,12 @@ int write_full(int fd, const void *buf, size_t len)
 
 	return 0;
 }
+
+#ifdef __GNUC__
+__attribute__((nonnull))
+#endif
+char *semanage_basename(const char *filename)
+{
+	char *p = strrchr(filename, '/');
+	return p ? p + 1 : (char *)filename;
+}
diff --git a/libsemanage/src/utilities.h b/libsemanage/src/utilities.h
index c2d484a7..7481077a 100644
--- a/libsemanage/src/utilities.h
+++ b/libsemanage/src/utilities.h
@@ -156,4 +156,17 @@ semanage_list_t *semanage_slurp_file_filter(FILE * file,
 
 int write_full(int fd, const void *buf, size_t len) WARN_UNUSED;
 
+/**
+ * Portable implementation of the glibc version of basename(3).
+ *
+ * @param filename  path to find basename of
+ *
+ * @return          basename of filename
+ */
+
+#ifdef __GNUC__
+__attribute__((nonnull))
+#endif
+char *semanage_basename(const char *filename);
+
 #endif
diff --git a/libsemanage/tests/test_utilities.c b/libsemanage/tests/test_utilities.c
index bbd5af30..70a76fe7 100644
--- a/libsemanage/tests/test_utilities.c
+++ b/libsemanage/tests/test_utilities.c
@@ -46,6 +46,7 @@ static void test_semanage_rtrim(void);
 static void test_semanage_str_replace(void);
 static void test_semanage_findval(void);
 static void test_slurp_file_filter(void);
+static void test_semanage_basename(void);
 
 static char fname[] = {
 	'T', 'E', 'S', 'T', '_', 'T', 'E', 'M', 'P', '_', 'X', 'X', 'X', 'X',
@@ -117,6 +118,10 @@ int semanage_utilities_add_tests(CU_pSuite suite)
 				test_slurp_file_filter)) {
 		goto err;
 	}
+	if (NULL == CU_add_test(suite, "semanage_basename",
+				test_semanage_basename)) {
+		goto err;
+	}
 	return 0;
       err:
 	CU_cleanup_registry();
@@ -346,3 +351,24 @@ static void test_slurp_file_filter(void)
 
 	semanage_list_destroy(&data);
 }
+
+static void test_semanage_basename(void)
+{
+	char *basename1 = semanage_basename("/foo/bar");
+	CU_ASSERT_STRING_EQUAL(basename1, "bar");
+
+	char *basename2 = semanage_basename("/foo/bar/");
+	CU_ASSERT_STRING_EQUAL(basename2, "");
+
+	char *basename3 = semanage_basename("/foo.bar");
+	CU_ASSERT_STRING_EQUAL(basename3, "foo.bar");
+
+	char *basename5 = semanage_basename(".");
+	CU_ASSERT_STRING_EQUAL(basename5, ".");
+
+	char *basename6 = semanage_basename("");
+	CU_ASSERT_STRING_EQUAL(basename6, "");
+
+	char *basename7 = semanage_basename("/");
+	CU_ASSERT_STRING_EQUAL(basename7, "");
+}
-- 
2.48.1

Re: [PATCH v2] libsemanage: create semanage_basename to ensure posix compliance

[Rahul Sandhu]

Ping

Thanks,
Rahul

Re: [PATCH v2] libsemanage: create semanage_basename to ensure posix compliance

[James Carter]

On Fri, Feb 21, 2025 at 4:41 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
>
> Passing a const char * to basename(3) is a glibc-specific extension, so
> create our own basename implementation. As it's a trivial 2 LOC, always
> use our implementation of basename even if glibc is available to avoid
> the complications of attaining the non-posix glibc implementation of
> basename(3) as _GNU_SOURCE needs to be defined, but libgen.h also needs
> to have not been included.
>
> Also fix a missing check for selinux_policy_root(3). From the man page:
> On failure, selinux_policy_root returns NULL.
>
> As the glibc basename(3) (unlike posix basename(3)) does not support
> having a nullptr passed to it, only pass the policy_root to basename(3)
> if it is non-null.
>
> Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsemanage/src/conf-parse.y       | 13 ++++++++++---
>  libsemanage/src/direct_api.c       |  1 +
>  libsemanage/src/utilities.c        |  9 +++++++++
>  libsemanage/src/utilities.h        | 13 +++++++++++++
>  libsemanage/tests/test_utilities.c | 26 ++++++++++++++++++++++++++
>  5 files changed, 59 insertions(+), 3 deletions(-)
>
> diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf-parse.y
> index 6cb8a598..d3ca5f1f 100644
> --- a/libsemanage/src/conf-parse.y
> +++ b/libsemanage/src/conf-parse.y
> @@ -21,6 +21,7 @@
>  %{
>
>  #include "semanage_conf.h"
> +#include "utilities.h"
>
>  #include <sepol/policydb.h>
>  #include <selinux/selinux.h>
> @@ -382,7 +383,10 @@ external_opt:   PROG_PATH '=' ARG  { PASSIGN(new_external->path, $3); }
>  static int semanage_conf_init(semanage_conf_t * conf)
>  {
>         conf->store_type = SEMANAGE_CON_DIRECT;
> -       conf->store_path = strdup(basename(selinux_policy_root()));
> +       const char *policy_root = selinux_policy_root();
> +       if (policy_root != NULL) {
> +               conf->store_path = strdup(semanage_basename(policy_root));
> +       }
>         conf->ignoredirs = NULL;
>         conf->store_root_path = strdup("/var/lib/selinux");
>         conf->compiler_directory_path = strdup("/usr/libexec/selinux/hll");
> @@ -544,8 +548,11 @@ static int parse_module_store(char *arg)
>         free(current_conf->store_path);
>         if (strcmp(arg, "direct") == 0) {
>                 current_conf->store_type = SEMANAGE_CON_DIRECT;
> -               current_conf->store_path =
> -                   strdup(basename(selinux_policy_root()));
> +               const char *policy_root = selinux_policy_root();
> +               if (policy_root != NULL) {
> +                       current_conf->store_path =
> +                           strdup(semanage_basename(policy_root));
> +               }
>                 current_conf->server_port = -1;
>         } else if (*arg == '/') {
>                 current_conf->store_type = SEMANAGE_CON_POLSERV_LOCAL;
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index 99cba7f7..ce12ccaf 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -26,6 +26,7 @@
>
>  #include <assert.h>
>  #include <fcntl.h>
> +#include <libgen.h>
>  #include <stdio.h>
>  #include <stdio_ext.h>
>  #include <stdlib.h>
> diff --git a/libsemanage/src/utilities.c b/libsemanage/src/utilities.c
> index 70b5b677..004ffb62 100644
> --- a/libsemanage/src/utilities.c
> +++ b/libsemanage/src/utilities.c
> @@ -349,3 +349,12 @@ int write_full(int fd, const void *buf, size_t len)
>
>         return 0;
>  }
> +
> +#ifdef __GNUC__
> +__attribute__((nonnull))
> +#endif
> +char *semanage_basename(const char *filename)
> +{
> +       char *p = strrchr(filename, '/');
> +       return p ? p + 1 : (char *)filename;
> +}
> diff --git a/libsemanage/src/utilities.h b/libsemanage/src/utilities.h
> index c2d484a7..7481077a 100644
> --- a/libsemanage/src/utilities.h
> +++ b/libsemanage/src/utilities.h
> @@ -156,4 +156,17 @@ semanage_list_t *semanage_slurp_file_filter(FILE * file,
>
>  int write_full(int fd, const void *buf, size_t len) WARN_UNUSED;
>
> +/**
> + * Portable implementation of the glibc version of basename(3).
> + *
> + * @param filename  path to find basename of
> + *
> + * @return          basename of filename
> + */
> +
> +#ifdef __GNUC__
> +__attribute__((nonnull))
> +#endif
> +char *semanage_basename(const char *filename);
> +
>  #endif
> diff --git a/libsemanage/tests/test_utilities.c b/libsemanage/tests/test_utilities.c
> index bbd5af30..70a76fe7 100644
> --- a/libsemanage/tests/test_utilities.c
> +++ b/libsemanage/tests/test_utilities.c
> @@ -46,6 +46,7 @@ static void test_semanage_rtrim(void);
>  static void test_semanage_str_replace(void);
>  static void test_semanage_findval(void);
>  static void test_slurp_file_filter(void);
> +static void test_semanage_basename(void);
>
>  static char fname[] = {
>         'T', 'E', 'S', 'T', '_', 'T', 'E', 'M', 'P', '_', 'X', 'X', 'X', 'X',
> @@ -117,6 +118,10 @@ int semanage_utilities_add_tests(CU_pSuite suite)
>                                 test_slurp_file_filter)) {
>                 goto err;
>         }
> +       if (NULL == CU_add_test(suite, "semanage_basename",
> +                               test_semanage_basename)) {
> +               goto err;
> +       }
>         return 0;
>        err:
>         CU_cleanup_registry();
> @@ -346,3 +351,24 @@ static void test_slurp_file_filter(void)
>
>         semanage_list_destroy(&data);
>  }
> +
> +static void test_semanage_basename(void)
> +{
> +       char *basename1 = semanage_basename("/foo/bar");
> +       CU_ASSERT_STRING_EQUAL(basename1, "bar");
> +
> +       char *basename2 = semanage_basename("/foo/bar/");
> +       CU_ASSERT_STRING_EQUAL(basename2, "");
> +
> +       char *basename3 = semanage_basename("/foo.bar");
> +       CU_ASSERT_STRING_EQUAL(basename3, "foo.bar");
> +
> +       char *basename5 = semanage_basename(".");
> +       CU_ASSERT_STRING_EQUAL(basename5, ".");
> +
> +       char *basename6 = semanage_basename("");
> +       CU_ASSERT_STRING_EQUAL(basename6, "");
> +
> +       char *basename7 = semanage_basename("/");
> +       CU_ASSERT_STRING_EQUAL(basename7, "");
> +}
> --
> 2.48.1
>
>

Re: [PATCH v2] libsemanage: create semanage_basename to ensure posix compliance

[James Carter]

On Wed, Mar 19, 2025 at 3:29 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Fri, Feb 21, 2025 at 4:41 AM Rahul Sandhu <nvraxn@gmail.com> wrote:
> >
> > Passing a const char * to basename(3) is a glibc-specific extension, so
> > create our own basename implementation. As it's a trivial 2 LOC, always
> > use our implementation of basename even if glibc is available to avoid
> > the complications of attaining the non-posix glibc implementation of
> > basename(3) as _GNU_SOURCE needs to be defined, but libgen.h also needs
> > to have not been included.
> >
> > Also fix a missing check for selinux_policy_root(3). From the man page:
> > On failure, selinux_policy_root returns NULL.
> >
> > As the glibc basename(3) (unlike posix basename(3)) does not support
> > having a nullptr passed to it, only pass the policy_root to basename(3)
> > if it is non-null.
> >
> > Signed-off-by: Rahul Sandhu <nvraxn@gmail.com>
>
> Acked-by: James Carter <jwcart2@gmail.com>
>

Merged.
Thanks,
Jim

> > ---
> >  libsemanage/src/conf-parse.y       | 13 ++++++++++---
> >  libsemanage/src/direct_api.c       |  1 +
> >  libsemanage/src/utilities.c        |  9 +++++++++
> >  libsemanage/src/utilities.h        | 13 +++++++++++++
> >  libsemanage/tests/test_utilities.c | 26 ++++++++++++++++++++++++++
> >  5 files changed, 59 insertions(+), 3 deletions(-)
> >
> > diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf-parse.y
> > index 6cb8a598..d3ca5f1f 100644
> > --- a/libsemanage/src/conf-parse.y
> > +++ b/libsemanage/src/conf-parse.y
> > @@ -21,6 +21,7 @@
> >  %{
> >
> >  #include "semanage_conf.h"
> > +#include "utilities.h"
> >
> >  #include <sepol/policydb.h>
> >  #include <selinux/selinux.h>
> > @@ -382,7 +383,10 @@ external_opt:   PROG_PATH '=' ARG  { PASSIGN(new_external->path, $3); }
> >  static int semanage_conf_init(semanage_conf_t * conf)
> >  {
> >         conf->store_type = SEMANAGE_CON_DIRECT;
> > -       conf->store_path = strdup(basename(selinux_policy_root()));
> > +       const char *policy_root = selinux_policy_root();
> > +       if (policy_root != NULL) {
> > +               conf->store_path = strdup(semanage_basename(policy_root));
> > +       }
> >         conf->ignoredirs = NULL;
> >         conf->store_root_path = strdup("/var/lib/selinux");
> >         conf->compiler_directory_path = strdup("/usr/libexec/selinux/hll");
> > @@ -544,8 +548,11 @@ static int parse_module_store(char *arg)
> >         free(current_conf->store_path);
> >         if (strcmp(arg, "direct") == 0) {
> >                 current_conf->store_type = SEMANAGE_CON_DIRECT;
> > -               current_conf->store_path =
> > -                   strdup(basename(selinux_policy_root()));
> > +               const char *policy_root = selinux_policy_root();
> > +               if (policy_root != NULL) {
> > +                       current_conf->store_path =
> > +                           strdup(semanage_basename(policy_root));
> > +               }
> >                 current_conf->server_port = -1;
> >         } else if (*arg == '/') {
> >                 current_conf->store_type = SEMANAGE_CON_POLSERV_LOCAL;
> > diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> > index 99cba7f7..ce12ccaf 100644
> > --- a/libsemanage/src/direct_api.c
> > +++ b/libsemanage/src/direct_api.c
> > @@ -26,6 +26,7 @@
> >
> >  #include <assert.h>
> >  #include <fcntl.h>
> > +#include <libgen.h>
> >  #include <stdio.h>
> >  #include <stdio_ext.h>
> >  #include <stdlib.h>
> > diff --git a/libsemanage/src/utilities.c b/libsemanage/src/utilities.c
> > index 70b5b677..004ffb62 100644
> > --- a/libsemanage/src/utilities.c
> > +++ b/libsemanage/src/utilities.c
> > @@ -349,3 +349,12 @@ int write_full(int fd, const void *buf, size_t len)
> >
> >         return 0;
> >  }
> > +
> > +#ifdef __GNUC__
> > +__attribute__((nonnull))
> > +#endif
> > +char *semanage_basename(const char *filename)
> > +{
> > +       char *p = strrchr(filename, '/');
> > +       return p ? p + 1 : (char *)filename;
> > +}
> > diff --git a/libsemanage/src/utilities.h b/libsemanage/src/utilities.h
> > index c2d484a7..7481077a 100644
> > --- a/libsemanage/src/utilities.h
> > +++ b/libsemanage/src/utilities.h
> > @@ -156,4 +156,17 @@ semanage_list_t *semanage_slurp_file_filter(FILE * file,
> >
> >  int write_full(int fd, const void *buf, size_t len) WARN_UNUSED;
> >
> > +/**
> > + * Portable implementation of the glibc version of basename(3).
> > + *
> > + * @param filename  path to find basename of
> > + *
> > + * @return          basename of filename
> > + */
> > +
> > +#ifdef __GNUC__
> > +__attribute__((nonnull))
> > +#endif
> > +char *semanage_basename(const char *filename);
> > +
> >  #endif
> > diff --git a/libsemanage/tests/test_utilities.c b/libsemanage/tests/test_utilities.c
> > index bbd5af30..70a76fe7 100644
> > --- a/libsemanage/tests/test_utilities.c
> > +++ b/libsemanage/tests/test_utilities.c
> > @@ -46,6 +46,7 @@ static void test_semanage_rtrim(void);
> >  static void test_semanage_str_replace(void);
> >  static void test_semanage_findval(void);
> >  static void test_slurp_file_filter(void);
> > +static void test_semanage_basename(void);
> >
> >  static char fname[] = {
> >         'T', 'E', 'S', 'T', '_', 'T', 'E', 'M', 'P', '_', 'X', 'X', 'X', 'X',
> > @@ -117,6 +118,10 @@ int semanage_utilities_add_tests(CU_pSuite suite)
> >                                 test_slurp_file_filter)) {
> >                 goto err;
> >         }
> > +       if (NULL == CU_add_test(suite, "semanage_basename",
> > +                               test_semanage_basename)) {
> > +               goto err;
> > +       }
> >         return 0;
> >        err:
> >         CU_cleanup_registry();
> > @@ -346,3 +351,24 @@ static void test_slurp_file_filter(void)
> >
> >         semanage_list_destroy(&data);
> >  }
> > +
> > +static void test_semanage_basename(void)
> > +{
> > +       char *basename1 = semanage_basename("/foo/bar");
> > +       CU_ASSERT_STRING_EQUAL(basename1, "bar");
> > +
> > +       char *basename2 = semanage_basename("/foo/bar/");
> > +       CU_ASSERT_STRING_EQUAL(basename2, "");
> > +
> > +       char *basename3 = semanage_basename("/foo.bar");
> > +       CU_ASSERT_STRING_EQUAL(basename3, "foo.bar");
> > +
> > +       char *basename5 = semanage_basename(".");
> > +       CU_ASSERT_STRING_EQUAL(basename5, ".");
> > +
> > +       char *basename6 = semanage_basename("");
> > +       CU_ASSERT_STRING_EQUAL(basename6, "");
> > +
> > +       char *basename7 = semanage_basename("/");
> > +       CU_ASSERT_STRING_EQUAL(basename7, "");
> > +}
> > --
> > 2.48.1
> >
> >