RFC: path display and path filtering features.

[Grant Edwards]

On 2011-02-24, Dmitry V. Levin <ldv at altlinux.org> 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.

OK.

> 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.

That wouldn't be difficult.  Is it something we want to do now?

>>  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?

Oops.  I added that so that the file used to tell emacs how to indent
the source files would be ignored by git.  I didn't mean to include it
in the patch.

>
>> @@ -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.

I'll fix that.

>> @@ -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.

Roger.

>> 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.

While I understand that sentence, I don't undestand how it is meant as
a comment on the diff above it.  Can you explain?

>> @@ -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.

Sorry about that, I'll fix it.

>> @@ -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.

I'll fix that too.

>> 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

I planned on re-indenting it after I was done working on it.  Since I
haven't found any tools/settings that produce the indentation you
desire, it's pretty difficult for me to maintain your desired
indentation while working on files.  After we've decided on the actual
code I'll fix the indentation.  Is there a set of options for ident or
astyle or some other tool that will produce the indentation you want?

>> +// 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.

Of course.

>> +// 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.

OK.

>> +
>> +  // 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.

I've been trying to, but it's difficult when the indentation is so
inconsistent (different styles in different files, spaces in some
places within a file tabs in others).

I don't know about others, but it I would find it a lot easier to work
on the source files if they all had a consistent, standard indentation
style that could be produced by a tool like indent or astyle (I don't
particulary care what standard or what tool).

-- 
Grant Edwards               grant.b.edwards        Yow! !  Up ahead!  It's a
                                  at               DONUT HUT!!
                              gmail.com