[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240424193038.GP4163@brightrain.aerifal.cx>
Date: Wed, 24 Apr 2024 15:30:38 -0400
From: Rich Felker <dalias@...c.org>
To: Duncan Bellamy <dunk@...kimushi.com>
Cc: info@...ordhuis.nl, musl@...ts.openwall.com
Subject: Re: [PATCH 1/2] V3 resubmitting old statx patch with changes
On Sat, Feb 24, 2024 at 11:56:32AM -0500, Rich Felker wrote:
> 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.)
It looks like this failed to fill the stx_attributes[_mask] fields,
leaving them uninitialized, in the fallback for pre-statx kernels.
This needs to be fixed. What should be done with them?
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
