From: "H. Hartzer" <h@hartzer.sh>
Subject: Re: Fix rmdir -p /foo/bar
To: "Theo Buehler" <tb@theobuehler.org>, "Christian Weisgerber" <naddy@mips.inka.de>
Cc: <tech@openbsd.org>
Date: Sun, 11 May 2025 16:12:43 +0000

Hi Theo, Christian, and tech@

Theo Buehler wrote:
>> @@ -92,9 +92,11 @@ rm_path(char *path)
>>  
>>  	while ((p = strrchr(path, '/')) != NULL) {
>>  		/* Delete trailing slashes. */
>> -		while (--p > path && *p == '/')
>> +		while (--p >= path && *p == '/')
>>  			continue;
>
> I don't think this approach is kosher. p can now end up pointing before
> path, which is UB. I think NetBSD's solution is cleaner, if slightly
> less efficient:
>
> https://github.com/NetBSD/src/blob/trunk/bin/rmdir/rmdir.c#L100

I'm not certain which approach is best. I wanted to toss in a regression
test that at least catches the ./-prefixed directory issue when using -p.

-Henrich


diff --git a/regress/bin/Makefile b/regress/bin/Makefile
index 12be52fbfde..6cb2c1ae860 100644
--- a/regress/bin/Makefile
+++ b/regress/bin/Makefile
@@ -1,6 +1,6 @@
 #	$OpenBSD: Makefile,v 1.14 2018/03/31 14:50:56 tobias Exp $
 
-SUBDIR+= cat chmod csh ed expr ksh ln md5 pax ps test
+SUBDIR+= cat chmod csh ed expr ksh ln md5 pax ps rmdir test
 
 install:
 
diff --git a/regress/bin/rmdir/Makefile b/regress/bin/rmdir/Makefile
new file mode 100644
index 00000000000..cbfa4a9f54b
--- /dev/null
+++ b/regress/bin/rmdir/Makefile
@@ -0,0 +1,6 @@
+REGRESS_TARGETS=test_rmdir
+
+test_rmdir:
+	sh test_rmdir.sh
+
+.include <bsd.regress.mk>
diff --git a/regress/bin/rmdir/test_rmdir.sh b/regress/bin/rmdir/test_rmdir.sh
new file mode 100644
index 00000000000..affe7f7cb0f
--- /dev/null
+++ b/regress/bin/rmdir/test_rmdir.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+# If this fails, you may want to run sh -x test_rmdir.sh for a trace to
+# help with debugging.
+
+set -e
+
+fail() {
+	echo $* 1>&2
+	exit 1
+}
+
+mktempdir() {
+	mktemp -d /tmp/tmp.regress.bin.rmdir.XXXXXXXXXX
+}
+
+# Tests the most common rmdir scenario.
+tempdir=$(mktempdir)
+rmdir "$tempdir"
+test -d "$tempdir" && fail "rmdir did not delete $tempdir"
+
+# Tests rmdir -p with a relative path.
+tempdir=$(mktempdir)
+mkdir -p "$tempdir/foo/bar"
+(cd "$tempdir" && rmdir -p foo/bar)
+rmdir "$tempdir"
+test -d "$tempdir" && fail "rmdir did not delete $tempdir"
+
+# Tests rmdir -p with a ./-prefixed relative path.
+tempdir=$(mktempdir)
+mkdir -p "$tempdir/foo/bar"
+(cd "$tempdir" && rmdir -p ./foo/bar)
+rmdir "$tempdir"
+test -d "$tempdir/foo" && fail "rmdir did not delete $tempdir"
+
+true