505517 - nsLocalFileUnix.cpp fails to check the return value of lstat{,64} in a couple of places.

Status: RESOLVED DUPLICATE of bug 389087

(Core :: XPCOM, defect)

Description

[ISHIKAWA, Chiaki]

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

Comment 1

[ISHIKAWA, Chiaki]

Attachment: Patch to nsLocalFileUnix.cpp (additional checks for lstat{,64} return values, etc.

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 2

[Benjamin Smedberg]

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.

Comment 3

[ISHIKAWA, Chiaki]

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.

Comment 4

[ISHIKAWA, Chiaki]

Attachment: Patch to nsLocalFileUnix.cpp (additional checks for lstat{,64} and a few other changes)

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.

Comment 5

[Magnus Melin [:mkmelin]]

Please also get superreview so the patch can land.

Comment 6

[Benjamin Smedberg]

This patch does not require superreview.

Comment 7

[ISHIKAWA, Chiaki]

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

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 9

[ISHIKAWA, Chiaki]

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.

Comment 10

[Magnus Melin [:mkmelin]]

Re-adding checkin-needed - patch still applies

Comment 11

[ISHIKAWA, Chiaki]

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

Comment 12

[Magnus Melin [:mkmelin]]

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

Comment 13

[ISHIKAWA, Chiaki]

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

TIA

Comment 14

[Magnus Melin [:mkmelin]]

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.