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

Hi Theo,

Theo Buehler wrote on Mon, May 12, 2025 at 10:03:50PM +0200:

> It reads correct to me.
> ok tb

Thanks for reviewing!

> Only bikesheds below.

>> +	p = strchr(path, '\0');

> If you add a check
> 
> 	if (p == path)
> 		return 1;

I really hate such unreachable code.  If i found this check during an
audit, it would totally trip me up and i would waste a lot of time
thinking that i clearly do not understand what the code is doing,
wondering how this can possibly happen.  Likely i would finally
conclude that whoever wrote the code was just confused and didn't
understand their own code, which would slow my code review down
even more because i would then see a need to be extra careful given
that the author of the code apparently wasn't on top of things.

> naddy's concern would be addressed. I'd prefer that over referring
> to 'rmdir("") doesn't succeed anyway' or other sorts of cleverness.

I think the standard tool for that purpose, i.e. to say "you may wonder
about this edge case, but it cannot happen", is assert(3).  While that
must not be used in a library, it's perfectly fine in a stand-alone
program.

> (If someone copies this chunk of code, they might not notice this
> assumption

Someone copying this code seems unreasonable to me, given how
specialized it is: "strip any trailing slashes and the last filename
component from the end, then iterate the ends of all other
components" really doesn't sound like something that might be
needed outside "rmdir -p".
Then again, an assertion does prevent overlooking this invariant.

> and a check in code is always better than a comment.)

I disagree.  Both serve more or less orthogonal purposes.
Checks guard against functional edge cases.  Comments help
code review and maintenance.
"An Apple Watch is always better than an orange juice."  ;-)

I took your other suggestions and re-tested, so here is an updated
patch.

Still OK?
  Ingo


Index: rmdir.c
===================================================================
RCS file: /cvs/src/bin/rmdir/rmdir.c,v
diff -u -p -r1.14 rmdir.c
--- rmdir.c	28 Jun 2019 13:34:59 -0000	1.14
+++ rmdir.c	12 May 2025 21:02:05 -0000
@@ -30,6 +30,7 @@
  * SUCH DAMAGE.
  */
 
+#include <assert.h>
 #include <err.h>
 #include <errno.h>
 #include <stdio.h>
@@ -66,43 +67,40 @@ main(int argc, char *argv[])
 	if (argc == 0)
 		usage();
 
-	for (errors = 0; *argv; argv++) {
-		char *p;
-
-		/* Delete trailing slashes, per POSIX. */
-		p = *argv + strlen(*argv);
-		while (--p > *argv && *p == '/')
-			continue;
-		*++p = '\0';
-
+	for (errors = 0; *argv != NULL; argv++) {
 		if (rmdir(*argv) == -1) {
 			warn("%s", *argv);
 			errors = 1;
 		} else if (pflag)
 			errors |= rm_path(*argv);
 	}
-
-	return (errors);
+	return errors;
 }
 
+/*
+ * Iteratively remove each pathname component, except the
+ * last one, because that one was already removed in main().
+ */
 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);
+	assert(*path != '\0');
+	p = strchr(path, '\0');
+	while (--p > path && *p == '/')
+		continue;
+
+	for (; p > path; p--) {
+		if (p[0] == '/' && p[-1] != '/') {
+			p[0] = '\0';
+			if (rmdir(path) == -1) {
+				warn("%s", path);
+				return 1;
+			}
 		}
 	}
-
-	return (0);
+	return 0;
 }
 
 static void __dead