[vchkpw] [SPAM] Bug in maildirquota.c
Hi everybody.
I've been hit by a bug in vdelivermail (in maildirquota.c, precisely), and I
want to share the
resolution I've found.
I've recently upgraded to vpopmail 5.4.33, and we experienced that occasionally
(once o twice a day) vdelivermail starts looping, eating all the CPU.
An strace on the offending instance resulted in:
read(5, 0xf5e4317, 2963227893) = -1 EINVAL (Invalid argument)
read(5, 0xf5e4316, 2963227894) = -1 EINVAL (Invalid argument)
read(5, 0xf5e4315, 2963227895) = -1 EINVAL (Invalid argument)
read(5, 0xf5e4314, 2963227896) = -1 EINVAL (Invalid argument)
read(5, 0xf5e4313, 2963227897) = -1 EINVAL (Invalid argument)
read(5, 0xf5e4312, 2963227898) = -1 EINVAL (Invalid argument)
read(5, 0xf5e4311, 2963227899) = -1 EINVAL (Invalid argument)
read(5, 0xf5e4310, 2963227900) = -1 EINVAL (Invalid argument)
read(5, 0xf5e430f, 2963227901) = -1 EINVAL (Invalid argument)
read(5, 0xf5e430e, 2963227902) = -1 EINVAL (Invalid argument)
read(5, 0xf5e430d, 2963227903) = -1 EINVAL (Invalid argument)
read(5, 0xf5e430c, 2963227904) = -1 EINVAL (Invalid argument)
...
...
The file descriptor number 5 of that process is pointing at the maildirsize
file of the mailbox, and is marked as deleted in /proc/procid/fd
We store the mail in a NetApp NFS share, without locking for performance
reasons, and I think I've found the problem:
the file is deleted by someone else while vdelivermail is reading it.
In maildirquota.c, function maildirsize_read, there is a while loop that reads:
while (l)
{
n=read(f, p, l);
if (n 0)
{
But n is defined as a unsigned int (64 bit) , so even if read returns a
negative value (error) the if is never trigged.
So I've made this patch:
--- maildirquota.c.orig 2014-01-31 12:21:22.0 +0100
+++ maildirquota.c 2014-01-31 12:08:47.0 +0100
@@ -337,7 +337,6 @@
int f;
char *p;
unsigned l;
- storage_t n;
int first;
int ret = 0;
@@ -360,15 +359,16 @@
while (l)
{
- n=read(f, p, l);
- if (n 0)
+ ssize_t nr;
+ nr=read(f, p, l);
+ if (nr 0)
{
close(f);
return (-1);
}
- if (n == 0) break;
- p += n;
- l -= n;
+ if (nr == 0)break;
+ p += nr;
+ l -= nr;
}
if (l == 0 || ret) /* maildir too big */
{
which fixes the problem.
Any chance to incorporate the fix in the next version ?
Thanks
--
Simone Lazzaris
QCom S.p.A.
!DSPAM:52eb8a1334261024417156!
[vchkpw] [SPAM] Bug in maildirquota.c
Hi everybody.
I've been hit by a bug in vdelivermail (in maildirquota.c, precisely), and I
want to share the
resolution I've found.
I've recently upgraded to vpopmail 5.4.33, and we experienced that occasionally
(once o twice a day) vdelivermail starts looping, eating all the CPU.
An strace on the offending instance resulted in:
read(5, 0xf5e4317, 2963227893) = -1 EINVAL (Invalid argument)
read(5, 0xf5e4316, 2963227894) = -1 EINVAL (Invalid argument)
read(5, 0xf5e4315, 2963227895) = -1 EINVAL (Invalid argument)
read(5, 0xf5e4314, 2963227896) = -1 EINVAL (Invalid argument)
read(5, 0xf5e4313, 2963227897) = -1 EINVAL (Invalid argument)
read(5, 0xf5e4312, 2963227898) = -1 EINVAL (Invalid argument)
read(5, 0xf5e4311, 2963227899) = -1 EINVAL (Invalid argument)
read(5, 0xf5e4310, 2963227900) = -1 EINVAL (Invalid argument)
read(5, 0xf5e430f, 2963227901) = -1 EINVAL (Invalid argument)
read(5, 0xf5e430e, 2963227902) = -1 EINVAL (Invalid argument)
read(5, 0xf5e430d, 2963227903) = -1 EINVAL (Invalid argument)
read(5, 0xf5e430c, 2963227904) = -1 EINVAL (Invalid argument)
...
...
The file descriptor number 5 of that process is pointing at the maildirsize
file of the mailbox, and is marked as deleted in /proc/procid/fd
We store the mail in a NetApp NFS share, without locking for performance
reasons, and I think I've found the problem:
the file is deleted by someone else while vdelivermail is reading it.
In maildirquota.c, function maildirsize_read, there is a while loop that reads:
while (l)
{
n=read(f, p, l);
if (n 0)
{
But n is defined as a unsigned int (64 bit) , so even if read returns a
negative value (error) the if is never trigged.
So I've made this patch:
--- maildirquota.c.orig 2014-01-31 12:21:22.0 +0100
+++ maildirquota.c 2014-01-31 12:08:47.0 +0100
@@ -337,7 +337,6 @@
int f;
char *p;
unsigned l;
- storage_t n;
int first;
int ret = 0;
@@ -360,15 +359,16 @@
while (l)
{
- n=read(f, p, l);
- if (n 0)
+ ssize_t nr;
+ nr=read(f, p, l);
+ if (nr 0)
{
close(f);
return (-1);
}
- if (n == 0) break;
- p += n;
- l -= n;
+ if (nr == 0)break;
+ p += nr;
+ l -= nr;
}
if (l == 0 || ret) /* maildir too big */
{
which fixes the problem.
Any chance to incorporate the fix in the next version ?
Thanks
--
Simone Lazzaris
QCom S.p.A.
!DSPAM:52eb8b0234261033718879!
Re: [vchkpw] Patch to disable vusaged
In data giovedì 03 settembre 2009 hai scritto: Simone Lazzaris wrote: Ok, but how can be syncronized the two vision of the quota, if only vpopmail uses vusaged ? I think that there can be only two cases 1) all tools use vusaged or 2) all tools use traditional Maildir++ quota. In any other combination, the two vision of the real maildir quota will go quickly out of sync. How would they go out of sync? They're both calculating disk usage. Any variances would be slight. I'm assuming your quotas are in the range of megabytes or more. If they're off by just a few kilobytes, what's the problem? Well, if I only put mail on the storage via vusaged, and only fetch mail via Maildir++, that means that the usage values will always be seriously wrong, right? I mean, vusaged will always see a positive usage, and Maildir++ always a negative one. They only will be right just after a resync with the filesystem, invalidating the purpose of both. In that case, it will be better to NOT have a quota tracking system. And that is precisely MY case. The whole point of the vusage daemon is to remove the overhead of exact quota calculation for a gain of efficiency. Todays quotas are large enough that a few kilobytes-- heck, even a few megabytes, or a few minutes of delay in actual vs reported disk usage, are unimportant. Having any process care about exact numbers is silly as long as the numbers being reported are relatively close. Futhermore, having more than one process actually accessing the disk is bad. Maildir++ is an implementation that works well, but fortunately, due to it's rather open-endedness, can easily be improved upon. 5.4 is not accepting feature changes, though I will look into adding a disable option into 5.5. Perhaps removing the fprintf calls would be enough. Further support for Maildir++ integration will be coming as well, so perhaps there will be no need for this type of patch anyway :) Removing the fprintf is ok for MY need, but can complicate the life of peoples who *does* want the vusaged to work, but forgot to set in place its configuration file, or have a syntax error in it. In that case, you want to have fprintfs that tells you what is wrong and why. That's why I thought of a configuration setting. So maybe the best thing to do is put in place a configuration option that let the sysadmin choose the usage tracking backend, Maildir++ or vusaged -- Simone Lazzaris INTERACTIVE NETWORK SRL Via Roggia Vignola 9, 24047 Treviglio (BG) tel : +39 0363.302820 fax : +39 0363.304352 web : http://www.interactive.eu email : s.lazza...@interactive.eu signature.asc Description: This is a digitally signed message part. !DSPAM:4aa0beb432711324013116!
Re: [vchkpw] Patch to disable vusaged
In data venerdì 04 settembre 2009 hai scritto: Simone Lazzaris wrote: Well, if I only put mail on the storage via vusaged, and only fetch mail via Maildir++, that means that the usage values will always be seriously wrong, right? Ah, you think that vusaged is a delivery agent, or that it needs to know about it's delivery agent to determine usage. It is not, and it does not. In the most simple form, it's just doing a 'du' on the directory. No. I'm aware that vusaged is only a usage tracker daemon; my concern is that I don't see the point of running a daemon which cannot possibly have information on the usage short of performing du on a directory. Directory that is mounted on a network appliance. And, frankly, any installation of a significative size is going to have the storage on a NAS. Removing the fprintf is ok for MY need, but can complicate the life of peoples who *does* want the vusaged to work, but forgot to set in place its configuration file, or have a syntax error in it. So maybe the best thing to do is put in place a configuration option that let the sysadmin choose the usage tracking backend, Maildir++ or vusaged Actually, the original problem you reported was that the client_connect connect() errors were cluttering up your logs, and that a missing vusagec.conf caused the client API to segfault. This had nothing to do with syntax errors in a configuration file, which would be reported regardless of whether you remove the fprintfs in client_connect. Ok. But my only concern about removing tout court the fprintf is that, in case a user should want to use vusaged but forgets to put the conf file in place, [s]he would have an hard time figuring out whats missing. In that case, the fprintf is useful. I thought that should be better explicitly tell the program what I want to do, than let it guess. The defaults should be safe. You're writing about concerns about superficial fprintfs in 5.4, a frozen version, that's why it makes sense to just comment out the fprintfs. 5.5 is a *much* different version, which treats the Maildir++ quota checking as broken, which it is. Ok, if the solution is to comment out some annoying fprintf, it'OK for me. But if the power of the free software is to let people choose, I'll be glad to choose what is already working for me. So I think that having an option (compile time, in configuration or whatever) to keep only the Maildir++ method is the right thing to have. -- Simone Lazzaris INTERACTIVE NETWORK SRL Via Roggia Vignola 9, 24047 Treviglio (BG) tel : +39 0363.302820 fax : +39 0363.304352 web : http://www.interactive.eu email : s.lazza...@interactive.eu signature.asc Description: This is a digitally signed message part. !DSPAM:4aa12cce32711674082549!
[vchkpw] Patch to disable vusaged
Hi everybody;
we are using vpopmail in a deployment which involves many programs.
We use maildrop for some (but not all) deliveries, dovecot for imap access,
some custom perl script to forward some mail.
Log story short, we cannot use (at the moment) vusaged to track the mail
usage.
I've looked the code and found that there were no option to disable the usage.
I can avoid to start the server, of course, but that will pollute the log
files with failure messages as the client cannot connect.
So I've wrote this patch to add a configuration option to vusagec.conf,
disabling the connection altogether.
Here is the patch:
--- ../vpopmail-5.4.28/client.c 2009-08-24 17:56:04.0 +0200
+++ client.c2009-09-03 09:32:50.0 +0200
@@ -74,6 +74,15 @@
fprintf(stderr, client_connect: warning: config_begin failed\n);
/*
+ Check if vusaged is disabled
+ */
+ str = config_fetch_by_name(config, Global, Disable);
+ if (str) {
+ fl = atoi(str);
+ if (fl == 1)
+ return 0;
+ }
+ /*
Get timeout
*/
I hope that will be included in the next releases of vpopmail.
I will also be nice if (adding a return 0; at line 74, and a couple of curly
braces around that) vdelivermail wouldn't segfault if the config file is
missing or unparsable.
Now, at version 5.4.28, if vusagec.conf is missing, the code try to proceed
and segfault.
Thanks for the attention
--
Simone Lazzaris
INTERACTIVE NETWORK SRL
Via Roggia Vignola 9, 24047 Treviglio (BG)
tel : +39 0363.302820
fax : +39 0363.304352
web : http://www.interactive.eu
email : s.lazza...@interactive.eu
signature.asc
Description: This is a digitally signed message part.
!DSPAM:4a9fd55c32711506825904!
Re: [vchkpw] Patch to disable vusaged
In data giovedì 03 settembre 2009 hai scritto: Simone Lazzaris wrote: Hi everybody; we are using vpopmail in a deployment which involves many programs. We use maildrop for some (but not all) deliveries, dovecot for imap access, some custom perl script to forward some mail. Log story short, we cannot use (at the moment) vusaged to track the mail usage. Why not? Can you be a little more detailed? If you're using vpopmail to manage users, it should know where their mail is stored. Mmmm. Maybe I've not understood what really vusaged does. Our setup is spreaded on many servers (think 20), with the mail stored on an NFS share (NetApp). Right now the various tool all use the maildirsize file (Maildir++ I think it's called) to track the usage, updating this as they put/fetch the email. Is vusaged supposed to work in a similar setup ? I'd have to integrate it with maildrop, dovecot and a couple of perl scripts. I've looked the code and found that there were no option to disable the usage. Turn off their quota and the vusage daemon shouldn't be looked at. If that isn't what's happening, then that is the bug. No, I want to use the quota, but with the old method, looking at the maildirsize file. That's missing (if I've understood the code). I will also be nice if (adding a return 0; at line 74, and a couple of curly braces around that) vdelivermail wouldn't segfault if the config file is missing or unparsable. Now, at version 5.4.28, if vusagec.conf is missing, the code try to proceed and segfault. Fixed in trunk. Thanks for the report! Thanks for the attention -- Simone Lazzaris INTERACTIVE NETWORK SRL Via Roggia Vignola 9, 24047 Treviglio (BG) tel : +39 0363.302820 fax : +39 0363.304352 web : http://www.interactive.eu email : s.lazza...@interactive.eu signature.asc Description: This is a digitally signed message part. !DSPAM:4a9fdcb432718419052476!
Re: [vchkpw] Patch to disable vusaged
In data giovedì 03 settembre 2009 hai scritto: Simone Lazzaris wrote: Our setup is spreaded on many servers (think 20), with the mail stored on an NFS share (NetApp). The vusage daemon is written with this in mind, though it's more efficient to have it run on the device providing storage so that it isn't doing disk polling over a network connection. Ok, I can undestand that. The vusage daemon accepts connections from an allowed list of IPs for usage queries so that it can be used in a cluster efficiently. Right now the various tool all use the maildirsize file (Maildir++ I think it's called) to track the usage, updating this as they put/fetch the email. Correct. vusaged supports Maildir++, and at this time, ignores maildirsize because it's redundant, and inefficient means of calculating storage. Later, vusaged will be updated to re-write maildirsize. It's currently set to be in addition to existing quota monitoring systems, with a greater efficiency, as to deprecate other quota configuration systems, but it should not interfere or cause number variances. Is vusaged supposed to work in a similar setup ? I'd have to integrate it with maildrop, dovecot and a couple of perl scripts. That depends upon a great many things, such as, what is checking quotas, and when. In general, if the daemon is running, and it does not have to be, both Maildir++ quotas, and vpopmail's vusage style of quota checking should work fine at the same time. If vusaged is not running, Maildir++ quotas should continue to work. Ok, but how can be syncronized the two vision of the quota, if only vpopmail uses vusaged ? I think that there can be only two cases 1) all tools use vusaged or 2) all tools use traditional Maildir++ quota. In any other combination, the two vision of the real maildir quota will go quickly out of sync. I've looked the code and found that there were no option to disable the usage. Turn off their quota and the vusage daemon shouldn't be looked at. If that isn't what's happening, then that is the bug. No, I want to use the quota, but with the old method, looking at the maildirsize file. That's missing (if I've understood the code). In 5.4.28, if the vusage daemon is not running, traditional Maildir++ quota checking is done. Yes, and this works, but it generate an error message each time the daemon is searched for. For a normal deliver, that means at least 3 error messages on the log file. And 3 attempts to open the socket. I think it's more efficent, and cleaner, to check if one wants to disable the daemon, adding a line in the config file. My patch just do that. Take a look at one line of my logfile: Sep 3 09:12:25 moss qmail: 1251961945.482023 delivery 35086: success: client_connect:_connect_failed:_2/client_connect:_connect_failed:_2/client_connect:_connect_failed:_2/client_connect:_connect_failed:_2/did_0+0+1/ Is only to avoid the repeated client_connect:_connect_failed string, and to retrieve some CPU cycle, that I've wrote the patch. -- Simone Lazzaris INTERACTIVE NETWORK SRL Via Roggia Vignola 9, 24047 Treviglio (BG) tel : +39 0363.302820 fax : +39 0363.304352 web : http://www.interactive.eu email : s.lazza...@interactive.eu signature.asc Description: This is a digitally signed message part. !DSPAM:4a9fe51f32711436111299!
