Re: [musl] [PATCH 1/2] V3 resubmitting old statx patch with changes

[Rich Felker <dalias@libc.org>]

On Wed, Aug 31, 2022 at 08:07:34PM +0100, Duncan Bellamy wrote:
> ---
>  include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/stat/statx.c   | 40 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)
>  create mode 100644 src/stat/statx.c

I'm picking back up review of this because it's been requested again.
Last time I dropped off because there continued to be unresolved
issues where it looked like this hadn't been tested.

I'll make the following changes and merge if there's no objection:

> diff --git a/include/sys/stat.h b/include/sys/stat.h
> index 10d446c4..21668a51 100644
> --- a/include/sys/stat.h
> +++ b/include/sys/stat.h
> @@ -70,6 +70,55 @@ extern "C" {
>  #define UTIME_NOW  0x3fffffff
>  #define UTIME_OMIT 0x3ffffffe
>  
> +#if defined(_GNU_SOURCE)
> +#include <stdint.h>

This should be moved above include of bits/alltypes.h as:

#ifdef _GNU_SOURCE
#define #define __NEED_int64_t
#define #define __NEED_uint62_t
#define #define __NEED_uint32_t
#endif

> +#define STATX_TYPE 1U
> +#define STATX_MODE 2U
> +#define STATX_NLINK 4U
> +#define STATX_UID 8U
> +#define STATX_GID 0x10U
> +#define STATX_ATIME 0x20U
> +#define STATX_MTIME 0x40U
> +#define STATX_CTIME 0x80U
> +#define STATX_INO 0x100U
> +#define STATX_SIZE 0x200U
> +#define STATX_BLOCKS 0x400U
> +#define STATX_BASIC_STATS 0x7ffU
> +#define STATX_BTIME 0x800U
> +#define STATX_ALL 0xfffU
> +
> +struct statx_timestamp {
> +	int64_t tv_sec;
> +	uint32_t tv_nsec, __pad;
> +};
> +
> +struct statx {
> +	uint32_t stx_mask;
> +	uint32_t stx_blksize;
> +	uint64_t stx_attributes;
> +	uint32_t stx_nlink;
> +	uint32_t stx_uid;
> +	uint32_t stx_gid;
> +	uint16_t stx_mode;
> +	uint16_t __pad0[1];
> +	uint64_t stx_ino;
> +	uint64_t stx_size;
> +	uint64_t stx_blocks;
> +	uint64_t stx_attributes_mask;
> +	struct statx_timestamp stx_atime;
> +	struct statx_timestamp stx_btime;
> +	struct statx_timestamp stx_ctime;
> +	struct statx_timestamp stx_mtime;
> +	uint32_t stx_rdev_major;
> +	uint32_t stx_rdev_minor;
> +	uint32_t stx_dev_major;
> +	uint32_t stx_dev_minor;
> +	uint64_t __pad1[14];
> +};
> +
> +int statx(int, const char *__restrict, int, unsigned, struct statx *__restrict);
> +#endif

This should probably be reordered to go with the other extensions, but
it's not really a big deal, that's just usually how the headers are
organized.

>  int stat(const char *__restrict, struct stat *__restrict);
>  int fstat(int, struct stat *);
>  int lstat(const char *__restrict, struct stat *__restrict);
> diff --git a/src/stat/statx.c b/src/stat/statx.c
> new file mode 100644
> index 00000000..4eae0d56
> --- /dev/null
> +++ b/src/stat/statx.c
> @@ -0,0 +1,40 @@
> +#define _GNU_SOURCE
> +#include <sys/stat.h>
> +#include <syscall.h>
> +#include <sys/sysmacros.h>
> +#include <errno.h>
> +
> +int statx(int dirfd, const char *restrict path, int flags, unsigned mask, struct statx *restrict stx)
> +{
> +	int ret = __syscall(SYS_statx, dirfd, path, flags, mask, stx);
> +
> +	#if defined(SYS_fstatat)
> +	if (ret != -ENOSYS) return __syscall_ret(ret);
> +	struct stat st;
> +	ret = fstatat(dirfd, path, &st, flags);
> +	if (ret == -ENOSYS) return -1;

fstatat returns -1 on error and sets errno. The condition also makes
no sense. The intent here is to return without writing to *stx when
fstatat failed. This is not specific to ENOSYS (which can't happen, at
least not without bogus seccomp meddling) but could happen for lots of
reasons. I think it should just be:

	if (ret) return ret;

> +	stx->stx_dev_major = major(st.st_dev);
> +	stx->stx_dev_minor = minor(st.st_dev);
> +	stx->stx_ino = st.st_ino;
> +	stx->stx_mode = st.st_mode;
> +	stx->stx_nlink = st.st_nlink;
> +	stx->stx_uid = st.st_uid;
> +	stx->stx_gid = st.st_gid;
> +	stx->stx_size = st.st_size;
> +	stx->stx_blksize = st.st_blksize;
> +	stx->stx_blocks = st.st_blocks;
> +	stx->stx_atime.tv_sec = st.st_atim.tv_sec;
> +	stx->stx_atime.tv_nsec = st.st_atim.tv_nsec;
> +	stx->stx_mtime.tv_sec = st.st_mtim.tv_sec;
> +	stx->stx_mtime.tv_nsec = st.st_mtim.tv_nsec;
> +	stx->stx_ctime.tv_sec = st.st_ctim.tv_sec;
> +	stx->stx_ctime.tv_nsec = st.st_ctim.tv_nsec;
> +	stx->stx_btime = (struct statx_timestamp){.tv_sec=0, .tv_nsec=0};
> +	stx->stx_mask = STATX_BASIC_STATS;

This is probably all fine. Rather than explicitly filling btime with
zeros, it might make sense to memset the whole struct zero first so
there's no stack data leak in unsupported extension fields.

> +	ret = EINVAL;
> +	return ret;

This is definitely wrong. statx returns 0 on success or -1 on failure,
never a positive value like EINVAL. I'm not even sure what the intent
was here..?

> +	#else
> +	return __syscall_ret(ret);
> +	#endif
> +}

This looks ok, but it would probably make sense to reverse the #ifdef
to #ifndef and flip the order in the file, so the return code using
negated error number like this is up with the __syscall it goes with
rather than separated. As written it's hard to see that the right
thing is being done in the path that uses errno vs the one using
negated error codes. (Admittedly, this being mishandled and needing to
review it in more detail is probably what made me think of this.)

Rich