[Qemu-devel] [PATCH] Fix utimensat (aka unbreak cp -a)

[Qemu-devel] [PATCH] Fix utimensat (aka unbreak cp -a)

[Arnaud Patard]

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]



Don't use the glibc function for utimensat because glibc returns -EINVAL
if the path is null which is a different behaviour with the syscall.
path can be null because internally the glibc is using utimensat with
path null (for instance, see __futimes in
sysdeps/unix/sysv/linux/futimes.c in glibc tree).


Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
---

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: utimensat.patch --]
[-- Type: text/x-diff, Size: 1025 bytes --]

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b6dc6cc..6959da0 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -412,13 +412,6 @@ static int sys_unlinkat(int dirfd, const char *pathname, int flags)
   return (unlinkat(dirfd, pathname, flags));
 }
 #endif
-#ifdef TARGET_NR_utimensat
-static int sys_utimensat(int dirfd, const char *pathname,
-    const struct timespec times[2], int flags)
-{
-  return (utimensat(dirfd, pathname, times, flags));
-}
-#endif
 #else /* !CONFIG_ATFILE */
 
 /*
@@ -478,12 +471,12 @@ _syscall3(int,sys_symlinkat,const char *,oldpath,
 #if defined(TARGET_NR_unlinkat) && defined(__NR_unlinkat)
 _syscall3(int,sys_unlinkat,int,dirfd,const char *,pathname,int,flags)
 #endif
+#endif /* CONFIG_ATFILE */
 #if defined(TARGET_NR_utimensat) && defined(__NR_utimensat)
 _syscall4(int,sys_utimensat,int,dirfd,const char *,pathname,
           const struct timespec *,tsp,int,flags)
 #endif
 
-#endif /* CONFIG_ATFILE */
 
 #ifdef CONFIG_INOTIFY
 #include <sys/inotify.h>

Re: [Qemu-devel] [PATCH] Fix utimensat (aka unbreak cp -a)

[Laurent Desnogues]

On Tue, Apr 21, 2009 at 10:24 AM, Arnaud Patard
<arnaud.patard@rtp-net.org> wrote:
>
>
> Don't use the glibc function for utimensat because glibc returns -EINVAL
> if the path is null which is a different behaviour with the syscall.
> path can be null because internally the glibc is using utimensat with
> path null (for instance, see __futimes in
> sysdeps/unix/sysv/linux/futimes.c in glibc tree).
>
>
> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>

Acked-by: Laurent Desnogues <laurent.desnogues@gmail.com>

Re: [Qemu-devel] [PATCH] Fix utimensat (aka unbreak cp -a)

[Riku Voipio]

On Tue, Apr 21, 2009 at 10:24:03AM +0200, Arnaud Patard wrote:
> Don't use the glibc function for utimensat because glibc returns -EINVAL
> if the path is null which is a different behaviour with the syscall.
> path can be null because internally the glibc is using utimensat with
> path null (for instance, see __futimes in
> sysdeps/unix/sysv/linux/futimes.c in glibc tree).

Soo.. glibc uses utimensat to implement futimens, but doesn't allow
applications to use utimensat in the same way. What a mess.

But if we are to go towards using libc calls, your patch is a step
backwards. In case pathname is null, we can use futimens.


commit 0f34ff059fb9ad6e8c8fa5161d9d17265286bb62
Author: Riku Voipio <riku.voipio@iki.fi>
Date:   Tue Apr 21 15:01:51 2009 +0300

    linux-user: fix utimensat when used as futimens
    
    The glibc function for utimensat glibc returns -EINVAL when the path is null
    which is a different behaviour with the syscall.
    
    path can be null because internally the glibc is using utimensat with
    path null when implmenting futimens. If path is null, call futimes
    instead.

diff --git a/configure b/configure
index 08df436..fc980a4 100755
--- a/configure
+++ b/configure
@@ -1208,6 +1208,25 @@ EOF
   fi
 fi
 
