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=-3.1 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,T_SCC_BODY_TEXT_LINE 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 DA5902587F for ; Sat, 24 Feb 2024 17:56:29 +0100 (CET) Received: (qmail 10148 invoked by uid 550); 24 Feb 2024 16:52:59 -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 10107 invoked from network); 24 Feb 2024 16:52:59 -0000 Date: Sat, 24 Feb 2024 11:56:32 -0500 From: Rich Felker To: Duncan Bellamy Cc: info@bnoordhuis.nl, musl@lists.openwall.com Message-ID: <20240224165632.GA4163@brightrain.aerifal.cx> References: <20200119121247.37310-1-info@bnoordhuis.nl> <20220831190735.52016-1-dunk@denkimushi.com> <20220831190735.52016-2-dunk@denkimushi.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220831190735.52016-2-dunk@denkimushi.com> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH 1/2] V3 resubmitting old statx patch with changes 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.) Rich