Re: [Dovecot] quota vs. antispam issue
Johannes Berg wrote: Ok, I've committed that, so it should work now. Somebody please test. I guess the extra semicolon (after save_init) is a typo? - return asbox-module_ctx.super.save_init(t, flags, keywords, received_date, + ret = asbox-module_ctx.super.save_init;(t, flags, keywords, received_date, Cheers, Anders.
Re: [Dovecot] quota vs. antispam issue
I will test it tomorrow. Thanks for your work. 2008/7/24 Johannes Berg [EMAIL PROTECTED]: On Mon, 2008-07-21 at 15:26 +0300, Timo Sirainen wrote: On Mon, 2008-07-21 at 06:48 +0200, Johannes Berg wrote: On Sun, 2008-07-20 at 23:53 +0300, Timo Sirainen wrote: Ok, that seems to work, but I think a better alternative would probably be to make dest_mail a struct mail ** like the context. That'd be an API change and I'd rather not do that for v1.1. But I suppose it would be the best permanent solution, so I'll do that for v1.2. Right, yeah, it'd be an API change, though I suppose the only external plugin is probably mine ;) If you wanted to do it you could make some header file declare a macro SAVE_FINISH_HAS_STRUCT_MAIL_PP, but I'm ok with doing it in 1.2, except that means that during 1.1 antispam and quota cannot be used together. No, this should help with v1.1: http://hg.dovecot.org/dovecot-1.1/rev/8dc6541b4426 You could then do it like quota plugin and I think it should work. Ok, I've committed that, so it should work now. Somebody please test. johannes
Re: [Dovecot] quota vs. antispam issue
On Sun, 2008-07-20 at 23:53 +0300, Timo Sirainen wrote: Ok, that seems to work, but I think a better alternative would probably be to make dest_mail a struct mail ** like the context. That'd be an API change and I'd rather not do that for v1.1. But I suppose it would be the best permanent solution, so I'll do that for v1.2. Right, yeah, it'd be an API change, though I suppose the only external plugin is probably mine ;) If you wanted to do it you could make some header file declare a macro SAVE_FINISH_HAS_STRUCT_MAIL_PP, but I'm ok with doing it in 1.2, except that means that during 1.1 antispam and quota cannot be used together. How about these: http://hg.dovecot.org/dovecot-1.1/rev/8dc6541b4426 http://hg.dovecot.org/dovecot-1.2/rev/dc280df713f4 Will check them out when I'm done travelling, sitting on a train right now without connectivity. The problem isn't flags, it's that if the dest_mail is non-NULL then the mail storage backends must assume that caller wants to do something with the mail, so it should be added to index. For example with mbox if dest_mail=NULL the mails are added to index only if the mbox is already fully synchronized. With dest_mail!=NULL the mbox always gets synchronized (which could be slow). Ok, I didn't know that part, thanks for the explanation. johannes signature.asc Description: This is a digitally signed message part
Re: [Dovecot] quota vs. antispam issue
On Mon, 2008-07-21 at 06:48 +0200, Johannes Berg wrote: On Sun, 2008-07-20 at 23:53 +0300, Timo Sirainen wrote: Ok, that seems to work, but I think a better alternative would probably be to make dest_mail a struct mail ** like the context. That'd be an API change and I'd rather not do that for v1.1. But I suppose it would be the best permanent solution, so I'll do that for v1.2. Right, yeah, it'd be an API change, though I suppose the only external plugin is probably mine ;) If you wanted to do it you could make some header file declare a macro SAVE_FINISH_HAS_STRUCT_MAIL_PP, but I'm ok with doing it in 1.2, except that means that during 1.1 antispam and quota cannot be used together. No, this should help with v1.1: http://hg.dovecot.org/dovecot-1.1/rev/8dc6541b4426 You could then do it like quota plugin and I think it should work. signature.asc Description: This is a digitally signed message part
Re: [Dovecot] quota vs. antispam issue
On Fri, 2008-07-18 at 20:12 +0200, Johannes Berg wrote: On Fri, 2008-07-18 at 20:58 +0300, Timo Sirainen wrote: So the quota code eventually sees both ctx-dest_mail = NULL and qt- tmp_mail = NULL. I'm not really sure what the right fix for this is.. ctx-dest_mail should be set by something. Perhaps if quota/ antispam overrides it it should set it, and mailbox_save_init() shouldn't set it if it's already set.. Ok, that seems to work, but I think a better alternative would probably be to make dest_mail a struct mail ** like the context. That'd be an API change and I'd rather not do that for v1.1. But I suppose it would be the best permanent solution, so I'll do that for v1.2. How about these: http://hg.dovecot.org/dovecot-1.1/rev/8dc6541b4426 http://hg.dovecot.org/dovecot-1.2/rev/dc280df713f4 Alternatively, we could ensure that dest_mail is never NULL to start with, and have mailbox_save_init() create one, but I'm not sure about the performance implications of that. The plugins could, if that's possible (haven't checked) set those flags of what they require, and if there are no plugins then a struct mail with no MAIL_FETCH_* flags wouldn't hurt much, would it? The problem isn't flags, it's that if the dest_mail is non-NULL then the mail storage backends must assume that caller wants to do something with the mail, so it should be added to index. For example with mbox if dest_mail=NULL the mails are added to index only if the mbox is already fully synchronized. With dest_mail!=NULL the mbox always gets synchronized (which could be slow). signature.asc Description: This is a digitally signed message part
[Dovecot] quota vs. antispam issue
Can you help me maybe? I don't see the bug.
QUOTA=maildir QUOTA_RULE='*:storage=100M' MAIL_PLUGINS=antispam quota
MAIL_PLUGIN_DIR=/home/johannes/Projects/dovecot/antispam gdb --args
/home/johannes/Projects/dovecot/dovecot-1.1/src/imap/imap
GNU gdb 6.8-debian
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later http://gnu.org/licenses/gpl.html
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law. Type show copying
and show warranty for details.
This GDB was configured as powerpc-linux-gnu...
(gdb) run
Starting program: /home/johannes/Projects/dovecot/dovecot-1.1/src/imap/imap
* PREAUTH [CAPABILITY IMAP4rev1 SASL-IR SORT THREAD=REFERENCES MULTIAPPEND
UNSELECT LITERAL+ IDLE CHILDREN NAMESPACE LOGIN-REFERRALS UIDPLUS LIST-EXTENDED
I18NLEVEL=1] Logged in as johannes
01 select SPAM
* FLAGS (\Answered \Flagged \Deleted \Seen \Draft)
* OK [PERMANENTFLAGS (\Answered \Flagged \Deleted \Seen \Draft \*)] Flags
permitted.
* 8 EXISTS
* 0 RECENT
* OK [UIDVALIDITY 1212140311] UIDs valid
* OK [UIDNEXT 9] Predicted next UID
01 OK [READ-WRITE] Select completed.
A003 APPEND SPAM () {2}
+ OK
ab
Program received signal SIGSEGV, Segmentation fault.
0x100929f4 in mail_get_physical_size (mail=0x0, size_r=0xbfeaf030) at mail.c:100
100 return p-v.get_physical_size(mail, size_r);
(gdb) bt
#0 0x100929f4 in mail_get_physical_size (mail=0x0, size_r=0xbfeaf030) at
mail.c:100
#1 0x0fe29be0 in quota_try_alloc (ctx=0x101707a8, mail=0x0,
too_large_r=0xbfeaf060)
at quota.c:818
#2 0x0fe303dc in quota_check (t=0x10170158, mail=0x0) at quota-storage.c:148
#3 0x0fe30968 in quota_save_finish (ctx=0x10177c10) at quota-storage.c:251
#4 0x0fdfec58 in antispam_save_finish (ctx=0x10177c10) at
antispam-storage-1.1.c:178
#5 0x10097c94 in mailbox_save_finish (_ctx=0x10159004) at mail-storage.c:738
#6 0x10016988 in cmd_append_continue_message (cmd=0x10158f98) at
cmd-append.c:408
#7 0x10015804 in client_input_append (cmd=0x10158f98) at cmd-append.c:79
#8 0x101029e0 in io_loop_handler_run (ioloop=0x10154aa8) at ioloop-epoll.c:201
#9 0x10101514 in io_loop_run (ioloop=0x10154aa8) at ioloop.c:308
#10 0x1003094c in main (argc=1, argv=0xbfeaf4b4, envp=0xbfeaf4bc) at main.c:293
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.
johannes
signature.asc
Description: This is a digitally signed message part
Re: [Dovecot] quota vs. antispam issue
On Jul 18, 2008, at 8:40 PM, Johannes Berg wrote: Can you help me maybe? I don't see the bug. .. 0x100929f4 in mail_get_physical_size (mail=0x0, size_r=0xbfeaf030) at mail.c:100 100 return p-v.get_physical_size(mail, size_r); (gdb) bt #0 0x100929f4 in mail_get_physical_size (mail=0x0, size_r=0xbfeaf030) at mail.c:100 #1 0x0fe29be0 in quota_try_alloc (ctx=0x101707a8, mail=0x0, too_large_r=0xbfeaf060) at quota.c:818 .. return quota_check(ctx-transaction, ctx-dest_mail != NULL ? ctx-dest_mail : qt-tmp_mail); I recently wondered about that code. The problem is: 1. save_init() is called with dest_mail=NULL 2. antispam sees that dest_mail=NULL and sets it, and calls super.save_init() 3. quota sees that dest_mail != NULL so it doesn't set qt-tmp_mail 4. mailbox_save_init() stores ctx-dest_mail = NULL (because it doesn't see the updated value) So the quota code eventually sees both ctx-dest_mail = NULL and qt- tmp_mail = NULL. I'm not really sure what the right fix for this is.. ctx-dest_mail should be set by something. Perhaps if quota/ antispam overrides it it should set it, and mailbox_save_init() shouldn't set it if it's already set.. PGP.sig Description: This is a digitally signed message part
Re: [Dovecot] quota vs. antispam issue
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
signature.asc
Description: This is a digitally signed message part
Re: [Dovecot] quota vs. antispam issue
I recently wondered about that code. The problem is: 1. save_init() is called with dest_mail=NULL 2. antispam sees that dest_mail=NULL and sets it, and calls super.save_init() 3. quota sees that dest_mail != NULL so it doesn't set qt-tmp_mail 4. mailbox_save_init() stores ctx-dest_mail = NULL (because it doesn't see the updated value) Good. I just analysed it down to the same thing :) So the quota code eventually sees both ctx-dest_mail = NULL and qt- tmp_mail = NULL. I'm not really sure what the right fix for this is.. ctx-dest_mail should be set by something. Perhaps if quota/ antispam overrides it it should set it, and mailbox_save_init() shouldn't set it if it's already set.. Ok, so mailbox_save_init() is the code I said about that it only sets it up later. johannes signature.asc Description: This is a digitally signed message part
Re: [Dovecot] quota vs. antispam issue
On Fri, 2008-07-18 at 20:58 +0300, Timo Sirainen wrote: So the quota code eventually sees both ctx-dest_mail = NULL and qt- tmp_mail = NULL. I'm not really sure what the right fix for this is.. ctx-dest_mail should be set by something. Perhaps if quota/ antispam overrides it it should set it, and mailbox_save_init() shouldn't set it if it's already set.. Ok, that seems to work, but I think a better alternative would probably be to make dest_mail a struct mail ** like the context. Alternatively, we could ensure that dest_mail is never NULL to start with, and have mailbox_save_init() create one, but I'm not sure about the performance implications of that. The plugins could, if that's possible (haven't checked) set those flags of what they require, and if there are no plugins then a struct mail with no MAIL_FETCH_* flags wouldn't hurt much, would it? johannes signature.asc Description: This is a digitally signed message part
