User-Agent: Mozilla/5.0 (X11; U; Linux i686; en; rv:1.9.0.11) Gecko/20080528 Epiphany/2.22 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en; rv:1.9.0.11) Gecko/20080528 Epiphany/2.22 nsLocalFileUnix.cpp even after it is patched by the fix in bug 389087 fails to check the return value of lstat{,64} in a couple of places. I also throw in a cleanup concerning using of LL_* macro i one place, and added a clarifying comment to a portion of code which I found was confusing when I checked the lstat{,64}, stat{,64} usage. The uploaded patch is inclusive of the patch proposed in bug 389087 and the excerpted changes explained below. Once the patch goes into comm-central and the version is bumped up, I can re-create the new patch against then current source. TIA Explanation. ======================================== 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. @@ -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; } ======================================== 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. @@ -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; @@ -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; Reproducible: Always

The patch commented in comment #1. (Sorry, I used the "clone this bug" link, and I assumed that all the relevant entries are copied. Unfortunately, they aren't. Currently this bug is filed against file handling of firefox, but it should be xpcom of Core.)

Comment on attachment 389734 [details] [diff] [review] Patch to nsLocalFileUnix.cpp (additional checks for lstat{,64} return values, etc. >diff -r 1e7c406956d3 xpcom/io/nsLocalFileUnix.cpp > // 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; What size is st_mtime? If it's a 32-bit type then the multiplication is 32-bit and could overflow. I think you want PRInt64(sbuf.st_mtime) * PR_MSEC_PER_SECat You use (PRInt64)value a couple times below... please C++-style PRInt64(value) instead. Looks good in general, though.

Thank you for the comment #2. I have created a new patch. I have cast the mentioned sbuf.st_mtime as suggested, and also converted a few other casts into the C++ style as requested. I am afraid that I modified the cast(s) that appeared in the original patch of the bug 389087 to follow the C++ style notation, and so the patch uploaded here is no longer the "patch in bug 38907" + my own patch, but rather "slightly modified patch of bug 38907" + my own patch. So just for the sake of completeness, I am attaching the patch for nsLocalFileUnix.cpp, AND the patch for nsLocalFileUnix.h (the patch for the header file is unmodified from the one approved in bug 389087). With the uploaded patch, you can add the patch in bug 38907 and the fixes that are proposed here. Please advise in which form the final patch should be uploaded. Thank you in advance for your attention. The patch follows.

The new patch created to follow the suggestions in comment #2. The patch now also includes the patch for nsLocalFileUnix.h approved in bug 389087 for completeness's sake.

Please also get superreview so the patch can land.

This patch does not require superreview.

Just a clarification for the original poster. So I don't need to do anything as of now (?) BTW, once this patch lands in the trunk, I would like to this patch retrofitted to TB 2.0.0.x series(!). From what I see, I guess TB3 will be at least a few months away from release, and in the meantime, people suffer from the incorrect handling of largish files and got stuck with unmodifiable folder or lose their e-mail messages (BAD!) when they use TB2.0. So I would like this patch retrofitted into TB2.0.0.x. (Please see bug 387502 why this is important. This patch is only half the required patch to fix the problem mentioned in 387502, but we do need this.) I guess I have to file another bugzilla entry for this request. Or maybe is the existing bug 387502 enough (?). TIA

This patch seems to have been obsoleted by the fix in bug 389087?

To comment # 8, the answer is "NO" because the patch in 38908 misses a couple of places where the return value of lstat64() are not checked. So the additional checking of the return value needs to be added to the file, nsLocalFileUnix.cpp. To be frank, I have no idea where the affected functions are called, but given that the flaky handling of these functions led to dataloss incidents to people whose file folders grew too large, we ought to tighten up the return value checks where we notice them.

Re-adding checkin-needed - patch still applies

What should I do? Do I have to re-generate a new patch ???

No need to do anything. Once someone checks the patch in they will mark it resolved fixed, and remove the checkin-needed keyword.

Thank you for the clarification. I will wait for the check-in. TIA

Sorry, must have looked at wrong branch. Comment 8 is correct. http://hg.mozilla.org/mozilla-central/rev/67d9577dcf44 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4ca9e3371fbf So this bug fixed alreday.