+# check if utimensat and futimens are supported
+utimens=no
+cat > $TMPC << EOF
+#define _ATFILE_SOURCE
+#define _GNU_SOURCE
+#include <stddef.h>
+#include <fcntl.h>
+
+int main(void)
+{
+    utimensat(AT_FDCWD, "foo", NULL, 0);
+    futimens(0, NULL);
+    return 0;
+}
+EOF
+if $cc $ARCH_CFLAGS -o $TMPE $TMPC 2> /dev/null ; then
+  utimens=yes
+fi
+
 # Check if tools are available to build documentation.
 if [ -x "`which texi2html 2>/dev/null`" ] && \
    [ -x "`which pod2man 2>/dev/null`" ]; then
@@ -1604,6 +1623,9 @@ fi
 if test "$atfile" = "yes" ; then
   echo "#define CONFIG_ATFILE 1" >> $config_h
 fi
+if test "$utimens" = "yes" ; then
+  echo "#define CONFIG_UTIMENSAT 1" >> $config_h
+fi
 if test "$inotify" = "yes" ; then
   echo "#define CONFIG_INOTIFY 1" >> $config_h
 fi
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 408ccc6..4dad5c1 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -406,13 +406,6 @@ static int sys_unlinkat(int dirfd, const char *pathname, int flags)
   return (unlinkat(dirfd, pathname, flags));
 }
 #endif
-#ifdef TARGET_NR_utimensat
-static int sys_utimensat(int dirfd, const char *pathname,
-    const struct timespec times[2], int flags)
-{
-  return (utimensat(dirfd, pathname, times, flags));
-}
-#endif
 #else /* !CONFIG_ATFILE */
 
 /*
@@ -476,12 +469,24 @@ _syscall3(int,sys_symlinkat,const char *,oldpath,
 #if defined(TARGET_NR_unlinkat) && defined(__NR_unlinkat)
 _syscall3(int,sys_unlinkat,int,dirfd,const char *,pathname,int,flags)
 #endif
+
+#endif /* CONFIG_ATFILE */
+
+#ifdef CONFIG_UTIMENSAT
+static int sys_utimensat(int dirfd, const char *pathname,
+    const struct timespec times[2], int flags)
+{
+    if (pathname == NULL)
+        return futimens(dirfd, times);
+    else
+        return utimensat(dirfd, pathname, times, flags);
+}
+#else
 #if defined(TARGET_NR_utimensat) && defined(__NR_utimensat)
 _syscall4(int,sys_utimensat,int,dirfd,const char *,pathname,
           const struct timespec *,tsp,int,flags)
 #endif
-
-#endif /* CONFIG_ATFILE */
+#endif /* CONFIG_UTIMENSAT  */
 
 #ifdef CONFIG_INOTIFY
 #include <sys/inotify.h>

Re: [Qemu-devel] [PATCH] Fix utimensat (aka unbreak cp -a)

[Jamie Lokier]

Riku Voipio wrote:
> Soo.. glibc uses utimensat to implement futimens, but doesn't allow
> applications to use utimensat in the same way. What a mess.

It protects against apps passing in NULL path due to a bug and
touching the directory by accident.  A minor bit of protection indeed.

> But if we are to go towards using libc calls, your patch is a step
> backwards. In case pathname is null, we can use futimens.

I agree, good idea.

-- Jamie

Re: [Qemu-devel] [PATCH] Fix utimensat (aka unbreak cp -a)

[Martin Mohring]

i found another related case it seems:

when i call in a chroot under arm with qemu user mode:

$ touch /var/log/wtmp

the resulting file has a date/time of 1.1.1970, although the clock is
correct in the system.

calling

$ touch /var/log/wtmp /var/run/utmp /var/log/btmp
/bin/touch: setting times of `/var/run/utmp': Invalid argument
/bin/touch: setting times of `/var/log/btmp': Invalid argument

results in the invalid argutment error. I ll currently check which
syscalls are involved here.
the time of 1.1.1970 looks to me like a wrongly passed argument (of ==0).

