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