Hello Christian,

Christian Weisgerber wrote on Sun, May 11, 2025 at 09:20:10PM +0200:

> So there are at least three bugs:
> 
> (1)
> # rmdir -p /.ssh/agent
> rmdir: : No such file or directory
> 
> (2)
> $ mkdir dir
> $ ln -s dir lnk
> $ rmdir lnk     # should fail
> rmdir: lnk: Not a directory
> $ rmdir lnk/    # should remove dir, that's what rmdir(2) does
> rmdir: lnk: Not a directory

Here is a comprehensive patch for these two, see below.
I'm including the KNF parts (*argv != NULL, return())
because that actually makes the patch more readable (more context).

Rationale:

 * The /* Delete trailing slashes, per POSIX. */ part is actually
   a POSIX violation and must be removed completely.
   That's your bug (2).
   Apart from that, main() is fine.

   Seeing that the behaviour you demand in (2) is indeed required
   by POSIX is a bit tricky, but i agree:
    - POSIX rmdir(1) says:
      "For each dir operand, the rmdir utility shall perform actions
       equivalent to the rmdir() function called with the dir operand
       as its only argument."
    - POSIX rmdir(2) says:
      "If path names a symbolic link, then rmdir() shall fail
       and set errno to [ENOTDIR].
      That confirms your demand "lnk: Not a directory".
    - POSIX rmdir(2) says:
      "The rmdir() function shall remove a directory whose name
       is given by path."
      So this demands a mapping path -> directory file.
      But that is just what "Base Definitions 3.254 Pathname"
      calls "Pathname Resolution".
    - Base Definitions 4.16 Pathname Resolution
      among other things defines when pathname resolution is complete.
      In particular, pathname resoution of "lnk" is complete,
      implying that rmdir(2) fails with ENOTDIR,
      but pathname resolution is never complete when there is a
      slash at the end, so to resolve "lnk/", it is tranformed
      into "dir", so "dir" is the directory "whose name is given by"
      lnk/, and consequently "dir" gets removed.

 * The easiest way to fix rm_path() actually is to rewrite it
   completely.  The simplest algorithm is to iterate the path
   backwards, and at every slash following a non-slash,
   issue rmdir(2).  Skipping trailing slashes in rm_path()
   is needed because main() no longer does that and we don't
   want to rmdir(2) the final component a second time.

   Having two nested loops on p is not necessary and just confusing.


> (3)
> # rmdir /	# should return EBUSY, kernel problem
> rmdir: /: Is a directory

I'll leave that one to kernel hackers.

Yours,
  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 11:55:35 -0000
@@ -66,43 +66,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);
+	p = strchr(path, '\0');
+	while (--p > path && *p == '/')
+		continue;
+
+	while (p > path) {
+		if (*p == '/' && p[-1] != '/') {
+			*p = '\0';
+			if (rmdir(path) == -1) {
+				warn("%s", path);
+				return 1;
+			}
 		}
+		p--;
 	}
-
-	return (0);
+	return 0;
 }
 
 static void __dead