Closed Bug 598416 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: XPCOM, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+
blocking1.9.2 --- .18+
status1.9.2 --- .18-fixed

People

(Reporter: benedict, Assigned: taras.mozilla)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [tb31wants][needs branch patch for tbird topcrash] [qa-examined-192] [qa-needs-STR])

Attachments

(2 files, 2 obsolete files)

We're getting EXCEPTION_IN_PAGE_ERROR from some accessing some mmap'd data. We need to handle these exceptions.
Assignee: nobody → tglek
__try/__catch, that is!

What happens on Linux in this case?
Summary: add try/catch in nsZipCursor::Read → add __try/__catch in nsZipCursor::Read
Blocks: 595924
Summary: add __try/__catch in nsZipCursor::Read → add try/catch in places in nsZipReader.cpp that access mmaped memory directly
(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
(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
(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
Attached patch workaround (obsolete) — Splinter Review
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.
Attachment #477604 - Flags: review?
Attachment #477604 - Flags: review? → review?(benjamin)
Blocks: 574339, 588873
Attached patch Now with more try/catching (obsolete) — Splinter Review
Attachment #477604 - Attachment is obsolete: true
Attachment #477686 - Flags: review?(benjamin)
Attachment #477604 - Flags: review?(benjamin)
Blocks: 596826
Attachment #477686 - Flags: review?(benjamin) → review+
Requesting blocking status as this is a lowrisk crashfix.
blocking2.0: --- → ?
blocking2.0: ? → betaN+
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
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 ?            \
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.
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?
(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.
Attached patch cleaned up patchSplinter Review
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.
Attachment #477686 - Attachment is obsolete: true
Attachment #489876 - Flags: review+
Looks like all of the fails I was worried about were known random-oranges. Going to land this.
http://hg.mozilla.org/mozilla-central/rev/be3852cc225b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 611984
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
Blocks: 622523
(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?
(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
would like to see this land in 1.9.2 to bake and ship to resolve Thunderbird crashes.
blocking1.9.2: --- → ?
I don't think we'd hold a release for this so please push for a landing. Marking "needed" so approval requests get expedited.
blocking1.9.2: ? → needed
(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?
blocking1.9.2: needed → .18+
Whiteboard: [tb31needs]
Whiteboard: [tb31needs] → [tb31needs][needs branch patch for tbird topcrash]
Attached patch Patch for 1.9.2Splinter Review
It's largely the same as the original patch, except a new try/catch is added for file extract since there is no zipcursor.
Attachment #537416 - Flags: review?(tglek)
Whiteboard: [tb31needs][needs branch patch for tbird topcrash] → [tb31wants][needs branch patch for tbird topcrash]
Comment on attachment 537416 [details] [diff] [review]
Patch for 1.9.2

># HG changeset patch
># User Michael Wu <mwu@mozilla.com>
># 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;
> 
> /**
Attachment #537416 - Flags: review?(tglek) → review+
Attachment #537416 - Flags: approval1.9.2.18+
Thanks a lot for getting to this guys!
What is the STR here, especially for 1.9.2?
Whiteboard: [tb31wants][needs branch patch for tbird topcrash] → [tb31wants][needs branch patch for tbird topcrash] [qa-examined-192] [qa-needs-STR]
(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.
Depends on: 1215214
You need to log in before you can comment on or make changes to this bug.