From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-0.7 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: from second.openwall.net (second.openwall.net [193.110.157.125]) by inbox.vuxu.org (Postfix) with SMTP id 2119A215AD for ; Wed, 24 Apr 2024 21:30:31 +0200 (CEST) Received: (qmail 23894 invoked by uid 550); 24 Apr 2024 19:30:26 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 23853 invoked from network); 24 Apr 2024 19:30:25 -0000 Date: Wed, 24 Apr 2024 15:30:38 -0400 From: Rich Felker To: Duncan Bellamy Cc: info@bnoordhuis.nl, musl@lists.openwall.com Message-ID: <20240424193038.GP4163@brightrain.aerifal.cx> References: <20200119121247.37310-1-info@bnoordhuis.nl> <20220831190735.52016-1-dunk@denkimushi.com> <20220831190735.52016-2-dunk@denkimushi.com> <20240224165632.GA4163@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240224165632.GA4163@brightrain.aerifal.cx> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [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 > > 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 > > +#include > > +#include > > +#include > > + > > +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