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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL:
<http://dovecot.org/pipermail/dovecot/attachments/20080718/144a916f/attachment-0002.bin>
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.. -------------- next part -------------- A non-text attachment was scrubbed... Name: PGP.sig Type: application/pgp-signature Size: 194 bytes Desc: This is a digitally signed message part URL: <http://dovecot.org/pipermail/dovecot/attachments/20080718/7a67e67c/attachment-0002.bin>
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: signature.asc 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-0002.bin>