598416 - add try/catch in places in nsZipReader.cpp that access mmaped memory directly

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benedict Hsieh [:benedict]]

We're getting EXCEPTION_IN_PAGE_ERROR from some accessing some mmap'd data. We need to handle these exceptions.

Comment 1

[Benjamin Smedberg]

__try/__catch, that is!

What happens on Linux in this case?

Comment 2

[(dormant account)]

(In reply to comment #1)
> __try/__catch, that is!
> 
> What happens on Linux in this case?

SIGBUS. I haven't seen a single such crash on Linux

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #0)
> We're getting EXCEPTION_IN_PAGE_ERROR from some accessing some mmap'd data. We
> need to handle these exceptions.

Why are we getting this exception?

/be

Comment 4

[(dormant account)]

(In reply to comment #3)
> (In reply to comment #0)
> > We're getting EXCEPTION_IN_PAGE_ERROR from some accessing some mmap'd data. We
> > need to handle these exceptions.
> 
> Why are we getting this exception?

ms docs are very vague about this. Supposedly it's due to files residing on network drives that disappear, but also due to virtual memory exhaustion(more likely).

http://msdn.microsoft.com/en-us/library/aa366801%28VS.85%29.aspx

Comment 5

[(dormant account)]

Attachment: workaround

This should work. Unfortunately the only way to test this is to land it and see if the crashes go away. 

A similar fix needs to be applied to fasl code. Luckily due to whole-file checksum, it's more localized there.

Comment 6

[(dormant account)]

Attachment: Now with more try/catching

Comment 7

[(dormant account)]

Requesting blocking status as this is a lowrisk crashfix.

Comment 8

[timeless]

Comment on attachment 477686 [details] [diff] [review]
Now with more try/catching

I don't think the ;'s are needed, you only use them 2/8 times.
>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(rv = NS_ERROR_FAILURE);

>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(return nsnull)

>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE)

>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE)

>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE);

>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(return nsnull)

>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(return nsnull)

>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(return nsnull)

>+#define MOZ_WIN_MEM_TRY_BEGIN __try {
>+#define MOZ_WIN_MEM_TRY_CATCH(cmd) }                                    \
>+  __except(GetExceptionCode()==EXCEPTION_IN_PAGE_ERROR ?            \
>+           EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH)   \
>+  {                                                                 \
>+    cmd;                                                            \
>+  }
>+#else
>+#define MOZ_WIN_MEM_TRY_BEGIN
>+#define MOZ_WIN_MEM_TRY_CATCH(cmd)
>+#endif

Comment 9

[timeless]

oh, and the second #define seems to have it's \ too far to the right.

