[valgrind] [Bug 506813] execveat wrapper need to do more checking

[Mark Wielaard]

https://bugs.kde.org/show_bug.cgi?id=506813

--- Comment #4 from Mark Wielaard <m...@klomp.org> ---
> +       } else {
> +           if (ARG5 & VKI_AT_SYMLINK_NOFOLLOW) {
> +              struct vg_stat statbuf;
> +              SysRes statres;
> +              statres = VG_(stat)(path, &statbuf);
> +              if (sr_isError(statres) || VKI_S_ISLNK(statbuf.mode)) {
> +                 SET_STATUS_Failure( VKI_ELOOP );
> +                 return;
> +              }
> +           }
> +           if(ARG5 & ~(VKI_AT_SYMLINK_NOFOLLOW | VKI_AT_EMPTY_PATH)) {
> +              SET_STATUS_Failure( VKI_EINVAL );
> +              return;
> +           }

Yes, this is the correct check to do.

But after reading this code twice now (see also bug #506806) I believe the
if/else logic here is somewhat wonky.
The exact same code is there slightly upward, but then for relative paths
(path[0] != '/').

Inside the relative path block there is some funky logic that probably
shouldn't be there.
And the AT_SYMLINK_NOFOLLOW logic should probably be only afterwards (the same
for absolute and relative paths).

I think the check for the relative path check can be reduced to:

         if (arg_1 != VKI_AT_FDCWD
             && !ML_(fd_allowed)(arg_1, "execveat", tid, False) && arg_1) {
            SET_STATUS_Failure( VKI_EBADF );
            return;
         }
         /* If pathname is empty and AT_EMPTY_PATH is 
            set then dirfd describes the whole path. */
         if (path[0] == '\0') {
            if (ARG5 & VKI_AT_EMPTY_PATH) {
               if (VG_(resolve_filename)(arg_1, &buf)) {
                  path = buf;
                  check_pathptr = False;
               }
            }
         } else if (VG_(resolve_filename)(arg_1, &buf)) {
            abs_path = VG_(malloc)("execveat",
                                   (VG_(strlen)(buf) + 1
                                    + VG_(strlen)(path) + 1)); 
            VG_(sprintf)(abs_path, "%s/%s", buf, path);
            path = abs_path;
            check_pathptr = False;
         }

And then you do the VKI_AT_SYMLINK_NOFOLLOW and ~(VKI_AT_SYMLINK_NOFOLLOW |
VKI_AT_EMPTY_PATH) just for both relative and absolute (the relative path will
have been made absolute at this point).

Hope this makes sense.

-- 
You are receiving this mail because:
You are watching all bug changes.