Re: [musl] realpath without procfs -- should be ready for inclusion

[Alexey Izbyshev <izbyshev@ispras.ru>]

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.

Alexey