The default bug view has changed. See this FAQ.

nsILocalFileUnix affected by 32bit stat/statvfs/truncate, therefore does not work with large files

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
XPCOM
--
major
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: nmaier, Assigned: nmaier)

Tracking

({fixed1.8.1.24})

Trunk
mozilla1.9.2a1
x86
Linux
fixed1.8.1.24
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(status1.9.1 .3-fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

10 years ago
The nsILocalFileUnix implementation issues regular 32bit stat calls in most functions.
stat will however fail for EOVERFLOW for large files that have sizes beyond the 32bit border. Therefore most of the those functions will fail, e.g. remove, isWriteable, non-atomic moveTo.

This is particularly bad IMO as nsIFileStreams will allow to write such large files, but there is not way to later delete them, as nsIFile::remove will fail.
(Assignee)

Comment 1

10 years ago
Created attachment 273245 [details] [diff] [review]
v1, convert stat, lstar, statvs, truncate to 64bit where available

Similar to what is done with statfs/statvfs already I introduced some STAT/LSTAT macros that will do most of the work.
Other stuff is "branched" by the HAVE_* macros.
Attachment #273245 - Flags: review?(dougt)
(Assignee)

Updated

10 years ago
Depends on: 389089
Comment on attachment 273245 [details] [diff] [review]
v1, convert stat, lstar, statvs, truncate to 64bit where available

Try bsmedberg...
Attachment #273245 - Flags: review?(dougt) → review?(benjamin)

Comment 3

9 years ago
Comment on attachment 273245 [details] [diff] [review]
v1, convert stat, lstar, statvs, truncate to 64bit where available

I worry about using a name like STAT might conflict with something else in the global namespace.

(e.g., TRUE/FALSE which IIRC get stepped on by X)
Attachment #273245 - Flags: superreview?(cbiesinger)
Attachment #273245 - Flags: review?(dougt)
Attachment #273245 - Flags: review?(benjamin)
(Assignee)

Comment 4

9 years ago
(In reply to comment #3)
> (From update of attachment 273245 [details] [diff] [review])
> I worry about using a name like STAT might conflict with something else in the
> global namespace.
> 
> (e.g., TRUE/FALSE which IIRC get stepped on by X)
> 

At least I did not run into any issues when I did a build prior to posting the patch...

Comment 5

9 years ago
Comment on attachment 273245 [details] [diff] [review]
v1, convert stat, lstar, statvs, truncate to 64bit where available

in SetFileSize(PRInt64), if we do not have truncate64, don't we do the wrong thing?  Shouldn't we error out instead of casting off the upper 32bits?

What do you think of having all of the *64 functions should fail similar to |GetDiskSpaceAvailable| when the 64 bit version of stat isn't available?

why, in GetNativeTarget, did you change this:

-    PRInt32 size;
-    LL_L2I(size, targetSize64);
+    PRInt32 size = (PRInt32)targetSize64;
(Assignee)

Comment 6

9 years ago
(In reply to comment #5)
> (From update of attachment 273245 [details] [diff] [review])
> in SetFileSize(PRInt64), if we do not have truncate64, don't we do the wrong
> thing?  Shouldn't we error out instead of casting off the upper 32bits?
hmm. right. Something precondition like this should do:
if (aFileSize > 0xffffffff) error;
(I didn't think much about this to be honest, just left it like it was before.)

> What do you think of having all of the *64 functions should fail similar to
> |GetDiskSpaceAvailable| when the 64 bit version of stat isn't available?
Huh?
It should be possible to stat files < 4Gb on such systems, don't you think?
I think it is safe to assume that systems not implementing the *64-API in general do not support 64-bit sized files, so no problem here...

> 
> why, in GetNativeTarget, did you change this:
> 
> -    PRInt32 size;
> -    LL_L2I(size, targetSize64);
> +    PRInt32 size = (PRInt32)targetSize64;
> 
The LL_-API is obsolete/deprecated for what I know. This one is better readable IMO.
Whiteboard: DUPEME
Attachment #273245 - Flags: review?(dougt) → review?(benjamin)
Comment on attachment 273245 [details] [diff] [review]
v1, convert stat, lstar, statvs, truncate to 64bit where available

It would be nice to unit-test this... is there a way to do that without creating a 4G file during unit-testing?
Attachment #273245 - Flags: review?(benjamin) → review+
Flags: in-testsuite?
Version: unspecified → Trunk
Comment on attachment 273245 [details] [diff] [review]
v1, convert stat, lstar, statvs, truncate to 64bit where available

dougt, can you sr this, as biesi is way too busy.
Attachment #273245 - Flags: superreview?(cbiesinger) → superreview?(doug.turner)

Comment 9

9 years ago
presumably we could write an ldpreload which changed what stat and friends did for a limited set of paths. kinda heavy handed, but it should work.
Doug - any chance on that sr?  I've got people pinging me about this...
Comment on attachment 273245 [details] [diff] [review]
v1, convert stat, lstar, statvs, truncate to 64bit where available

no need for a sr here.  benjamin is good enough.
Attachment #273245 - Flags: superreview?(doug.turner) → superreview+
Keywords: checkin-needed
Comment on attachment 273245 [details] [diff] [review]
v1, convert stat, lstar, statvs, truncate to 64bit where available


Could you update this 1.5 years old cvs patch to a current hg one ? Thanks.
3 out of 11 hunks failed to apply.
Keywords: checkin-needed
Whiteboard: DUPEME
(Assignee)

Comment 14

8 years ago
Created attachment 371475 [details] [diff] [review]
v.1.1, convert stat, lstat, statvs, truncate to 64bit where available, central edition
[Checkin: Comment 15]

Same as the old patch, this time for mozilla-central.
Built and tested again.

Replaced some other code that only partly addressed the 64bit stat API. Seems some other bugs were in need to have part of the issue fixed.
Attachment #273245 - Attachment is obsolete: true
Attachment #371475 - Flags: superreview?(benjamin)
Attachment #371475 - Flags: review?(benjamin)
Attachment #371475 - Flags: superreview?(benjamin)
Attachment #371475 - Flags: superreview+
Attachment #371475 - Flags: review?(benjamin)
Attachment #371475 - Flags: review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Comment on attachment 371475 [details] [diff] [review]
v.1.1, convert stat, lstat, statvs, truncate to 64bit where available, central edition
[Checkin: Comment 15]


http://hg.mozilla.org/mozilla-central/rev/67d9577dcf44
Attachment #371475 - Attachment description: v.1.1, convert stat, lstat, statvs, truncate to 64bit where available, central edition → v.1.1, convert stat, lstat, statvs, truncate to 64bit where available, central edition [Checkin: Comment 15]
(In reply to comment #15)
> (From update of attachment 371475 [details] [diff] [review])
> 
> http://hg.mozilla.org/mozilla-central/rev/67d9577dcf44

Will need 1.9.1 patch:
{
patching file xpcom/io/nsLocalFileUnix.cpp
Hunk #7 FAILED at 1379
Hunk #8 FAILED at 1450
Hunk #9 FAILED at 1497
3 out of 9 hunks FAILED
}
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: wanted1.9.1?
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing: needs patch]
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 17

8 years ago
Created attachment 373164 [details] [diff] [review]
v.1.1, convert stat, lstat, statvs, truncate to 64bit where available, 1.9.1 edition

Same as the central patch.
Built and tested again.

Again replacing only the partly implemented code dealing with 64bit API. Other code left as is.
Attachment #373164 - Flags: superreview?(benjamin)
Attachment #373164 - Flags: review?(benjamin)
Attachment #373164 - Flags: superreview?(benjamin)
Attachment #373164 - Flags: superreview+
Attachment #373164 - Flags: review?(benjamin)
Attachment #373164 - Flags: review+
Flags: wanted1.9.1?
Whiteboard: [needs 1.9.1 landing: needs patch] → [needs 1.9.1.x landing: needs approval]
Attachment #373164 - Flags: approval1.9.1.1?
Attachment #373164 - Flags: approval1.9.1.1? → approval1.9.1.2?

Comment 18

8 years ago
Coming in from bug 387502 which was apparently caused partially
due to the failure to use stat64, lstat64 and friends.

I tried to fix this file, and in doing so created a different patch late June.
As I was made aware of this patch, I noticed a couple of places where the
return value of lstat{,64} (LINK() macro) is not checked.

I also noticed a place where a math expression can be cleaned up, and another
place where a clarifying comment will help.
I am excerpting a relevant part below.

At least, I believe the additions of the checking of LSTAT() values
should improve long term maintenance and reliability of the code.


========================================
NS_IMETHODIMP
nsLocalFile::GetLastModifiedTimeOfLink(PRInt64 *aLastModTimeOfLink)
----------------------------------------
A few calls to LL_* macro can be simplified.

-    LL_I2L(*aLastModTimeOfLink, (PRInt32)sbuf.st_mtime);
 
     // lstat returns st_mtime in seconds

-    PRInt64 msecPerSec;
-    LL_I2L(msecPerSec, PR_MSEC_PER_SEC);
-    LL_MUL(*aLastModTimeOfLink, *aLastModTimeOfLink, msecPerSec);
+
+    *aLastModTimeOfLink = sbuf.st_mtime * PR_MSEC_PER_SEC;


========================================
NS_IMETHODIMP
nsLocalFile::IsSymlink(PRBool *_retval)
----------------------------------------
Missing check of LSTAT() return value.


@@ -1477,8 +1460,12 @@
     CHECK_mPath();
     _retval.Truncate();
 
-    struct stat symStat;
-    lstat(mPath.get(), &symStat);
+    struct STAT symStat;
+
+    // We should check the return value of lstat{,64} calls
+    if( LSTAT(mPath.get(), &symStat) != 0)
+        return NSRESULT_FOR_ERRNO();
+
     if (!S_ISLNK(symStat.st_mode))
         return NS_ERROR_FILE_INVALID_PATH;
 


========================================
NS_IMETHODIMP
nsLocalFile::GetNativeTarget(nsACString &_retval)
----------------------------------------
A check for the return value of lstat/lstat64 was missing, and
a couple of clarifying comments were added.


@@ -1486,8 +1473,13 @@
     if (NS_FAILED(GetFileSizeOfLink(&targetSize64)))
         return NS_ERROR_FAILURE;
 
-    PRInt32 size;
-    LL_L2I(size, targetSize64);
+    // Clarifying comment: The following code assumes that
+    // the  in targetSize64 doesn't exceed 2^31.
+    // That is, the path is not longer than 2^31. I think it is a safe 
+    // assumption, but should be commented for maintenance purposes.
+
+    PRInt32 size = (PRInt32)targetSize64;
+
     char *target = (char *)nsMemory::Alloc(size + 1);
     if (!target)
         return NS_ERROR_OUT_OF_MEMORY;
@@ -1536,7 +1528,7 @@
         PRInt32 len = strlen(target);
         while (target[len-1] == '/' && len > 1)
             target[--len] = '\0';
-        if (lstat(flatRetval.get(), &symStat) < 0) {
+        if (LSTAT(flatRetval.get(), &symStat) < 0) {
             rv = NSRESULT_FOR_ERRNO();
             break;
         }
@@ -1544,7 +1536,7 @@
             rv = NS_ERROR_FILE_INVALID_PATH;
             break;
         }
-        size = symStat.st_size;
+        size = (PRInt32)symStat.st_size; // Assumption of 32bit size.
         if (readlink(flatRetval.get(), target, size) < 0) {
             rv = NSRESULT_FOR_ERRNO();
             break;

Comment 19

8 years ago
Oops.
In comment 18,
The missing check of LSTAT() listed below
nsLocalFile::IsSymlink(PRBool *_retval)
should be part of the required change for GetNAtiveTarget() [that follows].

The missing  checking of LSTAT() value in  
nsLocalFile::IsSymlink(PRBool *_retval) is as follows.

@@ -1407,9 +1387,12 @@
     NS_ENSURE_ARG_POINTER(_retval);
     CHECK_mPath();
 

-    struct stat symStat;
-    lstat(mPath.get(), &symStat);
-    *_retval=S_ISLNK(symStat.st_mode);
+    struct STAT symStat;
+    // We should check the return code of lstat64/lstat
+    if ( LSTAT(mPath.get(), &symStat) != 0)
+        return NSRESULT_FOR_ERRNO();
+
+    *_retval = S_ISLNK(symStat.st_mode);
     return NS_OK;
 }
It'd be much more useful if you filed a new bug and created a patch:
https://developer.mozilla.org/en/Creating_a_patch

Updated

8 years ago
Whiteboard: [needs 1.9.1.x landing: needs approval] → [needs 1.9.1.x landing: needs approval][tb3needs]

Comment 21

8 years ago
Thank you for comment #20.

Additional fix for nsLocalFileUnix.cpp has been re-filed as
bug 505517 "nsLocalFileUnix.cpp fails to check the return value of lstat{,64} in a couple of places.", so that will be the place where nsLocalFileUnix.cpp patch
will be discussed in the future, hopefully.

(The original bug which prompted me to investigate nsLocalFileUnix.cpp
will fixed with additional patch to 
nsLocalMailFolder.cpp, and that will  continue to be discussed in bug 387502)
 
[PS: somehow "clone this bug" didn't work as I had expected and so a few
entries are messed up in the new bug entry :-( ]
Attachment #373164 - Flags: approval1.9.1.2? → approval1.9.1.2-
Comment on attachment 373164 [details] [diff] [review]
v.1.1, convert stat, lstat, statvs, truncate to 64bit where available, 1.9.1 edition

I don't see why this is needed for 1.9.1 given that it's been a bug for just over two years. Please renominate with rationale for why this is now needed.
Comment on attachment 373164 [details] [diff] [review]
v.1.1, convert stat, lstat, statvs, truncate to 64bit where available, 1.9.1 edition

I believe we should take this for 1.9.1 for Thunderbird especially because it's a leading cause of dataloss for large mailboxes. It's quite low-risk and has been baking on trunk for a long time.
Attachment #373164 - Flags: approval1.9.1.2- → approval1.9.1.2?
Attachment #373164 - Flags: approval1.9.1.2? → approval1.9.1.3?
Comment on attachment 373164 [details] [diff] [review]
v.1.1, convert stat, lstat, statvs, truncate to 64bit where available, 1.9.1 edition

We'll consider it for .3 then. Any way to test this?
Testing it involves creating a 2+GB file, so not really at least on our build slaves.
Whiteboard: [needs 1.9.1.x landing: needs approval][tb3needs] → [needs 1.9.1.x landing][tb3needs]
Attachment #373164 - Flags: approval1.9.1.3? → approval1.9.1.3+
Comment on attachment 373164 [details] [diff] [review]
v.1.1, convert stat, lstat, statvs, truncate to 64bit where available, 1.9.1 edition

Approved for 1.9.1.3. a=ss
doesn't NTFS and linux file systems (and hopefully HFS+?) support holes in files? i.e. you could:

  fd = fopen("foo", "w");
  fseek(fd, 5 GB, SEEK_SET);
  fputs("1", fd);

and that wouldn't actually take up 5 GB.
Keywords: checkin-needed
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4ca9e3371fbf
status1.9.1: --- → .3-fixed
Keywords: checkin-needed
Whiteboard: [needs 1.9.1.x landing][tb3needs] → [tb3needs]
Whiteboard: [tb3needs]

Updated

8 years ago
Duplicate of this bug: 505517
(Assignee)

Updated

8 years ago
Duplicate of this bug: 476216

Comment 31

7 years ago
Hi,

I am uploading TB 2.0.0.23 patch for this bug based on the
patch already landed in TB3 series.

TB 2.0.0.23 and previous versions suffer from the same bug
(and incorrect checking of folder size Bug 387502 )
and the data loss was reported.

I have uploaded  TB 2.0.0.23 patch for Bug 387502 (of nsLocalMailFolder.cpp),
and the patches uploaded here together should solve
the TB2.0.0.2x series creating 4GB+ folder.

TIA.

Comment 32

7 years ago
Created attachment 413987 [details] [diff] [review]
TB 2.0.0.23 patch (of the same bug).

Backported patch for TB 2.0.0.2x series.

Updated

7 years ago
Attachment #413987 - Flags: review?(benjamin)

Comment 33

7 years ago
Comment on attachment 413987 [details] [diff] [review]
TB 2.0.0.23 patch (of the same bug).

review requested.

Comment 34

7 years ago
Which version of Firefox will this fix be in?
Attachment #413987 - Flags: review?(benjamin) → review+

Comment 35

7 years ago
(In reply to comment #34)
> Which version of Firefox will this fix be in?

Sorry, this is for older (and as of now, current) version of Thunderbird 2.0.0.23 series. [That is all I can say as an end user. I am still learning the ropes of
many branches of TB/FF, etc.]

Updated

7 years ago
Attachment #413987 - Flags: approval1.8.1.next?

Comment 36

7 years ago
(In reply to comment #35)
> (In reply to comment #34)
> > Which version of Firefox will this fix be in?
> 
> Sorry, this is for older (and as of now, current) version of Thunderbird
> 2.0.0.23 series. [That is all I can say as an end user. I am still learning the
> ropes of
> many branches of TB/FF, etc.]

If this fix is for Thunderbird only, why was my bug (https://bugzilla.mozilla.org/show_bug.cgi?id=476216 - posted for Firefox) merged with this one as a duplicate?

Comment 37

7 years ago
(In reply to comment #36)
> (In reply to comment #35)
> > (In reply to comment #34)
> > > Which version of Firefox will this fix be in?
> > 
> > Sorry, this is for older (and as of now, current) version of Thunderbird
> > 2.0.0.23 series. [That is all I can say as an end user. I am still learning the
> > ropes of
> > many branches of TB/FF, etc.]
> 
> If this fix is for Thunderbird only, why was my bug
> (https://bugzilla.mozilla.org/show_bug.cgi?id=476216 - posted for Firefox)
> merged with this one as a duplicate?

Maybe my short answer in comment 35 was very misleading.

This bug has something to do with a component/lilbrary called
xpcom, specifically io subsystem in it. A version of this component
has the deficiency of not using stat64() calls which must be
used for a large file under POSIX-compliant  systems instead of calling
stat().

You will note the following keywords near the top of this bugzilla entry.

>Core
>Component: 	XPCOM
>Version: 	Trunk
>Platform: 	x86 Linux 

TB2.0.0.23 uses a version of XPCOM which could not grok files of 2GB+, and
I am not sure which version of firefox (is yours 3.0.x?) uses which version
of XPCOM, but your reported problem certainly looks similar to the xpcom/io problem under linux discussed here.

*I think* that is why yours was merged as duplicate.

Comment 38

7 years ago
The accompanying patch for Bug 387502 got approval yesterday, and
so I set approval1.8.1.next of that patch to '?'.

I hope the patch for this bug, which has approval1.8.1.next to'?' for several days,  and Bug 387502 get picked up for the next release
of 2.0.0.24 soon.

TIA

Comment 39

7 years ago
Just out of curiosity:

1. Will this fix be in for Firefox 3.5.x?
2. Why did it take TWO YEARS for this to get fixed?
Comment on attachment 413987 [details] [diff] [review]
TB 2.0.0.23 patch (of the same bug).

Approved for 1.8.1.24, a=dveditz for release-drivers
Attachment #413987 - Flags: approval1.8.1.next? → approval1.8.1.next+

Comment 41

7 years ago
Standard8, can you verify that this builds on a 2.0 linux tree? I don't have either...

Comment 42

7 years ago
(In reply to comment #41)
> Standard8, can you verify that this builds on a 2.0 linux tree? I don't have
> either...

A comment from the side: I re-checked that the patch(es) compile inside
the source file directory that was created from 2.0.0.23 tarball with
the latest CVS update. I checked this morning (Jan 26, JST). So should be compilable in a similar setting IMHO.

TIA
Checked into 1.8.1.24:
Checking in xpcom/io/nsLocalFileUnix.cpp;
/cvsroot/mozilla/xpcom/io/nsLocalFileUnix.cpp,v  <--  nsLocalFileUnix.cpp
new revision: 1.126.8.4; previous revision: 1.126.8.3
done
Checking in xpcom/io/nsLocalFileUnix.h;
/cvsroot/mozilla/xpcom/io/nsLocalFileUnix.h,v  <--  nsLocalFileUnix.h
new revision: 1.24.28.2; previous revision: 1.24.28.1
Keywords: fixed1.8.1.24

Comment 44

7 years ago
(In reply to comment #43)
> Checked into 1.8.1.24:

Thank you.
You need to log in before you can comment on or make changes to this bug.