Re: smtpd: mail.* tempfail vs permfail
On Wed, 25 Sep 2019 15:23:06 +0200, Martijn van Duren wrote: > I talked to gilles@ about this and he's somewhat inclined to agree with > me that this is a TEMPFAIL situation. Even if the directory where the > mail is supposed to be delivered is permanently not accessible I'd > argue that we should keep this a TEMPFAIL, because an admin is inclined > to spot this misconfiguration mistake earlier this way (e.g. via mailq > monitoring) and mail won't get lost. This makes sense to me as well. Things like permission problems and lack of disk space should be considered TEMPFAIL not PERMFAIL. > The diff below fixes this for mail.maildir and mail.mboxfile. > For mail.maildir I left the cases that appeared to be obvious > misconfiguration as PERMFAIL, but might be worth to convert these to > EX_TEMPFAIL as well. Looks good to me other than the first corrupted util.c diff and duplicated mail.mboxfile.c diff. OK millert@ - todd
Re: smtpd: mail.* tempfail vs permfail
September 25, 2019 3:23 PM, "Martijn van Duren"
wrote:
> EHLO,
EHLO,
> Recently I moved my mailserver to a new HD resulting in temporary
> inaccessibility of my maildir (temporarily root owned because of
> recursive copy), leaving smtpd running thinking nothing of it.
> This resulted in a few mails being PERMFAIL.
>
> I talked to gilles@ about this and he's somewhat inclined to agree with
> me that this is a TEMPFAIL situation. Even if the directory where the
> mail is supposed to be delivered is permanently not accessible I'd
> argue that we should keep this a TEMPFAIL, because an admin is inclined
> to spot this misconfiguration mistake earlier this way (e.g. via mailq
> monitoring) and mail won't get lost.
>
> The diff below fixes this for mail.maildir and mail.mboxfile.
> For mail.maildir I left the cases that appeared to be obvious
> misconfiguration as PERMFAIL, but might be worth to convert these to
> EX_TEMPFAIL as well.
>
> Note that mail.lmtp already uses EX_TEMPFAIL in all cases.
>
> While here I also removed the mkdirs from utils.c, which got me
> confused at first (it's also part of mail.maildir.c and not used
> anywhere else).
>
> thoughts? OK?
I'm generally ok with making mda's less strict when it comes to errors
that can genuinely happen from a misconfiguration. I don't think we'll
convert all errors because by buffering on our side in hope that we're
going to recover somehow, we're also delaying the permanent failure on
the sending host which is not always desirable. A lot of errors can be
made less strict for sure, we need to address them case by case.
The mkdirs from utils.c were left-overs from when we converted maildir
to be a real mda rather than an smtpd builtin, good riddance.
diff reads ok to me
> martijn@
>
> Index: mail.maildir.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/mail.maildir.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 mail.maildir.c
> --- mail.maildir.c 3 Jul 2019 03:24:03 - 1.12
> +++ mail.maildir.c 25 Sep 2019 13:21:36 -
> @@ -31,6 +31,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #define MAILADDR_ESCAPE "!#$%&'*/?^`{|}~"
> @@ -93,8 +94,11 @@ maildir_mkdirs(const char *dirname)
> char pathname[PATH_MAX];
> char *subdirs[] = { "cur", "tmp", "new" };
>
> - if (mkdirs(dirname, 0700) == -1 && errno != EEXIST)
> - err(1, NULL);
> + if (mkdirs(dirname, 0700) == -1 && errno != EEXIST) {
> + if (errno == EINVAL || errno == ENAMETOOLONG)
> + err(1, NULL);
> + err(EX_TEMPFAIL, NULL);
> + }
>
> for (i = 0; i < nitems(subdirs); ++i) {
> ret = snprintf(pathname, sizeof pathname, "%s/%s", dirname,
> @@ -102,7 +106,7 @@ maildir_mkdirs(const char *dirname)
> if (ret < 0 || (size_t)ret >= sizeof pathname)
> errc(1, ENAMETOOLONG, "%s/%s", dirname, subdirs[i]);
> if (mkdir(pathname, 0700) == -1 && errno != EEXIST)
> - err(1, NULL);
> + err(EX_TEMPFAIL, NULL);
> }
> }
>
> @@ -178,9 +182,9 @@ maildir_engine(const char *dirname, int
>
> fd = open(tmp, O_CREAT | O_EXCL | O_WRONLY, 0600);
> if (fd == -1)
> - err(1, NULL);
> + err(EX_TEMPFAIL, NULL);
> if ((fp = fdopen(fd, "w")) == NULL)
> - err(1, NULL);
> + err(EX_TEMPFAIL, NULL);
>
> while ((linelen = getline(, , stdin)) != -1) {
> line[strcspn(line, "\n")] = '\0';
> @@ -194,19 +198,19 @@ maildir_engine(const char *dirname, int
> }
> free(line);
> if (ferror(stdin))
> - err(1, NULL);
> + err(EX_TEMPFAIL, NULL);
>
> if (fflush(fp) == EOF ||
> ferror(fp) ||
> fsync(fd) == -1 ||
> fclose(fp) == EOF)
> - err(1, NULL);
> + err(EX_TEMPFAIL, NULL);
>
> (void)snprintf(new, sizeof new, "%s/new/%s",
> is_junk ? junkpath : dirname, filename);
>
> if (rename(tmp, new) == -1)
> - err(1, NULL);
> + err(EX_TEMPFAIL, NULL);
>
> exit(0);
> }
> @@ -223,8 +227,10 @@ mkdirs_component(const char *path, mode_
> if (mkdir(path, mode | S_IWUSR | S_IXUSR) == -1)
> return 0;
> }
> - else if (!S_ISDIR(sb.st_mode))
> + else if (!S_ISDIR(sb.st_mode)) {
> + errno = ENOTDIR;
> return 0;
> + }
>
> return 1;
> }
> @@ -238,12 +244,16 @@ mkdirs(const char *path, mode_t mode)
> const char *p;
>
> /* absolute path required */
> - if (*path != '/')
> + if (*path != '/') {
> + errno = EINVAL;
> return 0;
> + }
>
> /* make sure we don't exceed PATH_MAX */
> - if (strlen(path) >= sizeof buf)
> + if (strlen(path) >= sizeof buf) {
> + errno = ENAMETOOLONG;
> return 0;
> + }
>
> memset(buf, 0, sizeof buf);
> for (p = path; *p; p++) {
> Index: mail.mboxfile.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/mail.mboxfile.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 mail.mboxfile.c
> --- mail.mboxfile.c 28 Jun 2019 13:32:50 - 1.2
> +++ mail.mboxfile.c 25 Sep 2019 13:21:36 -
> @@ -26,6 +26,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> static void mboxfile_engine(const char *sender, const char *filename);
> @@ -73,10 +74,10 @@ mboxfile_engine(const char *sender,
smtpd: mail.* tempfail vs permfail
EHLO,
Recently I moved my mailserver to a new HD resulting in temporary
inaccessibility of my maildir (temporarily root owned because of
recursive copy), leaving smtpd running thinking nothing of it.
This resulted in a few mails being PERMFAIL.
I talked to gilles@ about this and he's somewhat inclined to agree with
me that this is a TEMPFAIL situation. Even if the directory where the
mail is supposed to be delivered is permanently not accessible I'd
argue that we should keep this a TEMPFAIL, because an admin is inclined
to spot this misconfiguration mistake earlier this way (e.g. via mailq
monitoring) and mail won't get lost.
The diff below fixes this for mail.maildir and mail.mboxfile.
For mail.maildir I left the cases that appeared to be obvious
misconfiguration as PERMFAIL, but might be worth to convert these to
EX_TEMPFAIL as well.
Note that mail.lmtp already uses EX_TEMPFAIL in all cases.
While here I also removed the mkdirs from utils.c, which got me
confused at first (it's also part of mail.maildir.c and not used
anywhere else).
thoughts? OK?
martijn@
Index: mail.maildir.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mail.maildir.c,v
retrieving revision 1.12
diff -u -p -r1.12 mail.maildir.c
--- mail.maildir.c 3 Jul 2019 03:24:03 - 1.12
+++ mail.maildir.c 25 Sep 2019 13:21:36 -
@@ -31,6 +31,7 @@
#include
#include
#include
+#include
#include
#defineMAILADDR_ESCAPE "!#$%&'*/?^`{|}~"
@@ -93,8 +94,11 @@ maildir_mkdirs(const char *dirname)
charpathname[PATH_MAX];
char*subdirs[] = { "cur", "tmp", "new" };
- if (mkdirs(dirname, 0700) == -1 && errno != EEXIST)
- err(1, NULL);
+ if (mkdirs(dirname, 0700) == -1 && errno != EEXIST) {
+ if (errno == EINVAL || errno == ENAMETOOLONG)
+ err(1, NULL);
+ err(EX_TEMPFAIL, NULL);
+ }
for (i = 0; i < nitems(subdirs); ++i) {
ret = snprintf(pathname, sizeof pathname, "%s/%s", dirname,
@@ -102,7 +106,7 @@ maildir_mkdirs(const char *dirname)
if (ret < 0 || (size_t)ret >= sizeof pathname)
errc(1, ENAMETOOLONG, "%s/%s", dirname, subdirs[i]);
if (mkdir(pathname, 0700) == -1 && errno != EEXIST)
- err(1, NULL);
+ err(EX_TEMPFAIL, NULL);
}
}
@@ -178,9 +182,9 @@ maildir_engine(const char *dirname, int
fd = open(tmp, O_CREAT | O_EXCL | O_WRONLY, 0600);
if (fd == -1)
- err(1, NULL);
+ err(EX_TEMPFAIL, NULL);
if ((fp = fdopen(fd, "w")) == NULL)
- err(1, NULL);
+ err(EX_TEMPFAIL, NULL);
while ((linelen = getline(, , stdin)) != -1) {
line[strcspn(line, "\n")] = '\0';
@@ -194,19 +198,19 @@ maildir_engine(const char *dirname, int
}
free(line);
if (ferror(stdin))
- err(1, NULL);
+ err(EX_TEMPFAIL, NULL);
if (fflush(fp) == EOF ||
ferror(fp) ||
fsync(fd) == -1 ||
fclose(fp) == EOF)
- err(1, NULL);
+ err(EX_TEMPFAIL, NULL);
(void)snprintf(new, sizeof new, "%s/new/%s",
is_junk ? junkpath : dirname, filename);
if (rename(tmp, new) == -1)
- err(1, NULL);
+ err(EX_TEMPFAIL, NULL);
exit(0);
}
@@ -223,8 +227,10 @@ mkdirs_component(const char *path, mode_
if (mkdir(path, mode | S_IWUSR | S_IXUSR) == -1)
return 0;
}
- else if (!S_ISDIR(sb.st_mode))
+ else if (!S_ISDIR(sb.st_mode)) {
+ errno = ENOTDIR;
return 0;
+ }
return 1;
}
@@ -238,12 +244,16 @@ mkdirs(const char *path, mode_t mode)
const char *p;
/* absolute path required */
- if (*path != '/')
+ if (*path != '/') {
+ errno = EINVAL;
return 0;
+ }
/* make sure we don't exceed PATH_MAX */
- if (strlen(path) >= sizeof buf)
+ if (strlen(path) >= sizeof buf) {
+ errno = ENAMETOOLONG;
return 0;
+ }
memset(buf, 0, sizeof buf);
for (p = path; *p; p++) {
Index: mail.mboxfile.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mail.mboxfile.c,v
retrieving revision 1.2
diff -u -p -r1.2 mail.mboxfile.c
--- mail.mboxfile.c 28 Jun 2019 13:32:50 - 1.2
+++ mail.mboxfile.c 25 Sep 2019 13:21:36 -
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
#include
static voidmboxfile_engine(const char *sender, const char *filename);
@@ -73,10 +74,10 @@ mboxfile_engine(const char *sender, cons
fd = open(filename, O_CREAT | O_APPEND | O_WRONLY | O_EXLOCK, 0600);
