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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: benedict, Assigned: taras.mozilla)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+, blocking1.9.2 .18+, status1.9.2 .18-fixed)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

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

Updated

9 years ago
Assignee: nobody → tglek

Comment 1

9 years ago
__try/__catch, that is!

What happens on Linux in this case?
Summary: add try/catch in nsZipCursor::Read → add __try/__catch in nsZipCursor::Read
Reporter

Updated

9 years ago
Blocks: 595924
Assignee

Updated

9 years ago
Summary: add __try/__catch in nsZipCursor::Read → add try/catch in places in nsZipReader.cpp that access mmaped memory directly
Assignee

Comment 2

9 years ago
(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
Assignee

Comment 4

9 years ago
(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
Assignee

Comment 5

9 years ago
Posted 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?
Assignee

Updated

9 years ago
Attachment #477604 - Flags: review? → review?(benjamin)
Assignee

Updated

9 years ago
Blocks: 574339, 588873
Assignee

Comment 6

9 years ago
Posted 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)
Assignee

Updated

9 years ago
Blocks: 596826

Updated

9 years ago
Attachment #477686 - Flags: review?(benjamin) → review+
Assignee

Comment 7

9 years ago
Requesting blocking status as this is a lowrisk crashfix.
blocking2.0: --- → ?

Updated

9 years ago
blocking2.0: ? → betaN+

Comment 8

9 years ago
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

9 years ago
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 ?            \
Assignee

Comment 10

9 years ago
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?
Assignee

Comment 12

9 years ago
(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.
Assignee

Comment 13

9 years ago
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+
Assignee

Comment 14

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

Comment 15

9 years ago
http://hg.mozilla.org/mozilla-central/rev/be3852cc225b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
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
Assignee

Comment 17

9 years ago
(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]
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]
Assignee

Comment 23

8 years ago
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+

Updated

8 years ago
Attachment #537416 - Flags: approval1.9.2.18+

Comment 24

8 years ago
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.

Updated

4 years ago
Depends on: 1215214
You need to log in before you can comment on or make changes to this bug.