Re: [PATCH v3] trace-cmd: Add option to execute traced process as given user

[Slavomir Kaslev <kaslevs@vmware.com>]

On Tue, 2019-07-09 at 17:29 +0300, Tzvetomir Stoyanov (VMware) wrote:
> A new trace-cmd record option is added: "--user". If a comnmand is
> specified
> as trce-cmd argument, it is executed in the context of the given
> user.
> 
> Suggested-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
> [
>    v3 changes:
>       "--user" does not depend on option -F, it works with any
> command,
>        specified as trace-cmd argument.
>    v2 changes:
>      - Check for errors in change_user(). If an error occurs while
>        changing the user, the message is printed and the traced 
>        process is not executed.
> ]
> 
> 
>  Documentation/trace-cmd-record.1.txt |  4 +++
>  tracecmd/trace-record.c              | 47
> ++++++++++++++++++++++++++--
>  tracecmd/trace-usage.c               |  1 +
>  3 files changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/trace-cmd-record.1.txt
> b/Documentation/trace-cmd-record.1.txt
> index 4a59de9..6a0f975 100644
> --- a/Documentation/trace-cmd-record.1.txt
> +++ b/Documentation/trace-cmd-record.1.txt
> @@ -122,6 +122,10 @@ OPTIONS
>  *--mmap*::
>       Used with either *-F* or *-P*, save the traced process memory
> map into
>       the trace.dat file.
> +
> +*--user*::
> +     Execute the specified *command* as given user.
> +
>  *-C* 'clock'::
>       Set the trace clock to "clock".
>  
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 48081d4..acadd7a 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -33,6 +33,8 @@
>  #include <errno.h>
>  #include <limits.h>
>  #include <libgen.h>
> +#include <pwd.h>
> +#include <grp.h>
>  
>  #include "version.h"
>  #include "trace-local.h"
> @@ -208,6 +210,7 @@ struct common_record_context {
>  	struct buffer_instance *instance;
>  	const char *output;
>  	char *date2ts;
> +	char *user;
>  	int data_flags;
>  
>  	int record_all;
> @@ -1417,7 +1420,34 @@ static void trace_or_sleep(enum trace_type
> type)
>  		sleep(10);
>  }
>  
> -static void run_cmd(enum trace_type type, int argc, char **argv)
> +static int change_user(char *user)

Nit: `const char *user` would be more appropriate.

> +{
> +	struct passwd *pwd;
> +
> +	if (!user)
> +		return 0;
> +
> +	pwd = getpwnam(user);
> +	if (!pwd)
> +		return -1;
> +	if (initgroups(user, pwd->pw_gid) < 0)
> +		return -1;
> +	if (setgid(pwd->pw_gid) < 0)
> +		return -1;
> +	if (setuid(pwd->pw_uid) < 0)
> +		return -1;
> +
> +	if (setenv("HOME", pwd->pw_dir, 1) < 0)
> +		return -1;
> +	if (setenv("USER", pwd->pw_name, 1) < 0)
> +		return -1;
> +	if (setenv("LOGNAME", pwd->pw_name, 1) < 0)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static void run_cmd(enum trace_type type, char *user, int argc, char
> **argv)

Nit: same as above.

>  {
>  	int status;
>  	int pid;
> @@ -1438,6 +1468,10 @@ static void run_cmd(enum trace_type type, int
> argc, char **argv)
>  			dup2(save_stdout, 1);
>  			close(save_stdout);
>  		}
> +
> +		if (change_user(user) < 0)
> +			die("Failed to change user to %s", user);
> +
>  		if (execvp(argv[0], argv)) {
>  			fprintf(stderr, "\n********************\n");
>  			fprintf(stderr, " Unable to exec %s\n",
> argv[0]);
> @@ -4548,6 +4582,7 @@ void update_first_instance(struct
> buffer_instance *instance, int topt)
>  }
>  
>  enum {
> +	OPT_user		= 243,
>  	OPT_mmap		= 244,
>  	OPT_quiet		= 245,
>  	OPT_debug		= 246,
> @@ -4780,6 +4815,7 @@ static void parse_record_options(int argc,
>  			{"quiet", no_argument, NULL, OPT_quiet},
>  			{"help", no_argument, NULL, '?'},
>  			{"mmap", no_argument, NULL, OPT_mmap},
> +			{"user", required_argument, NULL, OPT_user},
>  			{"module", required_argument, NULL,
> OPT_module},
>  			{NULL, 0, NULL, 0}
>  		};
> @@ -5011,6 +5047,9 @@ static void parse_record_options(int argc,
>  		case 'i':
>  			ignore_event_not_found = 1;
>  			break;
> +		case OPT_user:
> +			ctx->user = strdup(optarg);

This is missing a check for allocation failure.

Other than that, looks great.

Reviewed-by: Slavomir Kaslev <kaslevs@vmware.com>

Cheers,

-- Slavi