Problems with Hurd's unlink in visudo

[Steven McDonald <steven.mcdonald@libremail.me>]

Hi,

I've been looking at the problems with visudo as tracked on alioth[1].
I've applied Justus's fix to use flock instead of lockf, and created a
patch that solves the segfault issue (see attached). visudo now seems
to work correctly on Hurd.

However, I'm uncertain whether the segfault should actually be
considered a bug in visudo, or whether it should rather be fixed in
Hurd's implementation of unlink (see backtrace[2]). As such, I'm asking
on the list for advice before I submit this as a bug report. (I'll be
submitting Justus's fix anyway, as it's really a separate problem.)

As you can see in the attached patch, visudo already uses a void cast
on unlink, suggesting that it doesn't care if the unlink operation
fails. Presumably on Linux and kFreeBSD (I haven't verified this, as
I'm not sure where it's defined), unlink exits gracefully with failure
if it is passed a null pointer, whereas on Hurd it doesn't check this
and segfaults as a result.

Is this something that should be fixed in Hurd (or perhaps eglibc), or
should I go ahead and submit the attached patch via the BTS?

[1] https://alioth.debian.org/tracker/index.php?func=detail&aid=303084&group_id=30628&atid=411594
[2] http://ftp.steven-mcdonald.id.au/visudo_backtrace.txt

Thanks,
Steven.

Ensure that sp->tpath is non-null before attempting to unlink
Index: sudo-1.8.3p1/plugins/sudoers/visudo.c
===================================================================
--- sudo-1.8.3p1.orig/plugins/sudoers/visudo.c	2011-10-22 00:01:26.000000000 +1100
+++ sudo-1.8.3p1/plugins/sudoers/visudo.c	2012-01-14 19:23:56.000000000 +1100
@@ -247,10 +247,11 @@
 
     /* Install the sudoers temp files. */
     tq_foreach_fwd(&sudoerslist, sp) {
-	if (!sp->modified)
-	    (void) unlink(sp->tpath);
-	else
+	if (!sp->modified) {
+	    if (sp->tpath) (void) unlink(sp->tpath);
+	} else {
 	    (void) install_sudoers(sp, oldperms);
+	}
     }
 
     exit(0);
@@ -599,7 +600,8 @@
 	    sp->tpath = NULL;
 	} else {
 	    warning(_("error renaming %s, %s unchanged"), sp->tpath, sp->path);
-	    (void) unlink(sp->tpath);
+	    if (sp->tpath)
+                (void) unlink(sp->tpath);
 	    return FALSE;
 	}
     }

Attachment: signature.asc
Description: PGP signature