linux-kernel - Re: [PATCH] cpupower: Make help command available for custom install dir

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALsPMBPd1O2zBxyvfTJiPvuWO+zaTxrYizmiKULr1A-77ME_Rw@mail.gmail.com>
Date: Fri, 21 Jun 2024 20:18:50 +0200
From: Roman Storozhenko <romeusmeister@...il.com>
To: Shuah Khan <skhan@...uxfoundation.org>
Cc: Thomas Renninger <trenn@...e.com>, Shuah Khan <shuah@...nel.org>, 
	Javier Carrasco <javier.carrasco.cruz@...il.com>, linux-pm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cpupower: Make help command available for custom install dir

On Fri, Jun 21, 2024 at 5:02 PM Shuah Khan <skhan@...uxfoundation.org> wrote:
>
> On 6/21/24 02:13, Roman Storozhenko wrote:
> > When the 'cpupower' utility installed in the custom dir, it fails to
> > render appopriate help info for a particular subcommand:
>
> appopriate -> appropriate
> Spell check the commit message.

Thanks for highlighting this, will fix.
>
> > $ LD_LIBRARY_PATH=lib64/ bin/cpupower help monitor
> > with error message like 'No manual entry for cpupower-monitor.1'
> > The issue is that under the hood it calls 'exec' function with
> > the following args: 'man cpupower-monitor.1'. In turn, 'man' search
> > path is defined in '/etc/manpath.config'. Of course it contains only
> > standard system man paths.
> > Make subcommands man pages available for user using the following rule:
> > Render a man page if it is installed in the custom install dir, otherwise
> > allow man to search this page by name system-wide as a last resort.
> >
>
> Good find.
>
> > Signed-off-by: Roman Storozhenko <romeusmeister@...il.com>
> > ---
> >   tools/power/cpupower/utils/cpupower.c | 54 ++++++++++++++++++++++++++++++-----
> >   1 file changed, 47 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/power/cpupower/utils/cpupower.c b/tools/power/cpupower/utils/cpupower.c
> > index 9ec973165af1..da4bc6de7494 100644
> > --- a/tools/power/cpupower/utils/cpupower.c
> > +++ b/tools/power/cpupower/utils/cpupower.c
> > @@ -12,6 +12,8 @@
> >   #include <unistd.h>
> >   #include <errno.h>
> >   #include <sched.h>
> > +#include <libgen.h>
> > +#include <limits.h>
> >   #include <sys/types.h>
> >   #include <sys/stat.h>
> >   #include <sys/utsname.h>
> > @@ -21,6 +23,8 @@
> >   #include "helpers/bitmask.h"
> >
> >   #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
> > +#define MAN_REL_PATH "/../man/man1/"
> > +#define MAN_SUFFIX ".1"
> >
> >   static int cmd_help(int argc, const char **argv);
> >
> > @@ -80,14 +84,17 @@ static void print_help(void)
> >
> >   static int print_man_page(const char *subpage)
> >   {
> > -     int len;
> > -     char *page;
> > +     char *page, *man_path, *exec_dir;
> > +     char exec_path[PATH_MAX];
> > +     int subpage_len;
> >
> > -     len = 10; /* enough for "cpupower-" */
> > -     if (subpage != NULL)
> > -             len += strlen(subpage);
> > +     if (!subpage)
> > +             return -EINVAL;
> >
> > -     page = malloc(len);
> > +     subpage_len = 10; /* enough for "cpupower-" */
> > +     subpage_len += strlen(subpage);
> > +
> > +     page = malloc(subpage_len);
> >       if (!page)
> >               return -ENOMEM;
> >
> > @@ -97,7 +104,40 @@ static int print_man_page(const char *subpage)
> >               strcat(page, subpage);
> >       }
> >
> > -     execlp("man", "man", page, NULL);
> > +     /* Get current process image name full path */
> > +     if (readlink("/proc/self/exe", exec_path, PATH_MAX) > 0) {
> > +
> > +             man_path = malloc(PATH_MAX);
> > +             if (!man_path) {
> > +                     free(page);
> > +                     return -ENOMEM;
> > +             }
> > +
> > +             exec_dir = strdup(exec_path);
> > +             if (!exec_dir) {
> > +                     free(page);
> > +                     free(man_path);
> > +                     return -ENOMEM;
> > +             }
> > +
> > +             *man_path = '\0';
> > +             strncat(man_path, dirname(exec_dir), strlen(exec_dir));
> > +             strncat(man_path, MAN_REL_PATH, strlen(MAN_REL_PATH));
> > +             strncat(man_path, page, strlen(page));
> > +             strncat(man_path, MAN_SUFFIX, strlen(MAN_SUFFIX));
> > +
> > +             free(exec_dir);
> > +
> > +             /* Check if file exists */
> > +             if (access(man_path, F_OK) == -1) {
> > +                     free(man_path);
> > +                     man_path = page;
> > +             }
> > +     } else {
> > +             man_path = page;
> > +     }
> > +
> > +     execlp("man", "man", man_path, NULL);
>
> You can simplify all of this by using getenv() to get the environment
> variables for the program.
>
> Take a look getenv() usages in the kernel sources for reference.

If you mean that I can extract the current working directory and then add
relative path to man page to it then yes, I can. But the issue with
this approach
is that this will work only if I run the binary from its directory,
otherwise it fail,
because current_working_directory is not the image_path. And there is no
environment variable which defines the path to the process's binary.
Just in case, I looked to the kernel sources which use getenv() in userspace and
also examined the list of the POSIX environment variables:
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html
And nothing came to my mind in terms of simplification.
So, please suggest me what I could change.

> >
>
> thanks,
> -- Shuah
>


-- 
Kind regards,
Roman Storozhenko