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=-2.8 required=5.0 tests=DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, 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 6B3772200B for ; Thu, 25 Apr 2024 01:55:58 +0200 (CEST) Received: (qmail 7955 invoked by uid 550); 24 Apr 2024 23:55:52 -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 7908 invoked from network); 24 Apr 2024 23:55:52 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1714002944; x=1714607744; darn=lists.openwall.com; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=szFZs/D3wB++sUMm3OKtkbounttXlVZ6ql3fIjKi8o8=; b=Uh0RAsv5ccANncj9EuS5IqZGYblcupQVNNxmlyLwXavxM5ydLE/d7Ud+A2vo8vqht1 MfnQaipJ04wEg+8EJ95NdTGGj366Rh6NK29izNw2zYREVhi8bh8fMqEVlg69Ct6Kg8fd 4inHsmrSHW7cz6o1QY2PDfXoHkRJ4TcOuHakJt5NFGLgWX+Vs3P3Qp1+1u1BTcpIdFc9 3a+qJFFoBmIXXVJk3r5p2chce2Z+VqoAScrQXTo7iPW2rrOP3d4A83rIviXjew/VWGV7 MbU+LqhHTpUbkIb6Aiwl0QrfV/lGuF/cikbnHyt9uURs/Bf7kN6/c2xeYi4xXrSUfyfH WWMw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714002944; x=1714607744; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=szFZs/D3wB++sUMm3OKtkbounttXlVZ6ql3fIjKi8o8=; b=j7d3y5bDGvknq8pIkH92gSR1zzWwiADfqhmEVR5JbsKbnyW0vh1abTH/59njV3XhRZ zwGFOe9A6xpM5XDFt7hLHOON1HPsyqJqrMDDa0l3WhRcro8G9d+H/JtWEl8oi4e1EnsF DmaBr6V9dkEDQAPvaOpwV192kLertkJqE8bnZ89NgCuWNwTLiwaLtxS8/6xZQwFialyc 6c2EOXVJyl7GKl4hbwDR8k9YYollD9iKCF9UAW1F9kjazLHRUcMu7R73t/+QsijgCZId Ai+Yi3LXnSFNhuOnuAzmqwdWmJtfN/htkOjth2EiKuofN63piuKg3ztb0qt+cn92rpBt gTMQ== X-Gm-Message-State: AOJu0Ywsa0abQihTSPTk98ol/04/ZreuSsvVY4Ubuk4zkeJbU46oV7mp H7o9kusmwSMZ5j7EHaNm7nCPYJYKWfzZmTwOZV2k/XyehFGOi+5b17RTnvpLMIjlzfyKi1sTVeR d30Jq/d3FfaLFIeufftrro8AQnCc6I8UF X-Google-Smtp-Source: AGHT+IFi+eTJ1htnfxfXhXSaGIvNkuneETqDxPPO7PdC3Z7AGgmU7tRvpgQyQD9VBynFDNzeqvhZiF8f8GQkPMMzlFQ= X-Received: by 2002:a05:6871:5810:b0:22e:d5f6:3174 with SMTP id oj16-20020a056871581000b0022ed5f63174mr4524664oac.34.1714002943558; Wed, 24 Apr 2024 16:55:43 -0700 (PDT) MIME-Version: 1.0 References: <20200119121247.37310-1-info@bnoordhuis.nl> <20220831190735.52016-1-dunk@denkimushi.com> <20220831190735.52016-2-dunk@denkimushi.com> <20240224165632.GA4163@brightrain.aerifal.cx> <20240424193038.GP4163@brightrain.aerifal.cx> In-Reply-To: <20240424193038.GP4163@brightrain.aerifal.cx> From: lolzery wowzery Date: Wed, 24 Apr 2024 19:55:32 -0400 Message-ID: To: musl@lists.openwall.com Cc: Duncan Bellamy , info@bnoordhuis.nl, tony.ambardar@gmail.co Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [musl] [PATCH 1/2] V3 resubmitting old statx patch with changes 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=E2=80=AFPM Rich Felker 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 > > > > 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 =3D __syscall(SYS_statx, dirfd, path, flags, mask, stx); > > > + > > > + #if defined(SYS_fstatat) > > > + if (ret !=3D -ENOSYS) return __syscall_ret(ret); > > > + struct stat st; > > > + ret =3D fstatat(dirfd, path, &st, flags); > > > + if (ret =3D=3D -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 =3D major(st.st_dev); > > > + stx->stx_dev_minor =3D minor(st.st_dev); > > > + stx->stx_ino =3D st.st_ino; > > > + stx->stx_mode =3D st.st_mode; > > > + stx->stx_nlink =3D st.st_nlink; > > > + stx->stx_uid =3D st.st_uid; > > > + stx->stx_gid =3D st.st_gid; > > > + stx->stx_size =3D st.st_size; > > > + stx->stx_blksize =3D st.st_blksize; > > > + stx->stx_blocks =3D st.st_blocks; > > > + stx->stx_atime.tv_sec =3D st.st_atim.tv_sec; > > > + stx->stx_atime.tv_nsec =3D st.st_atim.tv_nsec; > > > + stx->stx_mtime.tv_sec =3D st.st_mtim.tv_sec; > > > + stx->stx_mtime.tv_nsec =3D st.st_mtim.tv_nsec; > > > + stx->stx_ctime.tv_sec =3D st.st_ctim.tv_sec; > > > + stx->stx_ctime.tv_nsec =3D st.st_ctim.tv_nsec; > > > + stx->stx_btime =3D (struct statx_timestamp){.tv_sec=3D0, .tv_nsec= =3D0}; > > > + stx->stx_mask =3D 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 =3D 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