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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <4962b96d-d2d8-429d-9794-120349741a58@linuxfoundation.org>
Date: Fri, 21 Jun 2024 15:04:03 -0600
From: Shuah Khan <skhan@...uxfoundation.org>
To: Roman Storozhenko <romeusmeister@...il.com>
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, Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH] cpupower: Make help command available for custom install
 dir

On 6/21/24 12:18, Roman Storozhenko wrote:
> 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.
> 

How about setting MANPATH before running the command?

thanks,
-- Shuah