750207 - pop3 on linux checks quota incorrectly, resulting in wrong "Insufficient disk space" messages

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Michael Tokarev]

Attachment: proposed code patch that fixes this ussue

User Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:10.0.3) Gecko/20100101 Firefox/10.0.3 Iceweasel/10.0.3
Build ID: 20120330191503

Steps to reproduce:

On linux, enabled filesystem quota mechanism for the filesystem where the thunderbird profile resides (/home) but did not actually set any quota limits of the account in question.  Also created a POP3 account in thunderbird.


Actual results:

When TB tries to fetch mail over POP3, it displays the following message:

 There is not enough disk space to download new messages. Try deleting old mail, emptying the Trash folder, and compacting your mail folders, and then try again.

and does not download any messages from the mailbox.


Expected results:

It should download the messages and stop displaying false and misleading message.

The problem is that in nsLocalFileUnix.cpp:nsLocalFile::GetDiskSpaceAvailable(), there's a call to quotactl() which succeds if quota is enabled for the filesystem in question.  But if no quota is set for the user, corresponding limit will be 0, and the amount of bytes in the remote mailbox gets compared with this 0 and TB decides since there's zero available bytes, the messages can't be stored.

But limit == 0 actually means there's no limit at all.

There's a few more problems with the current code.

quotactl() return a structure with one field (dqb_valid) being a bitmask indicating which other fields (actual limits) are valid.  It is an error to access, say, qb_bhardlimit without checking dqb_valid&QIF_BLIMITS first.  It works just because current - at least linux - kernel code always sets this flag, but it does not have to.

This bug is a very old bug, introduced somewhere in 3.x version or even before, and is present up till current version.

Original discussion on mozillazine forum: http://forums.mozillazine.org/viewtopic.php?t=2456579

Comment 1

[Magnus Melin [:mkmelin]]

Are you willing to create a patch to fix that?

Comment 2

[Michael Tokarev]

I'm sorry but.. what?  I attached the patch fixing the bug to my bugreport...

Comment 3

[Magnus Melin [:mkmelin]]

Duh! Sorry about that. Please ask [e-mail] to review your patch.
https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed

Comment 4

[Benjamin Smedberg]

Comment on attachment 619518 [details] [diff] [review]
proposed code patch that fixes this ussue

stransky, can you check this? MichaelT, I suppose there really isn't a way to write an automated (xpcshell) test for this functionality?

Comment 5

[Michael Tokarev]

It is indeed difficult to write a test for this, since dealing with quotas usually requires root privileges (or else it defeats the purpose), and quota should be enabled by appropriate filesystem mount options and initial current quota collection run.  This case (quotas enabled but not set for a given user) may be checked in an automated script, and a script can either run (if the condition is met) or skipped (with the appropriate message saying that in order to run this script, you have to enable but not set quotas), but I highly doubt lots of environments will do that in order to run this test, so the whole thing wont be very useful.  Just IMHO anyway.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 619518 [details] [diff] [review]
proposed code patch that fixes this ussue

According to quota implementation in samba (which looks robust enough) the quota is not set when both dqb_bsoftlimit and dqb_bhardlimit are zero. (see http://svn.dd-wrt.com:8000/browser/src/router/samba36/source3/smbd/quotas.c?rev=18451 for instance, the quotas.c file).

The QIF_BLIMITS check is not necessary (according to the man page all flags are set by kernel) but it may not hurt us. But I don't see the check anywhere it in the samba quota implementation.

So please add the dqb_bsoftlimit and dqb_bhardlimit zero check at least.

Comment 7

[Michael Tokarev]

This is, well, not right understanding.

Either or both or none of softlimit and hardlimit can be set to 0 or set to something other than 0.  Setting hard limit to 0 but soft limit to non-0 means that softlimit will become hard after a grace period.  Setting soft limit to 0 but hard to non-0 means there's no soft limit, ony hard limit.

Samba tries to check both soft and hard limit.  In particular, it declares that user does not have any space if he exceeded soft limit _only_.  But softlimit is just that - soft, it is okay to exceed soft limit temporarily (up to a grace period in time and up to actual hard limit in size).  But note that samba here actually returns available disk space, not tries to undestand if the user has enough disk space to store some file -- which might be right thing to do.  Samba will actually try to write data or create files if client asked it to do so, and will return whatever actual _kernel_ error it will hit, if any.  The disk space is just informational thing for the user here, nothing more.

Mozilla on the other hand checks if the user has enough space to store mailbox data.  Initially it has been decided that only hard quota is interesting, and I understand why: dealing with soft quotas and grace periods is sort of insane, since the logic is not really trivial and the kernel may implement it slightly differently anyway.  And I can easily imagine users complaining when mozilla refuses to d/load messages saying about not enough disk space when the user exceeded only soft quota.

So I think checking for hard limit only (or not checking at all) is the way to do it in Mozilla.

Thanks,

/mjt

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Yes, you're right about the soft quota. My concern is just how to detect that the quota is not set, because dqb_bhardlimit = 0 can mean that there isn't any available block, right? Or do you mean that dqb_bhardlimit can't be set to 0 unless the quota is unset? And if so, is there any documentation about it?

Comment 9

[Michael Tokarev]

Each {soft,hard}{block,inode} limit is independent of all other.  If one is 0 it means this one is not set, and it does not mean anything at all about others.  That's what I mean about the "not right understanding", and this is what I described in the second paragraph above (in comment 7)

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Okay. But please provide an answer to my question why do you think the the dqb_bhardlimit != 0 check is enough when samba uses the dqb_bhardlimit != 0 && dqb_bsoftlimit != 0 check. Which makes sense to me, because if bsoftlimit is zero it can't became hardlimit after the grace period, right?)

Comment 11

[Michael Tokarev]

Here's the samba code (simplified a bit):

   if ((D.softlimit && D.curblocks >= D.softlimit) ||
       (D.hardlimit && D.curblocks >= D.hardlimit)) {
                *dfree = 0;
                *dsize = D.curblocks;
   } else if (D.softlimit==0 && D.hardlimit==0) {
                return(False);
   } else {
                if (D.softlimit == 0)
                       D.softlimit = D.hardlimit;
                *dfree = D.softlimit - D.curblocks;
                *dsize = D.softlimit;
  }


Translation: if either hard or soft limit is set and exceeded, we set disk free space to 0.  Else, if neither soft nor hard limit is set, we pretend we don't know what the disk free space is.  Else, which means at least one of the limits is set and neither is exceeded, we choose whatever is set and use it as the disk size.

It does not ensure that both is set, it merely uses whatever is set.

And as I already said in comment 7, it is perfectly legal to have softlimit != 0 and hardlimit = 0, there's nothing wrong with that.  It means that the user in question may use any available disk space but up to a grace period in time, after which he will have to free space to be able to write again.

Comment 12

[Martin Stránský [:stransky] (ni? me)]

I see. It looks like the whole implementation of the quota in mozilla is broken, because we use:

PRInt64 QuotaSpaceAvailable = PRInt64(fs_buf.f_bsize * dq.dqb_bhardlimit);

which actually should be:

PRInt64 QuotaSpaceAvailable = PRInt64(fs_buf.f_bsize * (dq.dqb_bhardlimit - dq.dqb_curspace));

and then it makes sense the dqb_bhardlimit zero check.

Comment 13

[Michael Tokarev]

It looks like you're right, I overlooked this place apparently.

But it shouldn't do it this way, since both curspace and hardlimit are unsigned, and limit might be less than curspace.  So it should actually be:

 PRInt64 QuotaSpaceAvailable =  dq.dqb_bhardlimit > dq.dqb_curspace ? PRInt64(fs_buf.f_bsize * (dq.dqb_bhardlimit - dq.dqb_curspace)) : 0;

(or something in that theme).

But dq.dqb_bhardlimit != 0 check should be in the if statement anyway.

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Yeah, please update your patch with those fixes.

Comment 15

[Michael Tokarev]

Attachment: another version of the proposed fix

I'm attaching a new version of the patch, which also corrects the other issue (taking into account already used blocks).  But this time it is not even compile-tested, since right now I don't a machine where I can try to compile it.

Comment 16

[Michael Tokarev]

btw, can we set status of this bug already, to something more appropriate than "unconfirmed" ? :)

