| Message ID | 20250220211249.574456-1-nvraxn@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | libsemanage: define basename macro for non-glibc systems | expand |
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
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
> 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
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
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);
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(+)