Thanks for taking a look at the patch. I attached an updated version
that uses vstring primitives and adds the missing free.
On Thu, Jun 16, 2016 at 7:42 PM, Wietse Venema <wie...@porcupine.org> wrote:
> One complication is that the smtpd_resolve_addr cache is not only
> used for validating recipients, but also for validating senders.
> See check_sender_rcpt_maps(), which is used by reject_unlisted_sender
> and smtpd_reject_unlisted_sender.
>
> It don't know if your patch also makes that case work better, but
> I suppose it is sufficient if it does not make things worse.
I guess in the context of reject_unlisted_sender, there are two cases
where behavior would change.
Case 1: When postfix is configured to allow mail from the sender
address to itself, but not allow mail from an arbitrary address to the
sender. In this case, sender address will now be considered listed
instead of unlisted.
Case 2: When postfix is configured to not allow mail from the sender
address to itself, but to allow mail from an arbitrary address to the
sender. In this case, the sender address will now be considered
unlisted instead of listed.
Both cases seem like they should be pretty rare. For case 1, the
change seems to me like it would be an improvement. For case 2, the
configuration seems nonsensical to begin with so I think it would be
harmless.
> Always indexing the "resolve address" cache by sender+recipient
> makes the cache less effective for everyone, and I expect that
> almost no-one uses sender-dependent routing. So I may prefer to
> make that part dependent on whether sender-dependent routing is
> actually enabled.
I'm not sure if the cache is more intended to speed up repeated
smtpd_resolve_addr() calls during processing of a single message, or
to speed up processing of multiple messages that have the same
senders/recipients. If the cache exists mainly for the first reason,
then I agree the patch in it's current form would make it less
effective, because the new smtpd_resolve_addr_from(sender, recipient)
call won't be able to reuse the result of previous
smtpd_resolve_addr(recipient) calls for the same message. One way to
deal with this without having to add a new check for sender-dependent
routing might be to replace other smtpd_resolve_addr(recipient) calls
with smtpd_resolve_addr_from(sender, recipient) calls. I could try to
do this, or try to add a more specialized check for sender-dependent
routing, if you have a suggestion on how to determine whether it's
enabled.
diff --git a/src/smtpd/smtpd_check.c b/src/smtpd/smtpd_check.c
index 74e42d7..d2796e1 100644
--- a/src/smtpd/smtpd_check.c
+++ b/src/smtpd/smtpd_check.c
@@ -5108,7 +5108,7 @@ static int check_rcpt_maps(SMTPD_STATE *state, const char *recipient,
/*
* Resolve the address.
*/
- reply = smtpd_resolve_addr(recipient);
+ reply = smtpd_resolve_addr_from(state->sender, recipient);
if (reply->flags & RESOLVE_FLAG_FAIL)
reject_dict_retry(state, recipient);
diff --git a/src/smtpd/smtpd_resolve.c b/src/smtpd/smtpd_resolve.c
index 7cd8e4c..e20bf18 100644
--- a/src/smtpd/smtpd_resolve.c
+++ b/src/smtpd/smtpd_resolve.c
@@ -11,6 +11,10 @@
/*
/* const RESOLVE_REPLY *smtpd_resolve_addr(addr)
/* const char *addr;
+/*
+/* const RESOLVE_REPLY *smtpd_resolve_addr_from(sender, addr)
+/* const char *sender;
+/* const char *addr;
/* DESCRIPTION
/* This module maintains a resolve client cache that persists
/* across SMTP sessions (not process life times). Addresses
@@ -26,6 +30,8 @@
/* Arguments:
/* .IP cache_size
/* The requested cache size.
+/* .IP sender
+/* The message sender.
/* .IP addr
/* The address to resolve.
/* DIAGNOSTICS
@@ -70,15 +76,28 @@
static CTABLE *smtpd_resolve_cache;
#define STR(x) vstring_str(x)
+#define SENDER_SEPARATOR '\1'
/* resolve_pagein - page in an address resolver result */
-static void *resolve_pagein(const char *addr, void *unused_context)
+static void *resolve_pagein(const char* key, void *unused_context)
{
static VSTRING *query;
RESOLVE_REPLY *reply;
+ const char *addr;
+ VSTRING *sender;
char *tmp;
+ tmp = strchr(key, SENDER_SEPARATOR);
+ if (tmp) {
+ addr = tmp + 1;
+ sender = vstring_alloc(addr - key);
+ vstring_strncat(sender, key, tmp - key);
+ } else {
+ addr = key;
+ sender = 0;
+ }
+
/*
* Initialize on the fly.
*/
@@ -95,7 +114,12 @@ static void *resolve_pagein(const char *addr, void *unused_context)
* Resolve the address.
*/
rewrite_clnt_internal(MAIL_ATTR_RWR_LOCAL, addr, query);
- resolve_clnt_query(STR(query), reply);
+ if (sender) {
+ resolve_clnt_query_from(STR(sender), STR(query), reply);
+ vstring_free(sender);
+ } else {
+ resolve_clnt_query(STR(query), reply);
+ }
tmp = mystrdup(STR(reply->recipient));
casefold(reply->recipient, tmp); /* XXX */
myfree(tmp);
@@ -140,6 +164,15 @@ void smtpd_resolve_init(int cache_size)
const RESOLVE_REPLY *smtpd_resolve_addr(const char *addr)
{
+ return smtpd_resolve_addr_from(0, addr);
+}
+
+/* smtpd_resolve_addr_from - resolve cached address */
+
+const RESOLVE_REPLY *smtpd_resolve_addr_from(const char *sender, const char *addr)
+{
+ const void *reply;
+ VSTRING *key;
/*
* Sanity check.
@@ -150,5 +183,16 @@ const RESOLVE_REPLY *smtpd_resolve_addr(const char *addr)
/*
* Reply from the read-through cache.
*/
- return (const RESOLVE_REPLY *) ctable_locate(smtpd_resolve_cache, addr);
+ if (sender) {
+ key = vstring_alloc(strlen(sender) + strlen(addr) + 2);
+ vstring_strcat(key, sender);
+ VSTRING_ADDCH(key, SENDER_SEPARATOR);
+ vstring_strcat(key, addr);
+ reply = ctable_locate(smtpd_resolve_cache, STR(key));
+ vstring_free(key);
+ } else {
+ reply = ctable_locate(smtpd_resolve_cache, addr);
+ }
+
+ return (const RESOLVE_REPLY *)reply;
}
diff --git a/src/smtpd/smtpd_resolve.h b/src/smtpd/smtpd_resolve.h
index bfbc494..235e66a 100644
--- a/src/smtpd/smtpd_resolve.h
+++ b/src/smtpd/smtpd_resolve.h
@@ -18,6 +18,7 @@
*/
extern void smtpd_resolve_init(int);
extern const RESOLVE_REPLY *smtpd_resolve_addr(const char *);
+extern const RESOLVE_REPLY *smtpd_resolve_addr_from(const char*, const char *);
/* LICENSE
/* .ad
