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

[lolzery wowzery <wowzeryest@gmail.com>]

Hi all! I'm new to git over email, new to musl, and I want to really
get into contributing to musl. So, I'd really appreciate any
tips/comments/info for a newbie like me!

Please do not merge these changes yet. There's numerous problems with
this code above and beyond the issues you pointed out, and each of
those issues led me down successive rabbit holes all over the musl
source code and I've ended up opening a sizable can of worms,
including io race condition bugs in fallback conditions, symbol
weakness problems, header/struct misorganization, and bugs in syscall
support detection making musl silently/unexpectedly fail under certain
seccomp configurations.

Also, let me cite the earlier patch from tony.ambardar@gmail.com about
renameat2. My patch will include a full proper renameat2
implementation with validation and fallback and will compile on
systems where SYS_renameat2 is undefined (using exclusively the
fallback.)

I recognize I probably sound like an over-eager-beaver fussing over
fools-gold as I'm new and none of you know me. (That's the assumption
I myself would make if a new guy showed up claiming to have lots of
bug fixes and stuff.) So, please suspend your disbelief for a short
while until I can give my full report and all fixes tomorrow for
everyone to review. Give me a chance to prove my commitment to musl
and watch as I uphold my pledge to maintain my areas of the code in
musl and keep them ship-shape.

Have a great day everyone and really looking forwards to sharing my
full report tomorrow!,
Jack


On Wed, Apr 24, 2024 at 3:30 PM Rich Felker <dalias@libc.org> wrote:
>
> 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