Closed Bug 450359 Opened 16 years ago Closed 16 years ago

nsFileSpec::Truncate() expects PRInt32 as the first argument so mail may disappear.

Categories

(Core :: XPCOM, defect)

1.8 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: hiro, Assigned: hiro)

References

Details

(Keywords: dataloss, fixed1.8.1.18)

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.15) Gecko/20080702 Ubuntu/8.04 (hardy) Firefox/2.0.0.15 Kazehakase/0.5.4
Build Identifier: 

Thunderbird can handle mailbox below 4GB, but nsFileSpec::Truncate() expects PRInt32 as the first argument so some mails may disappear if the mail box size is over 2GB.



Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Version: unspecified → 2.0
Component: General → XPCOM
Product: Thunderbird → Core
Version: 2.0 → 1.8 Branch
And Tell() should be expected PRUint32 too.

Index: xpcom/obsolete/nsFileSpecImpl.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpcom/obsolete/nsFileSpecImpl.cpp,v
retrieving revision 1.7
diff -d -u -p -u -8 -p -r1.7 nsFileSpecImpl.cpp
--- xpcom/obsolete/nsFileSpecImpl.cpp   29 Jun 2005 22:23:55 -0000      1.7
+++ xpcom/obsolete/nsFileSpecImpl.cpp   13 Aug 2008 06:22:22 -0000
@@ -731,17 +731,17 @@ NS_IMETHODIMP nsFileSpecImpl::Seek(PRInt
                nsInputFileStream is(mInputStream);
                is.seek(offset);
                result = is.error();
        }
        return result;
 }

 //----------------------------------------------------------------------------------------
