[Dovecot] quota vs. antispam issue

[Johannes Berg]

On Fri, 2008-07-18 at 19:40 +0200, Johannes Berg wrote:
> Can you help me maybe? I don't see the bug.

> I'll keep digging but I don't see why 
> 
>         return quota_check(ctx->transaction, ctx->dest_mail != NULL ?
>                            ctx->dest_mail : qt->tmp_mail);
> 
> should pass NULL in the second argument.

Ok, I think I see the issue, and I think both quota and antispam
(because I copied from quota) have it.

Consider quota_save_init:

quota_save_init(struct mailbox_transaction_context *t,
                enum mail_flags flags, struct mail_keywords *keywords,
                time_t received_date, int timezone_offset,
                const char *from_envelope, struct istream *input,
                struct mail *dest_mail, struct mail_save_context **ctx_r)
...
        if (dest_mail == NULL) {
                /* we always want to know the mail size */
                if (qt->tmp_mail == NULL) {
                        qt->tmp_mail = mail_alloc(t, MAIL_FETCH_PHYSICAL_SIZE,
                                                  NULL);
                }
                dest_mail = qt->tmp_mail;
        }

        return qbox->module_ctx.super.
                save_init(t, flags, keywords, received_date,   
                          timezone_offset, from_envelope,
                          input, dest_mail, ctx_r);
}


As you can see, this ends up always passing a non-NULL dest_mail into
super.save_init(). Now, antispam has the same code, and let's assume
that super.save_init() is quota_save_init, the code above. It will
always be passed a dest_mail which is non-NULL, thus qt->tmp_mail will
always be NULL.

Now, consider quota_save_finish. It does not get an explicit dest_mail,
so it takes it from the mail_save_context. There, however, it ends up
being NULL because antispam created the mail and not whatever fills the
mail_save_context. Hence, _both_ ctx->dest_mail and qt->tmp_mail end up
being NULL.

I think the problem is caused by the explicit/implicit API difference.
The quick fix would probably be to assign dest_mail also to the context
in quota_save_init, like this:

diff -r ffbe9f9e0376 src/plugins/quota/quota-storage.c
--- a/src/plugins/quota/quota-storage.c	Fri Jul 18 17:55:02 2008 +0300
+++ b/src/plugins/quota/quota-storage.c	Fri Jul 18 19:50:53 2008 +0200
@@ -233,10 +233,14 @@
 		dest_mail = qt->tmp_mail;
 	}
 
-	return qbox->module_ctx.super.
+	ret = qbox->module_ctx.super.
 		save_init(t, flags, keywords, received_date,
 			  timezone_offset, from_envelope,
 			  input, dest_mail, ctx_r);
+
+	(*ctx_r)->dest_mail = dest_mail;
+
+	return ret;
 }
 
 static int quota_save_finish(struct mail_save_context *ctx)


Except, that doesn't work, the dest_mail in the context is still NULL
again although the context is the same, so whichever code sets up the
context apparently only sets it up after our plugin returns, and the
plugin doesn't have access to it.

I think actually fixing it requires changes in the storage API.

johannes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
Url : http://dovecot.org/pipermail/dovecot/attachments/20080718/202028a0/attachment-0001.bin