perhaps:
+#define MOZ_WIN_MEM_TRY_BEGIN      __try {
+#define MOZ_WIN_MEM_TRY_CATCH(cmd) }                                \
+  __except(GetExceptionCode()==EXCEPTION_IN_PAGE_ERROR ?            \

Comment 10

[(dormant account)]

This patch adds try/catch to the code. This causes a mochitest failure:

ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_range_bounds.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - [object Event @ 0x74667c0 (native @ 0x6c3bef0)] at undefined:undefined

This failure doesn't occur if I change the defines to expand to { }(ie keep scoping, get rid of try/catch). The failure occurs even though no exception is caught(ie control flow remains the same). I am still trying to figure this out.

Comment 11

[Benjamin Smedberg]

That appears to be random-orange bug 608918, and probably isn't related to this bug at all. You see that failure on Windows only?

Comment 12

[(dormant account)]

(In reply to comment #11)
> That appears to be random-orange bug 608918, and probably isn't related to this
> bug at all. You see that failure on Windows only?

The try/catch isn't applicable on non-windows platforms.

Comment 13

[(dormant account)]

Attachment: cleaned up patch

Minor whitespace cleanups. Added an NS_WARNING() to the catch. Made the try/catch defines expand to {} on non-windows to keep scoping the same across platforms.

Comment 14

[(dormant account)]

Looks like all of the fails I was worried about were known random-oranges. Going to land this.

Comment 15

[(dormant account)]

http://hg.mozilla.org/mozilla-central/rev/be3852cc225b

Comment 16

[Wayne Mery (:wsmwk)]

Taras. Is this solid enough to land in 1.9.2?  It should fix at least 2 thunderbird topcrashes, and probably more.  (eg bug 622523)

I haven't checked any of these https://crash-stats.mozilla.com/query/query?product=Thunderbird&version=ALL%3AALL&range_value=2&range_unit=weeks&date=01%2F04%2F2011+10%3A06%3A12&query_search=signature&query_type=contains&query=nsJAR&build_id=&process_type=any&hang_type=any&do_query=1

... except memcmp | nsZipArchive::GetItem(char const*) bp-8706682f-53d0-44e4-bfaf-e8d9a2110104

Comment 17

[(dormant account)]

(In reply to comment #16)
> Taras. Is this solid enough to land in 1.9.2?  It should fix at least 2
> thunderbird topcrashes, and probably more.  (eg bug 622523)
> 

yeah it seems solid on trunk.

> ... except memcmp | nsZipArchive::GetItem(char const*)
> bp-8706682f-53d0-44e4-bfaf-e8d9a2110104

why is this one excepted?

Comment 18

[Wayne Mery (:wsmwk)]

(In reply to comment #17)
> (In reply to comment #16)
> > Taras. Is this solid enough to land in 1.9.2?  It should fix at least 2
> > thunderbird topcrashes, and probably more.  (eg bug 622523)
> > 
> 
> yeah it seems solid on trunk.

great! can you make that happen please?


> > ... except memcmp | nsZipArchive::GetItem(char const*)
> > bp-8706682f-53d0-44e4-bfaf-e8d9a2110104
> 
> why is this one excepted?

(poor wording) I mean, I checked that specific crash, but none of the rest in 
https://crash-stats.mozilla.com/query/query?product=Thunderbird&version=ALL%3AALL&range_value=2&range_unit=weeks&date=01%2F04%2F2011+10%3A06%3A12&query_search=signature&query_type=contains&query=nsJAR&build_id=&process_type=any&hang_type=any&do_query=1

Comment 19

[Wayne Mery (:wsmwk)]

would like to see this land in 1.9.2 to bake and ship to resolve Thunderbird crashes.

Comment 20

[Daniel Veditz [:dveditz]]

I don't think we'd hold a release for this so please push for a landing. Marking "needed" so approval requests get expedited.

Comment 21

[Wayne Mery (:wsmwk)]

(Taras in comment #17)
> (In reply to comment #16) 
> > ... except memcmp | nsZipArchive::GetItem(char const*)
> > bp-8706682f-53d0-44e4-bfaf-e8d9a2110104
> 
> why is this one excepted?

Meaning that I *did* check it/contacted the reporter.
Taras, can you prepare a patch for branch?

Comment 22

[Michael Wu [:mwu]]

Attachment: Patch for 1.9.2

It's largely the same as the original patch, except a new try/catch is added for file extract since there is no zipcursor.

Comment 23

[(dormant account)]

Comment on attachment 537416 [details] [diff] [review]
Patch for 1.9.2

># HG changeset patch
># User Michael Wu <[e-mail]>
># Parent 36bf853240c7e65f9ec19d92fbaa05b6866dbab6
>Bug 598416 - add try/catch in places in nsZipReader.cpp that access mmaped memory directly r=bsmedberg try: -b do -p win32 -u all -t none
>
>diff --git a/modules/libjar/nsJARInputStream.cpp b/modules/libjar/nsJARInputStream.cpp
>--- a/modules/libjar/nsJARInputStream.cpp
>+++ b/modules/libjar/nsJARInputStream.cpp
>@@ -47,6 +47,9 @@
> #include "nsEscape.h"
> #include "nsIFile.h"
> #include "nsDebug.h"
>+#if defined(XP_WIN)
>+#include <windows.h>
>+#endif
> 
> /*---------------------------------------------
>  *  nsISupports implementation
>@@ -212,6 +215,7 @@ nsJARInputStream::Read(char* aBuffer, PR
>     *aBytesRead = 0;
> 
>     nsresult rv = NS_OK;
>+MOZ_WIN_MEM_TRY_BEGIN
>     switch (mMode) {
>       case MODE_NOTINITED:
>         return NS_OK;
>@@ -250,6 +254,7 @@ nsJARInputStream::Read(char* aBuffer, PR
>         }
>         break;
>     }
>+MOZ_WIN_MEM_TRY_CATCH(rv = NS_ERROR_FAILURE)
>     return rv;
> }
> 
>diff --git a/modules/libjar/nsZipArchive.cpp b/modules/libjar/nsZipArchive.cpp
>--- a/modules/libjar/nsZipArchive.cpp
>+++ b/modules/libjar/nsZipArchive.cpp
>@@ -59,6 +59,9 @@
> #include "stdlib.h"
> #include "nsWildCard.h"
> #include "nsZipArchive.h"
>+#if defined(XP_WIN)
>+#include <windows.h>
>+#endif
> 
> /**
>  * Global allocator used with zlib. Destroyed in module shutdown.
>@@ -314,7 +317,7 @@ nsZipItem*  nsZipArchive::GetItem(const 
>                 return 0;
>         }
>     }
>-
>+MOZ_WIN_MEM_TRY_BEGIN
>     nsZipItem* item = mFiles[ HashName(aEntryName, len) ];
>     while (item) {
>       if ((len == item->nameLength) && 
>@@ -322,8 +325,9 @@ nsZipItem*  nsZipArchive::GetItem(const 
>         return item; //-- found it
>       item = item->next;
>     }
>+MOZ_WIN_MEM_TRY_CATCH(return nsnull)
>   }
>-  return 0;
>+  return nsnull;
> }
> 
> //---------------------------------------------
>@@ -341,6 +345,7 @@ nsresult nsZipArchive::ExtractFile(nsZip
>   if (!mFd)
>     return NS_ERROR_FAILURE;
> 
>+MOZ_WIN_MEM_TRY_BEGIN
>   // Directory extraction is handled in nsJAR::Extract,
>   // so the item to be extracted should never be a directory
>   PR_ASSERT(!item->IsDirectory());
>@@ -373,8 +378,8 @@ nsresult nsZipArchive::ExtractFile(nsZip
>       rv = ResolveSymlink(outname);
> #endif
>   }
>-
>   return rv;
>+MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE)
> }
> 
> //---------------------------------------------
>@@ -445,7 +450,7 @@ nsresult nsZipFind::FindNext(const char 
> 
>   *aResult = 0;
>   *aNameLen = 0;
>-
>+MOZ_WIN_MEM_TRY_BEGIN
>   // we start from last match, look for next
>   while (mSlot < ZIP_TABSIZE)
>   {
>@@ -474,7 +479,7 @@ nsresult nsZipFind::FindNext(const char 
>       return NS_OK;
>     }
>   }
>-
>+MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE)
>   return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST;
> }
> 
>@@ -526,7 +531,7 @@ nsresult nsZipArchive::BuildFileList()
>   PRUint8* buf;
>   PRUint8* startp = mFd->mFileData;
>   PRUint8* endp = startp + mFd->mLen;
>-
>+MOZ_WIN_MEM_TRY_BEGIN
>   PRUint32 centralOffset = 0;
>   for (buf = endp - ZIPEND_SIZE; buf > startp; buf--)
>   {
>@@ -582,6 +587,8 @@ nsresult nsZipArchive::BuildFileList()
> 
>   if (sig != ENDSIG)
>     return NS_ERROR_FILE_CORRUPTED;
>+
>+MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE)
>   return NS_OK;
> }
> 
>@@ -594,6 +601,7 @@ nsresult nsZipArchive::BuildSynthetics()
>     return NS_OK;
>   mBuiltSynthetics = true;
> 
>+MOZ_WIN_MEM_TRY_BEGIN
>   // Create synthetic entries for any missing directories.
>   // Do this when all ziptable has scanned to prevent double entries.
>   for (int i = 0; i < ZIP_TABSIZE; ++i)
>@@ -650,6 +658,7 @@ nsresult nsZipArchive::BuildSynthetics()
>       } /* end processing of dirs in item's name */
>     }
>   }
>+MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE)
>   return NS_OK;
> }
> 
>@@ -666,6 +675,7 @@ nsZipHandle* nsZipArchive::GetFD()
> PRUint8* nsZipArchive::GetData(nsZipItem* aItem)
> {
>   PR_ASSERT (aItem);
>+MOZ_WIN_MEM_TRY_BEGIN
> 
>   //-- read local header to get variable length values and calculate
>   //-- the real data offset
>@@ -692,6 +702,7 @@ PRUint8* nsZipArchive::GetData(nsZipItem
>     return nsnull;
> 
>   return data + offset;
>+MOZ_WIN_MEM_TRY_CATCH(return nsnull)
> }
> 
> //---------------------------------------------
>diff --git a/modules/libjar/nsZipArchive.h b/modules/libjar/nsZipArchive.h
>--- a/modules/libjar/nsZipArchive.h
>+++ b/modules/libjar/nsZipArchive.h
>@@ -53,8 +53,21 @@
> #include "zipstruct.h"
> #include "nsAutoPtr.h"
> 
>+#if defined(XP_WIN)
>+#define MOZ_WIN_MEM_TRY_BEGIN __try {
>+#define MOZ_WIN_MEM_TRY_CATCH(cmd) }                                \
>+  __except(GetExceptionCode()==EXCEPTION_IN_PAGE_ERROR ?            \
>+           EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH)   \
>+  {                                                                 \
>+    NS_WARNING("EXCEPTION_IN_PAGE_ERROR in " __FUNCTION__);         \
>+    cmd;                                                            \
>+  }
>+#else
>+#define MOZ_WIN_MEM_TRY_BEGIN {
>+#define MOZ_WIN_MEM_TRY_CATCH(cmd) }
>+#endif
>+
> class nsZipFind;
>-
> struct PRFileDesc;
> 
> /**

Comment 24

[LegNeato]

Thanks a lot for getting to this guys!

Comment 25

[Michael Wu [:mwu]]

Optimistically marking fixed.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fd7fb656724f

Comment 26

[Al Billings [:abillings - ex-MoCo]]

What is the STR here, especially for 1.9.2?

Comment 27

[Karl Tomlinson (:karlt)]

(In reply to Taras Glek (:taras) from comment #2)
> (In reply to comment #1)
> > __try/__catch, that is!
> > 
> > What happens on Linux in this case?
> 
> SIGBUS. I haven't seen a single such crash on Linux

Bug 682607 perhaps.