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.

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 on attachment 273245 [details] [diff] [review] v1, convert stat, lstar, statvs, truncate to 64bit where available Try bsmedberg...

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)

(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 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;

(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 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 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.

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.

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.

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 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

(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 }

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.

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;

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

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 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.

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.

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.

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.

Backported patch for TB 2.0.0.2x series.

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

Which version of Firefox will this fix be in?

(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.]

(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?

(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.

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

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

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

(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

(In reply to comment #43) > Checked into 1.8.1.24: Thank you.