return-value/errno for utimensat(<filefd>, NULL, NULL, 0) mismatch across musl and glibc: bug or a feature?

return-value/errno for utimensat(<filefd>, NULL, NULL, 0) mismatch across musl and glibc: bug or a feature?

[Sergei Trofimovich]

Hi musl@ folk!

The original issue popped in https://bugs.gentoo.org/549108#c22.
There glibc's utimensat() wrapper handles one corner case differently
from musl's wrapper.

Here is the minimal reproducer:

$ cat a.c
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stddef.h>

int main() {
    int fd = open("f", O_WRONLY|O_CREAT, 0666);
    return utimensat(fd, NULL, NULL, 0);
}

On glibc (x86_64 linux-5.2-rc5):

$ gcc a.c -o a && strace -etrace=open,openat,utimensat,exit_group ./a
openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "f", O_WRONLY|O_CREAT, 0666) = 3
exit_group(-1)                          = ?
+++ exited with 255 +++

On musl (x86_64 linux-5.2-rc5):
$ gcc a.c -o a && strace -etrace=open,openat,utimensat,exit_group ./a
open("f", O_WRONLY|O_CREAT, 0666)       = 3
utimensat(3, NULL, NULL, 0)             = 0
exit_group(0)                           = ?

The difference stems from this extra check in glibc:
    https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/utimensat.c;h=04b549f360b88a7e7c1e5e617158caf73299736b;hb=HEAD#l32

int utimensat (int fd, const char *file, const struct timespec tsp[2], int flags)
{
   if (file == NULL)
     return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
   /* Avoid implicit array coercion in syscall macros.  */
   return INLINE_SYSCALL (utimensat, 4, fd, file, &tsp[0], flags);
}

while musl just calls the syscall directly:

https://git.musl-libc.org/cgit/musl/tree/src/stat/utimensat.c

int utimensat(int fd, const char *path, const struct timespec times[2], int flags)
{
	int r = __syscall(SYS_utimensat, fd, path, times, flags);
        // ...
	return __syscall_ret(r);
}

Is this divergence expected? Or maybe it's accidental? Does it make
sense to handle non-directory fds in utimensat() according to POSIX?

I wonder if we should drop the unstable test or some of libc implementations
actually deviates from the spec.

Thank you!

-- 

  Sergei

Re: return-value/errno for utimensat(<filefd>, NULL, NULL, 0) mismatch across musl and glibc: bug or a feature?

[Rich Felker]

On Tue, Jun 25, 2019 at 09:35:25PM +0100, Sergei Trofimovich wrote:
> Hi musl@ folk!
> 
> The original issue popped in https://bugs.gentoo.org/549108#c22.
> There glibc's utimensat() wrapper handles one corner case differently
> from musl's wrapper.
> 
> Here is the minimal reproducer:
> 
> $ cat a.c
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <stddef.h>
> 
> int main() {
>     int fd = open("f", O_WRONLY|O_CREAT, 0666);
>     return utimensat(fd, NULL, NULL, 0);
> }
> 
> On glibc (x86_64 linux-5.2-rc5):
> 
> $ gcc a.c -o a && strace -etrace=open,openat,utimensat,exit_group ./a
> openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
> openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
> openat(AT_FDCWD, "f", O_WRONLY|O_CREAT, 0666) = 3
> exit_group(-1)                          = ?
> +++ exited with 255 +++
> 
> On musl (x86_64 linux-5.2-rc5):
> $ gcc a.c -o a && strace -etrace=open,openat,utimensat,exit_group ./a
> open("f", O_WRONLY|O_CREAT, 0666)       = 3
> utimensat(3, NULL, NULL, 0)             = 0
> exit_group(0)                           = ?
> 
> The difference stems from this extra check in glibc:
>     https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/utimensat.c;h=04b549f360b88a7e7c1e5e617158caf73299736b;hb=HEAD#l32
> 
> int utimensat (int fd, const char *file, const struct timespec tsp[2], int flags)
> {
>    if (file == NULL)
>      return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
>    /* Avoid implicit array coercion in syscall macros.  */
>    return INLINE_SYSCALL (utimensat, 4, fd, file, &tsp[0], flags);
> }
> 
> while musl just calls the syscall directly:
> 
> https://git.musl-libc.org/cgit/musl/tree/src/stat/utimensat.c
> 
> int utimensat(int fd, const char *path, const struct timespec times[2], int flags)
> {
> 	int r = __syscall(SYS_utimensat, fd, path, times, flags);
>         // ...
> 	return __syscall_ret(r);
> }
> 
> Is this divergence expected? Or maybe it's accidental? Does it make
> sense to handle non-directory fds in utimensat() according to POSIX?
> 
> I wonder if we should drop the unstable test or some of libc implementations
> actually deviates from the spec.