-NS_IMETHODIMP nsFileSpecImpl::Tell(PRInt32 *_retval)
+NS_IMETHODIMP nsFileSpecImpl::Tell(PRUint32 *_retval)
 //----------------------------------------------------------------------------------------
 {
        TEST_OUT_PTR(_retval)
        if (!mInputStream)
                return NS_ERROR_NULL_POINTER;
        nsInputFileStream s(mInputStream);
        *_retval = s.tell();
        return s.error();
Index: xpcom/obsolete/nsIFileSpec.idl
===================================================================
RCS file: /cvsroot/mozilla/xpcom/obsolete/nsIFileSpec.idl,v
retrieving revision 1.7
diff -d -u -p -u -8 -p -r1.7 nsIFileSpec.idl
--- xpcom/obsolete/nsIFileSpec.idl      29 Jun 2005 22:23:55 -0000      1.7
+++ xpcom/obsolete/nsIFileSpec.idl      13 Aug 2008 06:22:22 -0000
@@ -150,17 +150,17 @@ interface nsIFileSpec : nsISupports
     /** Check eof() before each call.
      * CAUTION: false result only indicates line was truncated
      * to fit buffer, or an error occurred (OTHER THAN eof).
      */
        long write(in string data, in long requestedCount);
        void flush();

        void seek(in long offset);
-       long tell();
+       unsigned long tell();
        void endLine();
        attribute AString unicodePath;

 };

 // Define Contractid and CID
 %{C++
 // {a3020981-2018-11d3-915f-a957795b7ebc}
QA Contact: general → xpcom
Set severity to critical because of user data loss.
Severity: normal → critical
David, could you please take a look at the patch and nsParseMailbox.cpp in #2.

I think there is a risk of data loss.
Attached patch Patch for Tell() (obsolete) — Splinter Review
Attachment #333674 - Flags: review?(bienvenu)
Keywords: dataloss
Thx for the patch. We're not using nsIFileSpec on the trunk at all, so I don't think that part is needed...this patch is for the trunk, right?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #7)
> Thx for the patch. We're not using nsIFileSpec on the trunk at all, so I don't
> think that part is needed...this patch is for the trunk, right?

No, both patches for MOZILLA_1_8_BRANCH.


I guess the person commented in https://bugzilla.mozilla.org/show_bug.cgi?id=321371#c69 faced to this issue.
I confirmed breakage of mail folder.

Steps to reproduce

1. Create over 2GB Inbox. (I create it using dd command)
2. Disallow anti-virus clients to quarantine option.
3. Add a filter rule that subject contains test.
4. Send a mail with "test" subject from other mail client.
5. Receive the test mail by Thunderbird.

Result:

Thunderbird seemed to be giving up a process to filter the mail. I have no idea what was happened in Thunderbird but CPU usage of Thunderbird was 0 %.

Then I killed Thunderbird and restarted it, mails in the folder which is the target of the filtering were almost broken.

With patches in https://bugzilla.mozilla.org/show_bug.cgi?id=450698 and this bug, Thunderbird can receive test mail correctly.

Attachment #333512 - Flags: review?(shaver)
Comment on attachment 333674 [details] [diff] [review]
Patch for Tell()

The patch raises stability and robustness.
Attachment #333674 - Flags: approval1.8.1.17?
Comment on attachment 333512 [details] [diff] [review]
[checked in on branch] Patch for stable branch.

The patch raises stability and robustness.
Attachment #333512 - Flags: approval1.8.1.17?
Attached patch Revised patch for Tell(). (obsolete) — Splinter Review
I am sorry, my previous patch has typo.

Fix PRUInt32 -> PRUint32.
Attachment #333674 - Attachment is obsolete: true
Attachment #334209 - Flags: superreview?(shaver)
Attachment #334209 - Flags: review?(bienvenu)
Attachment #334209 - Flags: approval1.8.1.17?
Attachment #333674 - Flags: review?(bienvenu)
Attachment #333674 - Flags: approval1.8.1.17?
Setting dependency of Bug 321371, according to Comment #9.
Comment on attachment 334209 [details] [diff] [review]
Revised patch for Tell().

this looks OK, but I think there are other callers of tell() that should be changed as well...
Attachment #334209 - Flags: review?(bienvenu) → review+
Attachment #334209 - Attachment is obsolete: true
Attachment #334222 - Flags: superreview?(shaver)
Attachment #334222 - Flags: review+
Attachment #334222 - Flags: approval1.8.1.17?
Attachment #334209 - Flags: superreview?(shaver)
Attachment #334209 - Flags: approval1.8.1.17?
(In reply to comment #17)
> Created an attachment (id=334222) [details]
> Revided patch following comment #16

Oops! I meant "Revised" of course.

Removed dependency of Bug 321371, because main issue of Bug 321371 was different.
(Only several comments refer to "greater than 2GB" in that bug. They seem to be problem of Bug 449741, instead of problem of Bug 321371.)
(Comment to avoid misleading/confusion, for ease of track)
Problem reported by Comment #10 is Bug 449741.
Comment on attachment 333512 [details] [diff] [review]
[checked in on branch] Patch for stable branch.

Please get all reviews before requesting check-in approval
Attachment #333512 - Flags: approval1.8.1.17?
Attachment #334222 - Flags: approval1.8.1.17?
Comment on attachment 334222 [details] [diff] [review]
[checked in on 1.8] Revided patch following comment #16

Shaver's probably not too excited about reviewing this :-)
Attachment #334222 - Flags: superreview?(shaver) → superreview?(neil)
Aw, man, I was writing an aria about my feelings for nsFileSpec!

Thanks, though. :)
Attachment #334222 - Flags: superreview?(neil) → superreview+
Comment on attachment 333512 [details] [diff] [review]
[checked in on branch] Patch for stable branch.

Neil, could you please review the patch  for Truncate()?
Attachment #333512 - Flags: superreview?(neil)
Attachment #333512 - Flags: review?(shaver)
Attachment #333512 - Flags: review?(neil)
Attachment #334222 - Flags: approval1.8.1.17?
Attachment #333512 - Flags: superreview?(neil)
Attachment #333512 - Flags: superreview+
Attachment #333512 - Flags: review?(neil)
Attachment #333512 - Flags: review?(bienvenu)
Comment on attachment 333512 [details] [diff] [review]
[checked in on branch] Patch for stable branch.

thx for the patch
Attachment #333512 - Flags: review?(bienvenu) → review+
Attachment #333512 - Flags: approval1.8.1.17?
Comment on attachment 333512 [details] [diff] [review]
[checked in on branch] Patch for stable branch.

Both patches approved for 1.8.1.17, a=dveditz for release-drivers.
Attachment #333512 - Flags: approval1.8.1.17? → approval1.8.1.17+
Attachment #334222 - Flags: approval1.8.1.17? → approval1.8.1.17+
Assignee: nobody → poincare
Keywords: checkin-needed
Whiteboard: [checkin-needed: 1.8 branch]
Attachment #333512 - Flags: approval1.8.1.18?
Attachment #333512 - Flags: approval1.8.1.17-
Attachment #333512 - Flags: approval1.8.1.17+
Comment on attachment 334222 [details] [diff] [review]
[checked in on 1.8] Revided patch following comment #16

This will have to get looked at in 1.8.1.18. We're frozen.
Attachment #334222 - Flags: approval1.8.1.18?
Attachment #334222 - Flags: approval1.8.1.17-
Attachment #334222 - Flags: approval1.8.1.17+
Keywords: checkin-needed
Whiteboard: [checkin-needed: 1.8 branch]
Comment on attachment 333512 [details] [diff] [review]
[checked in on branch] Patch for stable branch.

Approved for 1.8.1.18, a=dveditz for release-drivers
Attachment #333512 - Flags: approval1.8.1.18? → approval1.8.1.18+
Comment on attachment 334222 [details] [diff] [review]
[checked in on 1.8] Revided patch following comment #16

Approved for 1.8.1.18, a=dveditz for release-drivers
Attachment #334222 - Flags: approval1.8.1.18? → approval1.8.1.18+
None of these patches are for the trunk, correct?
correct, we're not using nsFileSpec on the trunk
(In reply to comment #31)
> correct, we're not using nsFileSpec on the trunk

If it's not being used, can it be removed?
(In reply to comment #32)
> If it's not being used, can it be removed?

Bug 38122 ;-)
Status: NEW → ASSIGNED
Hardware: PC → All
Whiteboard: [c-n: 1.8.1 branch only, 2 patches]
Comment on attachment 333512 [details] [diff] [review]
[checked in on branch] Patch for stable branch.

Checking in xpcom/obsolete/nsFileSpec.h;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpec.h,v  <--  nsFileSpec.h
new revision: 1.11.4.1; previous revision: 1.11
done
Checking in xpcom/obsolete/nsFileSpecBeOS.cpp;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpecBeOS.cpp,v  <--  nsFileSpecBeOS.cpp
new revision: 1.3.28.1; previous revision: 1.3
done
Checking in xpcom/obsolete/nsFileSpecImpl.cpp;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpecImpl.cpp,v  <--  nsFileSpecImpl.cpp
new revision: 1.7.4.1; previous revision: 1.7
done
Checking in xpcom/obsolete/nsFileSpecMac.cpp;
/cvsroot/mozilla/xpcom/obsolete/Attic/nsFileSpecMac.cpp,v  <--  nsFileSpecMac.cpp
new revision: 1.3.28.1; previous revision: 1.3
done
Checking in xpcom/obsolete/nsFileSpecOS2.cpp;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpecOS2.cpp,v  <--  nsFileSpecOS2.cpp
new revision: 1.3.28.2; previous revision: 1.3.28.1
done
Checking in xpcom/obsolete/nsFileSpecUnix.cpp;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpecUnix.cpp,v  <--  nsFileSpecUnix.cpp
new revision: 1.9.8.2; previous revision: 1.9.8.1
done
Checking in xpcom/obsolete/nsFileSpecWin.cpp;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpecWin.cpp,v  <--  nsFileSpecWin.cpp
new revision: 1.10.12.3; previous revision: 1.10.12.2
done
Checking in xpcom/obsolete/nsIFileSpec.idl;
/cvsroot/mozilla/xpcom/obsolete/nsIFileSpec.idl,v  <--  nsIFileSpec.idl
new revision: 1.7.4.1; previous revision: 1.7
done
Attachment #333512 - Attachment description: Patch for stable branch. → [checked in on branch] Patch for stable branch.
Comment on attachment 334222 [details] [diff] [review]
[checked in on 1.8] Revided patch following comment #16

Checking in mailnews/addrbook/src/nsVCard.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsVCard.cpp,v  <--  nsVCard.cpp
new revision: 1.4.4.5; previous revision: 1.4.4.4
done
Checking in mailnews/db/msgdb/src/nsMailDatabase.cpp;
/cvsroot/mozilla/mailnews/db/msgdb/src/nsMailDatabase.cpp,v  <--  nsMailDatabase.cpp
new revision: 1.118.8.5; previous revision: 1.118.8.4
done
Checking in mailnews/import/src/ImportOutFile.cpp;
/cvsroot/mozilla/mailnews/import/src/ImportOutFile.cpp,v  <--  ImportOutFile.cpp
new revision: 1.6.28.1; previous revision: 1.6
done
Checking in mailnews/import/text/src/nsTextAddress.cpp;
/cvsroot/mozilla/mailnews/import/text/src/nsTextAddress.cpp,v  <--  nsTextAddress.cpp
new revision: 1.47.2.1; previous revision: 1.47
done
Checking in mailnews/local/src/nsLocalMailFolder.cpp;
/cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.cpp,v  <--  nsLocalMailFolder.cpp
new revision: 1.506.2.27; previous revision: 1.506.2.26
done
Checking in mailnews/local/src/nsParseMailbox.cpp;
/cvsroot/mozilla/mailnews/local/src/nsParseMailbox.cpp,v  <--  nsParseMailbox.cpp
new revision: 1.258.2.20; previous revision: 1.258.2.19
done
Checking in mailnews/local/src/nsPop3Sink.h;
/cvsroot/mozilla/mailnews/local/src/nsPop3Sink.h,v  <--  nsPop3Sink.h
new revision: 1.21.10.1; previous revision: 1.21
done
Checking in xpcom/obsolete/nsFileSpecImpl.cpp;
/cvsroot/mozilla/xpcom/obsolete/nsFileSpecImpl.cpp,v  <--  nsFileSpecImpl.cpp
new revision: 1.7.4.2; previous revision: 1.7.4.1
done
Checking in xpcom/obsolete/nsIFileSpec.idl;
/cvsroot/mozilla/xpcom/obsolete/nsIFileSpec.idl,v  <--  nsIFileSpec.idl
new revision: 1.7.4.2; previous revision: 1.7.4.1
done
Attachment #334222 - Attachment description: Revided patch following comment #16 → [checked in on 1.8] Revided patch following comment #16
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [c-n: 1.8.1 branch only, 2 patches]
Target Milestone: --- → mozilla1.9.1b2
Depends on: 494706
You need to log in before you can comment on or make changes to this bug.