[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
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:
- save_init() is called with dest_mail=NULL
- antispam sees that dest_mail=NULL and sets it, and calls
super.save_init() - quota sees that dest_mail != NULL so it doesn't set qt->tmp_mail
- 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..
I recently wondered about that code. The problem is:
- save_init() is called with dest_mail=NULL
- antispam sees that dest_mail=NULL and sets it, and calls
super.save_init()- quota sees that dest_mail != NULL so it doesn't set qt->tmp_mail
- 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
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
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).
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
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:
You could then do it like quota plugin and I think it should work.
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:
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
I will test it tomorrow.
Thanks for your work.
2008/7/24 Johannes Berg johannes@sipsolutions.net:
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:
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
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?
received_date,return asbox->module_ctx.super.save_init(t, flags, keywords,
received_date,ret = asbox->module_ctx.super.save_init;(t, flags, keywords,
Cheers, Anders.
Yes
I didn't realise that error, so the plugin did not work correctly. After fixing it (remove the ;), recompile, the plugin looks to work fine.
Thank you very much again.
2008/7/25 Anders mail@flac.kalibalik.dk:
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,
ret = asbox->module_ctx.super.save_init;(t, flags, keywords,Cheers, Anders.
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
participants (4)
-
Anders
-
Johannes Berg
-
Juan Asensio Sánchez
-
Timo Sirainen