I think the test is wrong. Passing a null pointer for a pathname
argument where the interface requires a pointer to a string is
undefined behavior unless the specification assigns special meaning to
the null argument, and here it doesn't:

http://pubs.opengroup.org/onlinepubs/9699919799/functions/utimensat.html

The EINVAL error is specified for different purposes, so glibc is
"wrong" on that too. Of course the "wrongness" isn't non-conforming,
because anything can happen on UB, but if they want to catch it the
error should probably be EFAULT for consistency.

If there's a concern that the musl behavior is allowing silent
incorrect behavior (operating on the fd argument rather than treating
it as a directory for the relative "at" operation), perhaps we should
either make it crash explicitly or simply do something like
path?path:(void*)-1 to produce EFAULT and still show the wrong
operation in strace. However I kinda don't like this since it makes
implementing futimens in terms of utimensat more roundabout -- we'd
have to introduce an extra internal symbol to get around the check.

Rich

Re: return-value/errno for utimensat(<filefd>, NULL, NULL, 0) mismatch across musl and glibc: bug or a feature?

[Sergei Trofimovich]

On Tue, 25 Jun 2019 17:25:19 -0400
Rich Felker <dalias@libc.org> wrote:

> On Tue, Jun 25, 2019 at 09:35:25PM +0100, Sergei Trofimovich wrote:
> > Hi musl@ folk!
> > 
> > The original issue popped in https://bugs.gentoo.org/549108#c22.
> > There glibc's utimensat() wrapper handles one corner case differently
> > from musl's wrapper.
> > 
> > Here is the minimal reproducer:
> > 
> > $ cat a.c
> > #include <sys/types.h>
> > #include <sys/stat.h>
> > #include <fcntl.h>
> > #include <stddef.h>
> > 
> > int main() {
> >     int fd = open("f", O_WRONLY|O_CREAT, 0666);
> >     return utimensat(fd, NULL, NULL, 0);
> > }
> > 
> > On glibc (x86_64 linux-5.2-rc5):
> > 
> > $ gcc a.c -o a && strace -etrace=open,openat,utimensat,exit_group ./a
> > openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
> > openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
> > openat(AT_FDCWD, "f", O_WRONLY|O_CREAT, 0666) = 3
> > exit_group(-1)                          = ?
> > +++ exited with 255 +++
> > 
> > On musl (x86_64 linux-5.2-rc5):
> > $ gcc a.c -o a && strace -etrace=open,openat,utimensat,exit_group ./a
> > open("f", O_WRONLY|O_CREAT, 0666)       = 3
> > utimensat(3, NULL, NULL, 0)             = 0
> > exit_group(0)                           = ?
> > 
> > The difference stems from this extra check in glibc:
> >     https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/utimensat.c;h=04b549f360b88a7e7c1e5e617158caf73299736b;hb=HEAD#l32
> > 
> > int utimensat (int fd, const char *file, const struct timespec tsp[2], int flags)
> > {
> >    if (file == NULL)
> >      return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
> >    /* Avoid implicit array coercion in syscall macros.  */
> >    return INLINE_SYSCALL (utimensat, 4, fd, file, &tsp[0], flags);
> > }
> > 
> > while musl just calls the syscall directly:
> > 
> > https://git.musl-libc.org/cgit/musl/tree/src/stat/utimensat.c
> > 
> > int utimensat(int fd, const char *path, const struct timespec times[2], int flags)
> > {
> > 	int r = __syscall(SYS_utimensat, fd, path, times, flags);
> >         // ...
> > 	return __syscall_ret(r);
> > }
> > 
> > Is this divergence expected? Or maybe it's accidental? Does it make
> > sense to handle non-directory fds in utimensat() according to POSIX?
> > 
> > I wonder if we should drop the unstable test or some of libc implementations
> > actually deviates from the spec.  
> 
> I think the test is wrong. Passing a null pointer for a pathname
> argument where the interface requires a pointer to a string is
> undefined behavior unless the specification assigns special meaning to
> the null argument, and here it doesn't:
> 
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/utimensat.html
> 
> The EINVAL error is specified for different purposes, so glibc is
> "wrong" on that too. Of course the "wrongness" isn't non-conforming,
> because anything can happen on UB, but if they want to catch it the
> error should probably be EFAULT for consistency.
> 
> If there's a concern that the musl behavior is allowing silent
> incorrect behavior (operating on the fd argument rather than treating
> it as a directory for the relative "at" operation), perhaps we should
> either make it crash explicitly or simply do something like
> path?path:(void*)-1 to produce EFAULT and still show the wrong
> operation in strace. However I kinda don't like this since it makes
> implementing futimens in terms of utimensat more roundabout -- we'd
> have to introduce an extra internal symbol to get around the check.

Thank you! We'll make test glibc-specific or drop it entirely.

-- 

  Sergei