From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 19820 invoked from network); 24 Nov 2020 06:31:07 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 24 Nov 2020 06:31:07 -0000 Received: (qmail 22401 invoked by uid 550); 24 Nov 2020 06:31:02 -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 22380 invoked from network); 24 Nov 2020 06:31:01 -0000 Date: Tue, 24 Nov 2020 01:30:49 -0500 From: Rich Felker To: Alexey Izbyshev Cc: musl@lists.openwall.com Message-ID: <20201124063048.GB534@brightrain.aerifal.cx> References: <20201122225619.GR534@brightrain.aerifal.cx> <97dd3cf7c69673e5962e9ccd46ea5131@ispras.ru> <20201123031932.GS534@brightrain.aerifal.cx> <20201123185633.GY534@brightrain.aerifal.cx> <20201123205259.GZ534@brightrain.aerifal.cx> <48faf5ab9a1f3c869c85897217db0d75@ispras.ru> <20201124042646.GA534@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] realpath without procfs -- should be ready for inclusion On Tue, Nov 24, 2020 at 08:13:56AM +0300, Alexey Izbyshev wrote: > On 2020-11-24 07:26, Rich Felker wrote: > >On Tue, Nov 24, 2020 at 06:39:59AM +0300, Alexey Izbyshev wrote: > >>On 2020-11-23 23:53, Rich Felker wrote: > >>>On Mon, Nov 23, 2020 at 01:56:33PM -0500, Rich Felker wrote: > >>>>On Sun, Nov 22, 2020 at 10:19:33PM -0500, Rich Felker wrote: > >>>>--- realpath8.c 2020-11-22 17:52:17.586481571 -0500 > >>>>+++ realpath9.c 2020-11-23 13:55:06.808458893 -0500 > >>>>@@ -19,7 +19,7 @@ > >>>> char *output = resolved ? resolved : buf; > >>>> size_t p, q, l, cnt=0; > >>>> > >>>>- l = strnlen(filename, sizeof stack + 1); > >>>>+ l = strnlen(filename, sizeof stack); > >>>> if (!l) { > >>>> errno = ENOENT; > >>>> return 0; > >>>>@@ -80,11 +80,16 @@ > >>>> return 0; > >>>> } > >>>> if (k==p) goto toolong; > >>>>+ if (!k) { > >>>>+ errno = ENOENT; > >>>>+ return 0; > >>>>+ } > >>>> if (++cnt == SYMLOOP_MAX) { > >>>> errno = ELOOP; > >>>> return 0; > >>>> } > >>>> p -= k; > >>>>+ if (stack[k-1]=='/') p++; > >>>> memmove(stack+p, stack, k); > >>> > >>>This is wrong and needs further consideration. > >>> > >>Yes, now memmove() overwrites NUL if p was at the end and stack[k-1] > >>== '/'. Is it true per POSIX that "rr/home" must resolve to "//home" > >>if "rr" -> "//"? > > > >I don't think // is even required be distinct from /, just permitted, > >but I think allowing it in userspace and handling it consistently is > >the right behavior in case you ever run on a kernel that does make use > >of the distinction. > > > >>If so, maybe something like the following instead: > >> > >>+ while (stack[p] == '/') p++; > >>+ if (stack[p] && stack[k-1] != '/') p--; > >> p -= k; > >>- if (stack[k-1]=='/') p++; > > > >Rather just: > > > > /* If link contents end in /, strip any slashes already on > > * stack to avoid /->// or //->/// or spurious toolong. */ > > if (stack[k-1]=='/') while (stack[p]=='/') p++; > > > >should work (before the p-=k;) > > > Yes, that looks good. > > >>I've also noticed other issues to be fixed, per POSIX: > >> > >>* ENOENT should be returned if filename is NULL > > > >Rather it looks like it's: > > > > [EINVAL] The file_name argument is a null pointer. > > > >ENOENT is only for empty string or ENOENT somewhere in the path > >traversal process. > > > Uh, yes, that was bad copy-paste or something. > > >>* ENOTDIR should be returned if the last component is not a > >>directory and the path has one or more trailing slashes > > > >Yes, that's precisely what I've been working on the past couple hours. > >I think you missed but .. will also erase a path component that's not > >a dir (e.g. /dev/null/.. -> /dev) and these are both instances of a > >common problem. I thought use of readlink covered all the ENOTDIR > >cases but it doesn't when the next component isn't covered by readlink > >or isn't present at all. > > > Yes, initially I forgot about this whole ENOTDIR issue completely, > and after noticing the problem with the last component, didn't look > further. I think before this goes upstream we should have a good set of testcases that can be contributed to libc-test. Do you have ideas for coverage? Some that come to mind: - Absolute argument starting with /, //, and /// - Absolute symlink target starting with /, //, and /// - Final / after symlink-to-dir, dir, symlink-to-nondir, nondir - Final / in link contents after [the above] - Initial .. in argument, cwd root or non-root - Initial .. in symlink target, symlink in root or non-root - Initial ... - .. following symlink-to-dir, dir, symlink-to-nondir, nondir - More .. than path depth - Null argument - Empty string argument - Empty string link contents (testable only with seccomp hack) - Argument valid abs path exact length PATH_MAX-1 - Argument valid rel path exact length PATH_MAX-1 to short abs path Some of these require namespace gymnastics to set up without running the tests as root. Rich