From patchwork Tue Oct 8 17:37:56 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella X-Patchwork-Id: 34881 Received: (qmail 97135 invoked by alias); 8 Oct 2019 17:38:03 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 97127 invoked by uid 89); 8 Oct 2019 17:38:03 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-qt1-f193.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=to:cc:references:from:openpgp:autocrypt:subject:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=5rGPTks/pib4nQJov1ROqAac0Wb45w97axe3M9evc+E=; b=npZ61CffIp+zQwZpgpDDM15wrbekqBbyFjUDnWDrm6BVw9ZVqGWLmtDv49HugqqeEC HQ1AOAf/QJSQHg9csLgQy4/cS5zeROd8BexdcXxhfB12Q6nqneSGaPN0UZ3lsZUkXYT9 2GPCWFXwXLri3A9/RL0ld1xlfmTwFSmXGK0z4+frwQzi8D+hrlqYjoKjvum2wZ0HV9sZ 4Q9XmiCXi5W79RyaOuCBicL2F1yen6W4fFJYyH6uxs34mPy+n8ncHdua9TTEYK4Qu+0U B1Uwd3hJXqU40gocAVOhDBkSa03U7iKZzgkN6gUo637T8yUKqz4wmmxeqjJt+IYV0hGs SV6A== Return-Path: To: Florian Weimer Cc: libc-alpha@sourceware.org References: <20190731183136.21545-1-adhemerval.zanella@linaro.org> <87k1av6kuh.fsf@oldenburg2.str.redhat.com> <49dc9440-d9a5-112f-9ec6-b62f4c0bcdf2@linaro.org> <87zhjmvoow.fsf@oldenburg2.str.redhat.com> <30395e41-d6c9-4fce-ca7a-933d55900fc7@linaro.org> <87imp0flym.fsf@oldenburg2.str.redhat.com> From: Adhemerval Zanella Openpgp: preference=signencrypt Subject: Re: [PATCH v2 1/5] mips: Do not malloc on getdents64 fallback Message-ID: <703d65b6-82bd-f557-7257-0fdd9cedb2f4@linaro.org> Date: Tue, 8 Oct 2019 14:37:56 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <87imp0flym.fsf@oldenburg2.str.redhat.com> On 07/10/2019 15:29, Florian Weimer wrote: > * Adhemerval Zanella: > >> +#define copy_field(dst, src) \ >> + ({ \ >> + typeof (src) _src; \ >> + memcpy (&_src, &(src), sizeof (src)); \ >> + typeof (dst) _dst = _src; \ >> + memcpy (&(dst), &_dst, sizeof (dst)); \ >> + }) >> + >> + copy_field (dp->d_ino, kdp->d_ino); >> + copy_field (dp->d_off, kdp->d_off); >> + copy_field (last_offset, kdp->d_off); >> + copy_field (dp->d_reclen, new_reclen); >> dp->d_type = *((char *) kdp + kdp->d_reclen - 1); > > I believe this still asserts the dynamic type of *dp, which is not what > we want. The truly portable way probably involves using offsetof and > not -> dereferencing. 8-( > > Considering that, I would probably drop copy_field, compile the file > with -fno-strict-aliasing, and add a comment to the (now plain > assignments) that this is okay due to -fno-strict-aliasing. > > But this is really up to you, I do not want to discuss this patch to > death. > I chatted with Rich Felker yesterday and we couldn't be sure that '&((T*)buf)->member' is indeed UB. In any case I changed to a portable way now and I think it should be ok to push. diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c index a8c65cccbf..905239cad9 100644 --- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c +++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c @@ -22,88 +22,113 @@ #include #include #include -#include #include +#include + ssize_t -__getdents64 (int fd, void *buf0, size_t nbytes) +__getdents64 (int fd, void *buf, size_t nbytes) { - char *buf = buf0; - /* The system call takes an unsigned int argument, and some length checks in the kernel use an int type. */ if (nbytes > INT_MAX) nbytes = INT_MAX; #ifdef __NR_getdents64 - ssize_t ret = INLINE_SYSCALL_CALL (getdents64, fd, buf, nbytes); - if (ret != -1) - return ret; + static int getdents64_supported = true; + if (atomic_load_relaxed (&getdents64_supported)) + { + ssize_t ret = INLINE_SYSCALL_CALL (getdents64, fd, buf, nbytes); + if (ret >= 0 || errno != ENOSYS) + return ret; + + atomic_store_relaxed (&getdents64_supported, false); + } #endif /* Unfortunately getdents64 was only wire-up for MIPS n64 on Linux 3.10. - If syscall is not available it need to fallback to non-LFS one. */ + If the syscall is not available it need to fallback to the non-LFS one. + Also to avoid an unbounded allocation through VLA/alloca or malloc (which + would make the syscall non async-signal-safe) it uses a limited buffer. + This is sub-optimal for large NBYTES, however this is a fallback + mechanism to emulate a syscall that kernel should provide. */ struct kernel_dirent - { - unsigned long d_ino; - unsigned long d_off; - unsigned short int d_reclen; - char d_name[256]; - }; + { +#if _MIPS_SIM == _ABI64 + uint64_t d_ino; + uint64_t d_off; +#else + uint32_t d_ino; + uint32_t d_off; +#endif + unsigned short int d_reclen; + char d_name[1]; + }; + + /* The largest possible practical length of the d_name member are 255 + Unicode characters in UTF-8 encoding, so d_name is 766 bytes long, plus + 18 (mips64) / 10 (mips64n32) bytes from header, for total of 784 (mips64) + / 776 (mips64n32) bytes total. Ensure that the minimum size holds at + least one entry. */ + enum { KBUF_SIZE = 1024 }; + char kbuf[KBUF_SIZE]; + size_t kbuf_size = nbytes < KBUF_SIZE ? nbytes : KBUF_SIZE; const size_t size_diff = (offsetof (struct dirent64, d_name) - - offsetof (struct kernel_dirent, d_name)); + - offsetof (struct kernel_dirent, d_name)); - size_t red_nbytes = MIN (nbytes - - ((nbytes / (offsetof (struct dirent64, d_name) - + 14)) * size_diff), - nbytes - size_diff); + struct dirent64 *dp = (struct dirent64 *) buf; - struct scratch_buffer tmpbuf; - scratch_buffer_init (&tmpbuf); - if (!scratch_buffer_set_array_size (&tmpbuf, red_nbytes, sizeof (uint8_t))) - INLINE_SYSCALL_ERROR_RETURN_VALUE (ENOMEM); + size_t nb = 0; + off64_t last_offset = -1; - struct kernel_dirent *skdp, *kdp; - skdp = kdp = tmpbuf.data; + ssize_t r = INLINE_SYSCALL_CALL (getdents, fd, kbuf, kbuf_size); + if (r <= 0) + return r; - ssize_t retval = INLINE_SYSCALL_CALL (getdents, fd, kdp, red_nbytes); - if (retval == -1) - { - scratch_buffer_free (&tmpbuf); - return -1; - } + struct kernel_dirent *skdp, *kdp; + skdp = kdp = (struct kernel_dirent *) kbuf; - off64_t last_offset = -1; - struct dirent64 *dp = (struct dirent64 *) buf; - while ((char *) kdp < (char *) skdp + retval) + while ((char *) kdp < (char *) skdp + r) { - const size_t alignment = _Alignof (struct dirent64); - /* Since kdp->d_reclen is already aligned for the kernel structure - this may compute a value that is bigger than necessary. */ - size_t new_reclen = ((kdp->d_reclen + size_diff + alignment - 1) - & ~(alignment - 1)); - if ((char *) dp + new_reclen > buf + nbytes) - { - /* Our heuristic failed. We read too many entries. Reset - the stream. */ - assert (last_offset != -1); - __lseek64 (fd, last_offset, SEEK_SET); - - if ((char *) dp == buf) + /* This is a conservative approximation, since some of size_diff might + fit into the existing padding for alignment. */ + size_t new_reclen = ALIGN_UP (kdp->d_reclen + size_diff, + _Alignof (struct dirent64)); + if (nb + new_reclen > nbytes) + { + /* Entry is too large for the fixed-size buffer. */ + if (last_offset == -1) { - scratch_buffer_free (&tmpbuf); - return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL); + __set_errno (EINVAL); + return -1; } - break; + /* The new entry will overflow the input buffer, rewind to last + obtained entry and return. */ + __lseek64 (fd, last_offset, SEEK_SET); + return (char *) dp - (char *) buf; } - - last_offset = kdp->d_off; - dp->d_ino = kdp->d_ino; - dp->d_off = kdp->d_off; - dp->d_reclen = new_reclen; + nb += new_reclen; + +#define DP_MEMBER(src, type, member) \ + (__typeof__((type){0}.member) *) \ + memcpy (&((__typeof__((type){0}.member)){0}), \ + ((char *)(src) + offsetof (type, member)), \ + sizeof ((type){0}.member)) + + memcpy (((char *)(dp) + offsetof (struct dirent64, d_ino)), + DP_MEMBER (kdp, struct kernel_dirent, d_ino), + sizeof ((struct dirent64){0}.d_ino)); + memcpy (((char *)(dp) + offsetof (struct dirent64, d_off)), + DP_MEMBER (kdp, struct kernel_dirent, d_ino), + sizeof ((struct dirent64){0}.d_ino)); + memcpy (&last_offset, + DP_MEMBER (kdp, struct kernel_dirent, d_off), + sizeof (last_offset)); + memcpy (DP_MEMBER (dp, struct dirent64, d_reclen), &new_reclen, + sizeof ((struct dirent64){0}.d_reclen)); dp->d_type = *((char *) kdp + kdp->d_reclen - 1); memcpy (dp->d_name, kdp->d_name, kdp->d_reclen - offsetof (struct kernel_dirent, d_name)); @@ -112,8 +137,7 @@ __getdents64 (int fd, void *buf0, size_t nbytes) kdp = (struct kernel_dirent *) (((char *) kdp) + kdp->d_reclen); } - scratch_buffer_free (&tmpbuf); - return (char *) dp - buf; + return (char *) dp - (char *) buf; } libc_hidden_def (__getdents64) weak_alias (__getdents64, getdents64)