Hi, Alle sabato 14 gennaio 2012, Steven McDonald ha scritto: > 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.) Great investigation, and thanks for bringing this issue again. I see you already sent the locking change to sudo, good job :) > 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. This is true. I did a bit of debug, and it seems sudo needs a fix, but a different one. Basically, when parsing the specified sudoers file (or the default one), the "sudoerslist" list is populated with the file being edit and, recursively, all the files in included directories (see /etc/sudoers.d/README explaining the #includedir directive). In that list, all the elements are structs "sudoersfile"; only the actual file being edit has the "doedit" member set to 1, and all the included files 0. If you see the foreach at line 234 in the main() of plugins/sudoers/visudo.c, all the !doedit files are skipped, and for the other ones edit_sudoers() is called, which initialize the "tpath" member if NULL. This means that later in main(), at the foreach at line 249, we can just skip the !doedit files as done in the foreach above. And actually, the crash happened in that foreach, while trying to unlink() the "tpath" of the "sudoersfile" representing... /etc/sudoers.d/README. Most probably the other archs don't mind, as their unlink() returns (at least on Linux and kFreeBSD) EFAULT (which is ignored anyway). Attached there is the fix I did, and so far gave no issues. (If you want to test it, you need first the patches in #655883 and #655894.) > Is this something that should be fixed in Hurd (or perhaps eglibc), I think we could also fix unlink() on the Hurd bits of glibc to give EINVAL on NULL parameter, or that would be considered (too) broken code anyway? > or should I go ahead and submit the attached patch via the BTS? +1 on the patch already sent (you can usually avoid sending changes of autogenerated files like configure). If you can give my fix a try, we could send this too. -- Pino Toscano
--- a/plugins/sudoers/visudo.c
+++ b/plugins/sudoers/visudo.c
@@ -247,6 +247,8 @@
/* Install the sudoers temp files. */
tq_foreach_fwd(&sudoerslist, sp) {
+ if (!sp->doedit)
+ continue;
if (!sp->modified)
(void) unlink(sp->tpath);
else
Attachment:
signature.asc
Description: This is a digitally signed message part.