Riku Voipio wrote:
> On Tue, Apr 21, 2009 at 10:24:03AM +0200, Arnaud Patard wrote:
>   
>> Don't use the glibc function for utimensat because glibc returns -EINVAL
>> if the path is null which is a different behaviour with the syscall.
>> path can be null because internally the glibc is using utimensat with
>> path null (for instance, see __futimes in
>> sysdeps/unix/sysv/linux/futimes.c in glibc tree).
>>     
>
> Soo.. glibc uses utimensat to implement futimens, but doesn't allow
> applications to use utimensat in the same way. What a mess.
>
> But if we are to go towards using libc calls, your patch is a step
> backwards. In case pathname is null, we can use futimens.
>
>
> commit 0f34ff059fb9ad6e8c8fa5161d9d17265286bb62
> Author: Riku Voipio <riku.voipio@iki.fi>
> Date:   Tue Apr 21 15:01:51 2009 +0300
>
>     linux-user: fix utimensat when used as futimens
>     
>     The glibc function for utimensat glibc returns -EINVAL when the path is null
>     which is a different behaviour with the syscall.
>     
>     path can be null because internally the glibc is using utimensat with
>     path null when implmenting futimens. If path is null, call futimes
>     instead.
>
> diff --git a/configure b/configure
> index 08df436..fc980a4 100755
> --- a/configure
> +++ b/configure
> @@ -1208,6 +1208,25 @@ EOF
>    fi
>  fi
>  
> +# check if utimensat and futimens are supported
> +utimens=no
> +cat > $TMPC << EOF
> +#define _ATFILE_SOURCE
> +#define _GNU_SOURCE
> +#include <stddef.h>
> +#include <fcntl.h>
> +
> +int main(void)
> +{
> +    utimensat(AT_FDCWD, "foo", NULL, 0);
> +    futimens(0, NULL);
> +    return 0;
> +}
> +EOF
> +if $cc $ARCH_CFLAGS -o $TMPE $TMPC 2> /dev/null ; then
> +  utimens=yes
> +fi
> +
>  # Check if tools are available to build documentation.
>  if [ -x "`which texi2html 2>/dev/null`" ] && \
>     [ -x "`which pod2man 2>/dev/null`" ]; then
> @@ -1604,6 +1623,9 @@ fi
>  if test "$atfile" = "yes" ; then
>    echo "#define CONFIG_ATFILE 1" >> $config_h
>  fi
> +if test "$utimens" = "yes" ; then
> +  echo "#define CONFIG_UTIMENSAT 1" >> $config_h
> +fi
>  if test "$inotify" = "yes" ; then
>    echo "#define CONFIG_INOTIFY 1" >> $config_h
>  fi
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 408ccc6..4dad5c1 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -406,13 +406,6 @@ static int sys_unlinkat(int dirfd, const char *pathname, int flags)
>    return (unlinkat(dirfd, pathname, flags));
>  }
>  #endif
> -#ifdef TARGET_NR_utimensat
> -static int sys_utimensat(int dirfd, const char *pathname,
> -    const struct timespec times[2], int flags)
> -{
> -  return (utimensat(dirfd, pathname, times, flags));
> -}
> -#endif
>  #else /* !CONFIG_ATFILE */
>  
>  /*
> @@ -476,12 +469,24 @@ _syscall3(int,sys_symlinkat,const char *,oldpath,
>  #if defined(TARGET_NR_unlinkat) && defined(__NR_unlinkat)
>  _syscall3(int,sys_unlinkat,int,dirfd,const char *,pathname,int,flags)
>  #endif
> +
> +#endif /* CONFIG_ATFILE */
> +
> +#ifdef CONFIG_UTIMENSAT
> +static int sys_utimensat(int dirfd, const char *pathname,
> +    const struct timespec times[2], int flags)
> +{
> +    if (pathname == NULL)
> +        return futimens(dirfd, times);
> +    else
> +        return utimensat(dirfd, pathname, times, flags);
> +}
> +#else
>  #if defined(TARGET_NR_utimensat) && defined(__NR_utimensat)
>  _syscall4(int,sys_utimensat,int,dirfd,const char *,pathname,
>            const struct timespec *,tsp,int,flags)
>  #endif
> -
> -#endif /* CONFIG_ATFILE */
> +#endif /* CONFIG_UTIMENSAT  */
>  
>  #ifdef CONFIG_INOTIFY
>  #include <sys/inotify.h>
>
>
>   

Re: [Qemu-devel] [PATCH] Fix utimensat (aka unbreak cp -a)

[Martin Mohring]

