Hi,
I will get back to you tomorrow as I have a very busy day today.
There is a work-around which is to use the %{mbox.from} variable
which is what we do on mbox and lmtp to avoid empty senders, but
your diff makes sense from a fast read, I need to assess if this
comes with side-effects or not before committing.
Thanks for your mail !
Gilles
On Tue, Nov 27, 2018 at 06:47:14PM +0200, Lauri Tirkkonen wrote:
> Hi, I noticed in my maillog that smtpd is dropping bounce messages
> destined for me on the floor, like so:
>
> Nov 27 16:04:44 quickman smtpd[66508]: 0000000000000000 mda delivery
> evpid=75062f6aefbfab27 from=<> to=<[email protected]>
> rcpt=<[email protected]> user=lotheac delay=0s result=PermFail stat=Error
> ("smtpd: mda command line could not be expanded: No such file or directory")
>
> my smtpd.conf is as follows:
>
> # $OpenBSD: smtpd.conf,v 1.11 2018/06/04 21:10:58 jmc Exp $
>
> # This is the smtpd server system-wide configuration file.
> # See smtpd.conf(5) for more information.
>
> table aliases file:/etc/mail/aliases
>
> pki quickman.lotheac.fi cert "/etc/ssl/quickman.lotheac.fi.fullchain.pem"
> pki quickman.lotheac.fi key "/etc/ssl/private/quickman.lotheac.fi.key"
>
> # To accept external mail, replace with: listen on all
> #
> listen on all tls
> listen on lo0 port 10028 tag DKIM
>
> #action "local" maildir alias <aliases>
> action "local" mda "/etc/mail/mda.sh '%{sender}'"
> action "relay" relay
> action "relay_dkim" relay host smtp://127.0.0.1:10027
>
> table maildest { "lotheac.fi" "quickman.lotheac.fi" }
> # Uncomment the following to accept external mail for domain "example.org"
> #
> match from any for domain <maildest> action "local"
> match for local action "local"
> match tag DKIM for any action "relay"
> match for any action "relay_dkim"
>
>
> so, it seems that the %{sender} expansion is failing somehow. From
> smtpd.conf(5):
>
> %{sender} sender email address, may be empty string
>
> clearly, in this case it *should* expand to the empty string - there is
> no envelope sender. However mda_expand_token returns 0 if the string
> ends up empty:
>
> 204 /* expanded string is empty */
> 205 i = strlen(string);
> 206 if (i == 0)
> 207 return 0;
>
> and it does, because above 'string' has been set to the empty string
> (lines 119-121):
>
> 115 if (!strcasecmp("sender", rtoken)) {
> 116 if (snprintf(tmp, sizeof tmp, "%s@%s",
> 117 dlv->sender.user, dlv->sender.domain) >=
> (int)sizeof tmp)
> 118 return -1;
> 119 if (strcmp(tmp, "@") == 0)
> 120 (void)strlcpy(tmp, "", sizeof tmp);
> 121 string = tmp;
> 122 }
>
> As an aside the error exit happens with err(), but mda_expand_format
> doesn't actually set errno, so ENOENT is not actually what is happening.
> Maybe that's another bug.
>
> So, mda_expand_format() assumes that a 0 retval from mda_expand_token()
> is an error, even though it's a valid value (ie. the length of the empty
> string). Maybe it should return (size_t)-1 instead for errors? Diff for
> that below, it makes the expansion work for me.
>
> I found this on 6.4, but also applies to -CURRENT.
>
> diff --git a/usr.sbin/smtpd/mda_variables.c b/usr.sbin/smtpd/mda_variables.c
> index a23112c71e8..0a1605772f2 100644
> --- a/usr.sbin/smtpd/mda_variables.c
> +++ b/usr.sbin/smtpd/mda_variables.c
> @@ -75,14 +75,14 @@ mda_expand_token(char *dest, size_t len, const char
> *token,
> mods = NULL;
>
> if (strlcpy(rtoken, token, sizeof rtoken) >= sizeof rtoken)
> - return 0;
> + return -1;
>
> /* token[x[:y]] -> extracts optional x and y, converts into offsets */
> if ((lbracket = strchr(rtoken, '[')) &&
> (rbracket = strchr(rtoken, ']'))) {
> /* ] before [ ... or empty */
> if (rbracket < lbracket || rbracket - lbracket <= 1)
> - return 0;
> + return -1;
>
> *lbracket = *rbracket = '\0';
> content = lbracket + 1;
> @@ -102,7 +102,7 @@ mda_expand_token(char *dest, size_t len, const char
> *token,
> }
> }
> if (errstr)
> - return 0;
> + return -1;
>
> /* token:mod_1,mod_2,mod_n -> extract modifiers */
> mods = strchr(rbracket + 1, ':');
> @@ -115,7 +115,7 @@ mda_expand_token(char *dest, size_t len, const char
> *token,
> if (!strcasecmp("sender", rtoken)) {
> if (snprintf(tmp, sizeof tmp, "%s@%s",
> dlv->sender.user, dlv->sender.domain) >= (int)sizeof
> tmp)
> - return 0;
> + return -1;
> if (strcmp(tmp, "@") == 0)
> (void)strlcpy(tmp, "", sizeof tmp);
> string = tmp;
> @@ -123,7 +123,7 @@ mda_expand_token(char *dest, size_t len, const char
> *token,
> else if (!strcasecmp("rcpt", rtoken)) {
> if (snprintf(tmp, sizeof tmp, "%s@%s",
> dlv->rcpt.user, dlv->rcpt.domain) >= (int)sizeof tmp)
> - return 0;
> + return -1;
> if (strcmp(tmp, "@") == 0)
> (void)strlcpy(tmp, "", sizeof tmp);
> string = tmp;
> @@ -131,7 +131,7 @@ mda_expand_token(char *dest, size_t len, const char
> *token,
> else if (!strcasecmp("dest", rtoken)) {
> if (snprintf(tmp, sizeof tmp, "%s@%s",
> dlv->dest.user, dlv->dest.domain) >= (int)sizeof tmp)
> - return 0;
> + return -1;
> if (strcmp(tmp, "@") == 0)
> (void)strlcpy(tmp, "", sizeof tmp);
> string = tmp;
> @@ -161,17 +161,17 @@ mda_expand_token(char *dest, size_t len, const char
> *token,
> else if (!strcasecmp("mbox.from", rtoken)) {
> if (snprintf(tmp, sizeof tmp, "%s@%s",
> dlv->sender.user, dlv->sender.domain) >= (int)sizeof
> tmp)
> - return 0;
> + return -1;
> if (strcmp(tmp, "@") == 0)
> (void)strlcpy(tmp, "MAILER-DAEMON", sizeof tmp);
> string = tmp;
> }
> else
> - return 0;
> + return -1;
>
> if (string != tmp) {
> if (strlcpy(tmp, string, sizeof tmp) >= sizeof tmp)
> - return 0;
> + return -1;
> string = tmp;
> }
>
> @@ -187,12 +187,12 @@ mda_expand_token(char *dest, size_t len, const char
> *token,
> break;
> }
> if (!token_modifiers[i].f(tmp, sizeof
> tmp))
> - return 0; /* modifier error */
> + return -1; /* modifier error */
> break;
> }
> }
> if ((size_t)i == nitems(token_modifiers))
> - return 0; /* modifier not found */
> + return -1; /* modifier not found */
> } while ((mods = sep) != NULL);
> }
>
> @@ -208,7 +208,7 @@ mda_expand_token(char *dest, size_t len, const char
> *token,
>
> /* begin offset beyond end of string */
> if (begoff >= i)
> - return 0;
> + return -1;
>
> /* end offset beyond end of string, make it end of string */
> if (endoff >= i)
> @@ -225,13 +225,13 @@ mda_expand_token(char *dest, size_t len, const char
> *token,
>
> /* check that final offsets are valid */
> if (begoff < 0 || endoff < 0 || endoff < begoff)
> - return 0;
> + return -1;
> endoff += 1; /* end offset is inclusive */
>
> /* check that substring does not exceed destination buffer length */
> i = endoff - begoff;
> if ((size_t)i + 1 >= len)
> - return 0;
> + return -1;
>
> string += begoff;
> for (; i; i--) {
> @@ -308,7 +308,7 @@ mda_expand_format(char *buf, size_t len, const struct
> deliver *dlv,
>
> exptoklen = mda_expand_token(exptok, sizeof exptok, token, dlv,
> ui, mda_command);
> - if (exptoklen == 0)
> + if (exptoklen == (size_t)-1)
> return 0;
>
> /* writing expanded token at ptmp will overflow tmpbuf */
>
> --
> Lauri Tirkkonen | lotheac @ IRCnet
>
--
Gilles Chehade @poolpOrg
https://www.poolp.org tip me: https://paypal.me/poolpOrg
