Last Comment Bug 389087 - nsILocalFileUnix affected by 32bit stat/statvfs/truncate, therefore does not work with large files
: nsILocalFileUnix affected by 32bit stat/statvfs/truncate, therefore does not ...
Status: RESOLVED FIXED
: fixed1.8.1.24
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Linux
: -- major (vote)
: mozilla1.9.2a1
Assigned To: Nils Maier [:nmaier]
:
Mentors:
: 476216 505517 (view as bug list)
Depends on: 389089
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-21 09:06 PDT by Nils Maier [:nmaier]
Modified: 2010-02-05 07:15 PST (History)
14 users (show)
reed: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.3-fixed


Attachments
v1, convert stat, lstar, statvs, truncate to 64bit where available (11.31 KB, patch)
2007-07-21 09:08 PDT, Nils Maier [:nmaier]
benjamin: review+
dougt: superreview+
Details | Diff | Review
v.1.1, convert stat, lstat, statvs, truncate to 64bit where available, central edition [Checkin: Comment 15] (10.85 KB, patch)
2009-04-07 10:31 PDT, Nils Maier [:nmaier]
benjamin: review+
benjamin: superreview+
Details | Diff | Review
v.1.1, convert stat, lstat, statvs, truncate to 64bit where available, 1.9.1 edition (11.43 KB, patch)
2009-04-16 10:39 PDT, Nils Maier [:nmaier]
benjamin: review+
benjamin: superreview+
samuel.sidler+old: approval1.9.1.3+
Details | Diff | Review
TB 2.0.0.23 patch (of the same bug). (12.25 KB, patch)
2009-11-22 20:19 PST, ISHIKAWA, Chiaki
benjamin: review+
dveditz: approval1.8.1.next+
Details | Diff | Review

Description Nils Maier [:nmaier] 2007-07-21 09:06:13 PDT
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.
Comment 1 Nils Maier [:nmaier] 2007-07-21 09:08:22 PDT
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.
Comment 2 Reed Loden [:reed] (use needinfo?) 2008-01-02 01:37:33 PST
Comment on attachment 273245 [details] [diff] [review]
v1, convert stat, lstar, statvs, truncate to 64bit where available

Try bsmedberg...
Comment 3 timeless 2008-01-02 03:40:59 PST
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)
Comment 4 Nils Maier [:nmaier] 2008-01-02 04:58:19 PST
(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 Doug Turner (:dougt) 2008-01-02 09:01:20 PST
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;
Comment 6 Nils Maier [:nmaier] 2008-01-02 09:37:23 PST
(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.
Comment 7 Benjamin Smedberg [:bsmedberg] 2008-03-04 18:13:29 PST
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?
Comment 8 Reed Loden [:reed] (use needinfo?) 2008-03-04 21:18:04 PST
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.
Comment 9 timeless 2008-04-08 02:14:33 PDT
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.
Comment 10 Shawn Wilsher :sdwilsh 2008-10-25 21:27:54 PDT
Doug - any chance on that sr?  I've got people pinging me about this...
Comment 11 Doug Turner (:dougt) 2009-03-12 07:47:44 PDT
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.
Comment 12 Serge Gautherie (:sgautherie) 2009-03-15 07:05:19 PDT
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.
Comment 13 Dão Gottwald [:dao] 2009-04-05 07:48:40 PDT
3 out of 11 hunks failed to apply.
Comment 14 Nils Maier [:nmaier] 2009-04-07 10:31:38 PDT
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.
Comment 15 Serge Gautherie (:sgautherie) 2009-04-16 05:30:03 PDT
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
Comment 16 Serge Gautherie (:sgautherie) 2009-04-16 05:33:03 PDT
(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
}
Comment 17 Nils Maier [:nmaier] 2009-04-16 10:39:09 PDT
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.
Comment 18 ISHIKAWA, Chiaki 2009-07-20 12:05:18 PDT
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 ISHIKAWA, Chiaki 2009-07-20 12:14:10 PDT
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;
 }
Comment 20 Shawn Wilsher :sdwilsh 2009-07-20 14:05:26 PDT
It'd be much more useful if you filed a new bug and created a patch:
https://developer.mozilla.org/en/Creating_a_patch
Comment 21 ISHIKAWA, Chiaki 2009-07-21 23:55:40 PDT
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 :-( ]
Comment 22 Samuel Sidler (old account; do not CC) 2009-07-23 09:40:46 PDT
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 23 Benjamin Smedberg [:bsmedberg] 2009-07-23 10:03:30 PDT
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.
Comment 24 Samuel Sidler (old account; do not CC) 2009-07-23 10:31:23 PDT
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?
Comment 25 Benjamin Smedberg [:bsmedberg] 2009-07-23 10:33:04 PDT
Testing it involves creating a 2+GB file, so not really at least on our build slaves.
Comment 26 Samuel Sidler (old account; do not CC) 2009-08-10 19:51:58 PDT
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
Comment 27 Christian :Biesinger (don't email me, ping me on IRC) 2009-08-10 22:36:35 PDT
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.
Comment 29 Magnus Melin 2009-08-24 11:43:29 PDT
*** Bug 505517 has been marked as a duplicate of this bug. ***
Comment 30 Nils Maier [:nmaier] 2009-10-06 08:38:14 PDT
*** Bug 476216 has been marked as a duplicate of this bug. ***
Comment 31 ISHIKAWA, Chiaki 2009-11-22 20:18:05 PST
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 ISHIKAWA, Chiaki 2009-11-22 20:19:29 PST
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.
Comment 33 ISHIKAWA, Chiaki 2009-11-22 20:22:07 PST
Comment on attachment 413987 [details] [diff] [review]
TB 2.0.0.23 patch (of the same bug).

review requested.
Comment 34 donrhummy 2009-11-23 06:46:11 PST
Which version of Firefox will this fix be in?
Comment 35 ISHIKAWA, Chiaki 2009-11-23 07:19:56 PST
(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.]
Comment 36 donrhummy 2009-11-24 07:25:41 PST
(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 ISHIKAWA, Chiaki 2009-11-24 08:30:05 PST
(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 ISHIKAWA, Chiaki 2009-12-02 09:13:37 PST
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 donrhummy 2009-12-17 11:57:45 PST
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 40 Daniel Veditz [:dveditz] 2010-01-19 13:04:02 PST
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
Comment 41 David :Bienvenu 2010-01-19 14:30:02 PST
Standard8, can you verify that this builds on a 2.0 linux tree? I don't have either...
Comment 42 ISHIKAWA, Chiaki 2010-01-25 23:50:11 PST
(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
Comment 43 Mark Banner (:standard8) 2010-02-05 04:02:09 PST
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
Comment 44 ISHIKAWA, Chiaki 2010-02-05 07:15:25 PST
(In reply to comment #43)
> Checked into 1.8.1.24:

Thank you.

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