Martin Mohring wrote:
> i found another related case it seems:
>
> when i call in a chroot under arm with qemu user mode:
>
> $ touch /var/log/wtmp
>
> the resulting file has a date/time of 1.1.1970, although the clock is
> correct in the system.
>
> calling
>
> $ touch /var/log/wtmp /var/run/utmp /var/log/btmp
> /bin/touch: setting times of `/var/run/utmp': Invalid argument
> /bin/touch: setting times of `/var/log/btmp': Invalid argument
>
> results in the invalid argutment error. I ll currently check which
> syscalls are involved here.
> the time of 1.1.1970 looks to me like a wrongly passed argument (of ==0).
>   
a trace of this:

27374 open("/var/log/wtmp",0x20941,0666) = 3
27374 dup2(3,0,3,0,0,1109324328) = 0
27374 close(3) = 0
27374 utimensat(0,"(null)",(nil),0) = 0
27374 close(0) = 0
27374 open("/var/run/utmp",0x20941,0666) = 0
27374 utimensat(0,"(null)",(nil),0) = -1 errno=22 (Invalid argument)
27374 close(0) = 0
27374 write(2,0x4007d430,12)/bin/touch:  = 12
27374 write(2,0x4007d408,32)setting times of `/var/run/utmp' = 32
27374 write(2,0x4007cfd0,18): Invalid argument = 18
27374 write(2,0x4007d3d8,1) = 1
27374 open("/var/log/btmp",0x20941,0666) = 0
27374 utimensat(0,"(null)",(nil),0) = -1 errno=22 (Invalid argument)
27374 close(0) = 0
27374 write(2,0x4007d430,12)/bin/touch:  = 12
27374 write(2,0x4007d408,32)setting times of `/var/log/btmp' = 32
27374 write(2,0x4007cfd0,18): Invalid argument = 18
27374 write(2,0x4007d3d8,1) = 1
27374 close(1) = 0
27374 close(2)

Re: [Qemu-devel] [PATCH] Fix utimensat (aka unbreak cp -a)

[Riku Voipio]

On Tue, Apr 21, 2009 at 06:38:17PM +0200, Martin Mohring wrote:
> > results in the invalid argutment error. I ll currently check which
> > syscalls are involved here.
> > the time of 1.1.1970 looks to me like a wrongly passed argument (of ==0).

no, NULL is what is supposed to be passed. try stracing a normal host
/bin/touch.

> 27374 open("/var/log/wtmp",0x20941,0666) = 3
> 27374 dup2(3,0,3,0,0,1109324328) = 0
> 27374 close(3) = 0
> 27374 utimensat(0,"(null)",(nil),0) = 0
> 27374 close(0) = 0
> 27374 open("/var/run/utmp",0x20941,0666) = 0
> 27374 utimensat(0,"(null)",(nil),0) = -1 errno=22 (Invalid argument)

It is unclear what svn revision you are using qemu, and/or if you are using
any of the patches in this thread. The patch I sent earlier, should fix
the invalid arg issue. As for the 1970 issue, try this patch.

commit 7f7e93aabde6b8f0e4a1a05537145efa9ed0156b
Author: Riku Voipio <riku.voipio@iki.fi>
Date:   Tue Apr 21 20:37:01 2009 +0300

    linux-user: fix utimensat with NULL timespec
    
    Don't try to copy timespec from user if it is NULL.

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e19e289..0049840 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6674,17 +6674,22 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 #if defined(TARGET_NR_utimensat) && defined(__NR_utimensat)
     case TARGET_NR_utimensat:
         {
-            struct timespec ts[2];
-            target_to_host_timespec(ts, arg3);
-            target_to_host_timespec(ts+1, arg3+sizeof(struct target_timespec));
+            struct timespec *tsp, ts[2];
+            if (!arg3) {
+                tsp = NULL;
+            } else {
+                target_to_host_timespec(ts, arg3);
+                target_to_host_timespec(ts+1, arg3+sizeof(struct target_timespec));
+                tsp = ts;
+            }
             if (!arg2)
-                ret = get_errno(sys_utimensat(arg1, NULL, ts, arg4));
+                ret = get_errno(sys_utimensat(arg1, NULL, tsp, arg4));
             else {
                 if (!(p = lock_user_string(arg2))) {
                     ret = -TARGET_EFAULT;
                     goto fail;
                 }
-                ret = get_errno(sys_utimensat(arg1, path(p), ts, arg4));
+                ret = get_errno(sys_utimensat(arg1, path(p), tsp, arg4));
                 unlock_user(p, arg2, 0);
             }
         }