RFC: path display and path filtering features.

[Dmitry V. Levin]

Hi,

On Wed, Feb 23, 2011 at 02:16:43PM -0600, Grant Edwards wrote:
> Attached are my current diffs. I've merged recent changes from the
> main git repository and cleaned up loose ends.
> 
> The general approach is:
> 
>  1) Store -P arguments both as-is and canonicalized.
> 
>  2) Compare stored paths against /proc/$pid/fd/$fd for fd arguments.
> 
>  3) Compare stored paths against path arguments.

Thanks, it works.  I think it could be merged after some cleanup and
re-indentation.

> Things on the possible (near-term) todo list:
> 
>  1) Canonicalize relative path args based on /proc/$pid/cwd
> 
>  2) Handle combination fd/path args such as those passed to openat()

Let's try simple combination without path canonicalization in both cases.
I'm still not quite comfortable with applying realpath(3) to user data,
from security PoV.  Maybe this kind of canonicalization should be enabled
only when requested explicitly, e.g. via special option.

>  3) Handle non-simple fd args like fd_set args passed to select() or
>     the array of pollfd structs passed to poll().

Agreed.

Now a few comments on the patch itself.

> diff --git a/.gitignore b/.gitignore
> index 9f17271..7828bad 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -4,6 +4,8 @@
>  .libs
>  .*.swp
>  
> +.dir-locals.el
> +
>  ChangeLog
>  
>  Makefile

This has nothing to do with new path features.
Why this change is needed?

> @@ -449,6 +450,7 @@ struct tcb {
>  #define syserror(tcp)	((tcp)->u_error != 0)
>  #define verbose(tcp)	(qual_flags[(tcp)->scno] & QUAL_VERBOSE)
>  #define abbrev(tcp)	(qual_flags[(tcp)->scno] & QUAL_ABBREV)
> +#define filtered(tcp)   ((tcp)->flags & TCB_FILTERED)

The indentation of the line being added differs from neighbouring lines.

> @@ -702,3 +704,11 @@ extern long ia32;
>  #endif
>  
>  extern int not_failing_only;
> +extern int show_fd_path;
> +extern int tracing_paths;
> +
> +extern int  pathtrace_select(char *path);
> +extern int  pathtrace_match(struct tcb *tcp);
> +extern void printfd(struct tcb* tcp, int fd);
> +
> +

No empty lines at the end, please.

> diff --git a/desc.c b/desc.c
> index 2b9f30a..ebbcf57 100644
> --- a/desc.c
> +++ b/desc.c
> @@ -309,7 +309,8 @@ int
>  sys_fcntl(struct tcb *tcp)
>  {
>  	if (entering(tcp)) {
> -		tprintf("%ld, ", tcp->u_arg[0]);
> +		printfd(tcp,tcp->u_arg[0]);
> +		tprintf(", ");
>  		printxval(fcntlcmds, tcp->u_arg[1], "F_???");
>  		switch (tcp->u_arg[1]) {
>  		case F_SETFD:

In this and other similar cases, a comma between function arguments is
necessary.

> @@ -586,14 +591,16 @@ sys_llseek(struct tcb *tcp)
>  		 * rather than one 64-bit argument for which LONG_LONG works
>  		 * appropriate for the native byte order.
>  		 */
> -		if (tcp->u_arg[4] == SEEK_SET)
> -			tprintf("%ld, %llu, ", tcp->u_arg[0],
> -				(((long long int) tcp->u_arg[1]) << 32
> -				 | (unsigned long long) (unsigned) tcp->u_arg[2]));
> -		else
> -			tprintf("%ld, %lld, ", tcp->u_arg[0],
> -				(((long long int) tcp->u_arg[1]) << 32
> -				 | (unsigned long long) (unsigned) tcp->u_arg[2]));
> +		if (tcp->u_arg[4] == SEEK_SET) {
> +			printfd(tcp,tcp->u_arg[0]);
> +			tprintf(", %llu, ", (((long long int) tcp->u_arg[1]) << 32
> +					     | (unsigned long long) (unsigned) tcp->u_arg[2]));
> +		}
> +		else {
> +			printfd(tcp,tcp->u_arg[0]);
> +			tprintf(", %lld, ", (((long long int) tcp->u_arg[1]) << 32
> +					     | (unsigned long long) (unsigned) tcp->u_arg[2]));
> +		}
>  	}
>  	else {
>  		long long int off;

Indentation before the change was better.

> @@ -229,9 +235,10 @@ sys_sendfile(tcp)
>  struct tcb *tcp;
>  {
>  	if (entering(tcp)) {
> -		tprintf("%ld, %ld, %llu, %lu", tcp->u_arg[0], tcp->u_arg[1],
> -			LONG_LONG(tcp->u_arg[2], tcp->u_arg[3]),
> -			tcp->u_arg[4]);
> +		printfd(tcp,tcp->u_arg[0]);
> +		tprintf(", ");
> +		printfd(tcp,tcp->u_arg[1]);
> +		tprintf(", %llu, %lu", LONG_LONG(tcp->u_arg[2], tcp->u_arg[3]),	tcp->u_arg[4]);
>  	} else {
>  		off_t offset;
>  

Likewise.

> diff --git a/pathtrace.c b/pathtrace.c
> new file mode 100644
> index 0000000..70babff
> --- /dev/null
> +++ b/pathtrace.c
> @@ -0,0 +1,304 @@

I suggest to apply code indentation of quota.c for new files.
This file is indented very different from quota.c

> +// get path associated with pid,fd tuple
> +static char *getpath(pid_t pid, int fd)
> +{
> +  static char path[PATH_MAX+1];
> +  char linkpath[64];
> +  ssize_t n;
> +
> +  if (fd<0)
> +    return NULL;
> +
> +  snprintf(linkpath, sizeof linkpath, "/proc/%d/fd/%d", pid, fd);
> +  n = readlink(linkpath, path, (sizeof path) - 0);

It has to be sizeof(path) - 1 instead.

> +// Add a path to the set we're tracing.  Also add the canonicalized
> +// versio of the path.  Secifying NULL will delete all paths.
> +int pathtrace_select(char *path)
> +{
> +  char *rpath;
> +
> +  if (path==NULL)
> +    return storepath(path);
> +
> +  if (storepath(path))
> +    return -1;
> +
> +  rpath = realpath(path,NULL);
> +
> +  if (rpath == NULL)
> +    {
> +      // not an error if user specifies non-existent file/path
> +      if (errno == ENOENT || errno == ENOTDIR)
> +        return 0;
> +      perror("Failed to resolve path");
> +      return -1;
> +    }

There are more potential error codes, e.g. EACCES, ELOOP and ENAMETOOLONG,
that should not be treated as errors.  Maybe strace should tolerate any
realpath errors.

> +
> +  // if realpath and specified path are same, we're done
> +  if (!strcmp(path,rpath))
> +    {
> +      free(rpath);
> +      return 0;
> +    }
> +
> +  tprintf("Requested path '%s' resolved into '%s'\n",path,rpath);

All debug messages should go to stderr.

> @@ -1256,7 +1274,7 @@ proc_open(struct tcb *tcp, int attaching)
>  #else /* FREEBSD */
>  	/* just unset the PF_LINGER flag for the Run-on-Last-Close. */
>  	if (ioctl(tcp->pfd, PIOCGFL, &arg) < 0) {
> -	        perror("PIOCGFL");
> +		perror("PIOCGFL");
>  		return -1;
>  	}
>  	arg &= ~PF_LINGER;

In this and other cases, please avoid re-indentation of unrelated code.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20110225/32a0e6e4/attachment.bin>