Comment 17

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 622677 [details] [diff] [review]
another version of the proposed fix

Looks okay, Thanks! Just some indentation nits:

+       && dq.dqb_bhardlimit) {

Please place the bracket on a newline. 

+        PRInt64 QuotaSpaceAvailable =
+            dq.dqb_bhardlimit < dq.dqb_curspace ? 0
+            : PRInt64(fs_buf.f_bsize * (dq.dqb_curspace - dq.dqb_bhardlimit));

Something like:

PRInt64 QuotaSpaceAvailable = 0;
if (dq.dqb_curspace > dq.dqb_bhardlimit)
    QuotaSpaceAvailable = PRInt64(fs_buf.f_bsize * (dq.dqb_curspace - dq.dqb_bhardlimit));

would look better here. r+ with those changes.

Comment 18

[Michael Tokarev]

Attachment: v3 patch

Fixed the identation as suggested.  Also fixed the inversed substraction (hardlimit - curspace is right, curspace - hardlimit is wrong).

Comment 19

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 622677 [details] [diff] [review]
another version of the proposed fix

Uhh, The patch is still wrong. Because dqb_curspace means "used blocks" and when quota is on, it's not bigger than dqb_bhardlimit (at least we don't care when it is).

So it has to be:

if (dq.dqb_bhardlimit > dq.dqb_curspace)
    QuotaSpaceAvailable = PRInt64(fs_buf.f_bsize * (dq.dqb_bhardlimit - dq.dqb_curspace));

Sorry for the confusion here.

Comment 20

[Michael Tokarev]

Heh. I was faster and found it before you :)

Comment 21

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 622703 [details] [diff] [review]
v3 patch

Yes, looks okay now.

Comment 22

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 622703 [details] [diff] [review]
v3 patch

Benjamin, can you confirm it?

Comment 23

[Benjamin Smedberg]

Comment on attachment 622703 [details] [diff] [review]
v3 patch

Please take a look at the normal style of commit messages in mozilla-central and make this commit message match, in particular: list the bug number and basic patch description in the first line along with the marking "r=stransky/bsmedberg". The full change description on subsequent lines as you have it is fine.

Comment 24

[Michael Tokarev]

do you want another attachment from me?  Come on guys, I just wanted to fix a bug but no matter how I asked no one knew.  So I found the problem, and provided a patch in a hope it will be easier this way -- for a code/project I had _never_ looked before.  To me the problem is solved, I just changed my quota setup to match the buggy mozilla behavour.  Next time (if I'll ever have a next time) I'll try to not do anything besides just submitting a bugreport.  Or wont even do that - after finding where the problem is and working around it locally.

Comment 25

[Benjamin Smedberg]

Michael, I want to thank you for the report and the patch. I figured that you were happy providing followup patches, since you presumably already have them in a convenient form. If you'd like somebody else to finish this off, please just let us know! Typically all you'd have to do from here is provide a patch in the form that the "checkin-needed" volunteers can push it directly to our tree and we're done.

Comment 26

[Martin Stránský [:stransky] (ni? me)]

Attachment: a patch for check-in

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b0c651c5559

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9b0c651c5559