From: Ingo Schwarze <schwarze@usta.de>
Subject: Re: Fix rmdir -p /foo/bar
To: Christian Weisgerber <naddy@mips.inka.de>
Cc: Theo Buehler <tb@theobuehler.org>, tech@openbsd.org
Date: Mon, 12 May 2025 21:48:31 +0200

Hello Christian,

Christian Weisgerber wrote on Mon, May 12, 2025 at 09:17:20PM +0200:
> Ingo Schwarze:

>>  * The /* Delete trailing slashes, per POSIX. */ part is actually
>>    a POSIX violation and must be removed completely.

> Yes, both FreeBSD and NetBSD also discovered this.

>>  * The easiest way to fix rm_path() actually is to rewrite it
>>    completely. [...]

> After staring at it far too long, I have now convinced myself that
> this is indeed correct for all corner cases.  OK naddy@,

Thanks for checking!

> but somebody else should also look over it carefully.

That would indeed be appreciated, considering that NetBSD and
FreeBSD fixed this in different ways, but both fixes caused
people to raise reasonable concerns on this list.  Most people,
myself included, would have expected such a a ten-line loop
to be fairly trivial to get right (i have certainly seen parsing
tasks several orders of magnitude more difficult
in /usr/src/usr.bin/mandoc/roff.c), but apparently the first
impression is misleading and it is not totally trivial.

> A small remark:

>>  int
>>  rm_path(char *path)
>>  {
>>  	char *p;
>>  
>> -	while ((p = strrchr(path, '/')) != NULL) {
>> -		/* Delete trailing slashes. */
>> -		while (--p > path && *p == '/')
>> -			continue;
>> -		*++p = '\0';
>> -
>> -		if (rmdir(path) == -1) {
>> -			warn("%s", path);
>> -			return (1);
>> +	p = strchr(path, '\0');
>> +	while (--p > path && *p == '/')
>> +		continue;

> So if we were ever to pass a zero-length string, this would result
> in UB, but since rm_path() will never be called with "", it is fine,
> I guess?

Exactly.  I chose not to put a comment to that effect because
(1) the successful call to rmdir(2) is just four lines above
    the call to rm_path(), and rmdir("") simply doesn't succeed and
(2) the new comment above the function rm_path() also implies
    that something was already removed, which also implies
    that *p cannot be empty.

Then again, if you want, i can put in a comment similar to the
following:

	p = strchr(path, '\0');
	/* We know that p > path because rmdir(path) succeeded. */
	while (--p > path && *p == '/')
		continue;

Yours,
  Ingo

>> +
>> +	while (p > path) {
>> +		if (*p == '/' && p[-1] != '/') {
>> +			*p = '\0';
>> +			if (rmdir(path) == -1) {
>> +				warn("%s", path);
>> +				return 1;
>> +			}
>>  		}
>> +		p--;
>>  	}
>> -
>> -	return (0);
>> +	return 0;
>>  }