1677194 - (CVE-2021-23962) ASAN error, use-after-poison, nsTreeBodyFrame::RowCountChanged(int, int)

ISHIKAWA, Chiaki

Assignee

Description

•

4 years ago

Attached file ASAN error log for use-after-poison — Details

I noticed a use-after-poison error when I ran
ASAN-version of C-C TB under its mochitest test suite
locally.

The full log is attached.
The line numbers may be slightly off due to local patches.

The error is detected during
test_delete_last_message_from_virtual_folder_closes_message_displays test
in
comm/mail/test/browser/folder-display/browser_deletionFromVirtualFolders.js

I think it relates to
nsTreeBodyFreame::RowCountChanged() <- the user
NS_NewBoxFrame() <- allocator

So I am choosing the component accordingly, but maybe I am wrong.

208:45.77 TEST_START: comm/mail/test/browser/folder-display/browser_deletionFromVirtualFolders.js
208:45.77 GECKO(95056) Chrome file doesn't exist: /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/_tests/testing/mochitest/browser/comm/mail/test/browser/folder-display/head.js
208:45.78 INFO Entering test bound setupModule
...
209:27.67 INFO Entering test bound test_delete_last_message_from_virtual_folder_closes_message_displays
...
209:27.78 GECKO(95056) ==95056==ERROR: AddressSanitizer: use-after-poison on address 0x62500006376c at pc 0x7f527a848909 bp 0x7ffcb55a9f90 sp 0x7ffcb55a9f88
209:27.78 GECKO(95056) READ of size 4 at 0x62500006376c thread T0
209:28.58 GECKO(95056) error: address range table at offset 0xd50 has an invalid tuple (length = 0) at offset 0xd60
209:28.96 GECKO(95056)	   #0 0x7f527a848908 in nsTreeBodyFrame::RowCountChanged(int, int) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/xul/tree/nsTreeBodyFrame.cpp:1606:7
209:28.97 GECKO(95056)	   #1 0x7f526c12a0ab in nsMsgDBView::NoteChange(unsigned int, int, int) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBView.cpp:5951:16
209:28.97 GECKO(95056)	   #2 0x7f526c0fdfa4 in nsMsgDBView::RemoveByIndex(unsigned int) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBView.cpp:3001:5
209:28.97 GECKO(95056)	   #3 0x7f526c29c082 in nsMsgThreadedDBView::RemoveByIndex(unsigned int) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgThreadedDBView.cpp:869:25
209:28.97 GECKO(95056)	   #4 0x7f526c128fd0 in nsMsgDBView::OnHdrDeleted(nsIMsgDBHdr*, unsigned int, int, nsIDBChangeListener*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBView.cpp:5844:5
209:28.98 GECKO(95056)	   #5 0x7f526c17f91b in nsMsgGroupView::OnHdrDeleted(nsIMsgDBHdr*, unsigned int, int, nsIDBChangeListener*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgGroupView.cpp:636:25
209:28.98 GECKO(95056)	   #6 0x7f526c25eb47 in nsMsgQuickSearchDBView::OnHdrDeleted(nsIMsgDBHdr*, unsigned int, int, nsIDBChangeListener*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgQuickSearchDBView.cpp:798:31
209:28.99 GECKO(95056)	   #7 0x7f526c5508e4 in nsMsgDatabase::NotifyHdrDeletedAll(nsIMsgDBHdr*, unsigned int, int, nsIDBChangeListener*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/db/msgdb/src/nsMsgDatabase.cpp:817:3
209:28.99 GECKO(95056)	   #8 0x7f526c55f2bd in nsMsgDatabase::DeleteHeader(nsIMsgDBHdr*, nsIDBChangeListener*, bool, bool) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/db/msgdb/src/nsMsgDatabase.cpp:1844:5
209:28.99 GECKO(95056)	   #9 0x7f526c9f1925 in nsMsgLocalMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, bool, bool, nsIMsgCopyServiceListener*, bool) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:1063:25
209:29.00 GECKO(95056)	   #10 0x7f526ca1616d in nsMsgLocalMailFolder::EndMove(bool) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:2699:21
209:29.00 GECKO(95056)	   #11 0x7f526bf58669 in nsCopyMessageStreamListener::EndCopy(nsISupports*, nsresult) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsCopyMessageStreamListener.cpp:135:39
209:29.00 GECKO(95056)	   #12 0x7f526bf5928d in nsCopyMessageStreamListener::OnStopRequest(nsIRequest*, nsresult) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsCopyMessageStreamListener.cpp:155:10
209:29.00 GECKO(95056)	   #13 0x7f526c22dcad in nsMsgProtocol::OnStopRequest(nsIRequest*, nsresult) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgProtocol.cpp:387:29
209:29.00 GECKO(95056)	   #14 0x7f526ca4b636 in nsMailboxProtocol::OnStopRequest(nsIRequest*, nsresult) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsMailboxProtocol.cpp:400:18
209:29.02 GECKO(95056)	   #15 0x7f526d73210d in nsInputStreamPump::OnStateStop() /NEW-SSD/NREF-COMM-CENTRAL/mozilla/netwerk/base/nsInputStreamPump.cpp:649:16
209:29.02 GECKO(95056)	   #16 0x7f526d72fec5 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/netwerk/base/nsInputStreamPump.cpp:397:21
209:29.02 GECKO(95056)	   #17 0x7f526d732bfc in non-virtual thunk to nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/netwerk/base/nsInputStreamPump.cpp
209:29.03 GECKO(95056)	   #18 0x7f526d1a04d8 in mozilla::SlicedInputStream::OnInputStreamReady(nsIAsyncInputStream*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/io/SlicedInputStream.cpp:416:22
209:29.04 GECKO(95056)	   #19 0x7f526d21c23d in nsInputStreamReadyEvent::Run() /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/io/nsStreamUtils.cpp:94:20
209:29.05 GECKO(95056)	   #20 0x7f526d2da356 in mozilla::RunnableTask::Run() /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/threads/TaskController.cpp:450:16
209:29.05 GECKO(95056)	   #21 0x7f526d2cb29f in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/threads/TaskController.cpp:720:26
209:29.05 GECKO(95056)	   #22 0x7f526d2c84ab in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/threads/TaskController.cpp:579:15
209:29.05 GECKO(95056)	   #23 0x7f526d2c8afd in mozilla::TaskController::ProcessPendingMTTask(bool) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/threads/TaskController.cpp:373:36
209:29.05 GECKO(95056)	   #24 0x7f526d2cc9a9 in operator() /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/threads/TaskController.cpp:123:37
209:29.05 GECKO(95056)	   #25 0x7f526d2cc9a9 in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_4>::Run() /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/nsThreadUtils.h:577:5
209:29.08 GECKO(95056)	   #26 0x7f526d30e5ec in nsThread::ProcessNextEvent(bool, bool*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/threads/nsThread.cpp:1197:14
209:29.08 GECKO(95056)	   #27 0x7f526d3904c1 in NS_InvokeByIndex /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:101
209:29.10 GECKO(95056)	   #28 0x7f526feae752 in Invoke /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/xpconnect/src/XPCWrappedNative.cpp:1620:10
209:29.10 GECKO(95056)	   #29 0x7f526feae752 in CallMethodHelper::Call() /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/xpconnect/src/XPCWrappedNative.cpp:1176:19
209:29.10 GECKO(95056)	   #30 0x7f526fe6f2b3 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/xpconnect/src/XPCWrappedNative.cpp:1142:23
209:29.10 GECKO(95056)	   #31 0x7f526fe73043 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:925:10
209:29.10 GECKO(95056)	   #32 0x178b5a98e80e  (<unknown module>)

Full log is attached.
Just in case, I am setting security flag. Please clear it as people see fit.

Daniel Veditz [:dveditz]

Updated

•

4 years ago

Group: core-security → layout-core-security

Keywords: csectype-framepoisoning

Emilio Cobos Álvarez (:emilio)

Comment 1

•

4 years ago

Seems like this can flush layout and kill the frame.

So easy fix would be to add an AutoWeakFrame around it and check it.

ISHIKAWA, Chiaki

Assignee

Comment 2

•

4 years ago

•

Edited

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

Seems like this can flush layout and kill the frame.

So easy fix would be to add an AutoWeakFrame around it and check it.

  // Adjust our selection.
  nsCOMPtr<nsITreeSelection> sel;
  mView->GetSelection(getter_AddRefs(sel)); <===  I take |sel| becomes the invalid pointer. 
  if (sel) sel->AdjustSelection(aIndex, aCount);

I do hit selection-count related assertions, etc. during C-C TB during testing for some time , and I have been wondering how that can happen.

It would be great that at least this particular problem can be eliminated.

BTW, shouldn't the code be better like the following?

  nsCOMPtr<nsITreeSelection> sel = nullptr;
 nsresult rc =  mView->GetSelection(getter_AddRefs(sel)); <===  I take |sel| becomes the invalid pointer. 
  if (NS_OK(rc) && sel) sel->AdjustSelection(aIndex, aCount);

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 3

•

4 years ago

I assume 'this' becomes invalid when nsTreeBodyFrame::RowCountChanged is called. That is the frame emilio mentioned.
Perhaps something like.
AutoWeakFrame weakFrame(this);
nsCOMPtr<nsITreeView> view = mView;
view->GetSelection(...);
NS_ENSURE_STATE(weakFrame.IsAlive());

ISHIKAWA, Chiaki

Assignee

Comment 4

•

4 years ago

Let me see if the above outline modifies the behavior of the ASAN test.

ISHIKAWA, Chiaki

Assignee

Comment 5

•

4 years ago

I have a question. See the comment.
(In reply to Olli Pettay [:smaug] from comment #3)

I assume 'this' becomes invalid when nsTreeBodyFrame::RowCountChanged is called. That is the frame emilio mentioned.
Perhaps something like.
AutoWeakFrame weakFrame(this);
nsCOMPtr<nsITreeView> view = mView;
view->GetSelection(...);
<==== This is |view->| and NOT |mView->| as in the original, correct?
NS_ENSURE_STATE(weakFrame.IsAlive());

ISHIKAWA, Chiaki

Assignee

Comment 6

•

4 years ago

The following patch does not eliminate the issue.
We still get the similar ASAN error.

diff --git a/layout/xul/tree/nsTreeBodyFrame.cpp b/layout/xul/tree/nsTreeBodyFrame.cpp
--- a/layout/xul/tree/nsTreeBodyFrame.cpp
+++ b/layout/xul/tree/nsTreeBodyFrame.cpp
@@ -1593,20 +1593,28 @@ nsresult nsTreeBodyFrame::RowCountChange
   if (aCount == 0 || !mView) return NS_OK;  // Nothing to do.
 
 #ifdef ACCESSIBILITY
   if (PresShell::IsAccessibilityActive()) {
     FireRowCountChangedEvent(aIndex, aCount);
   }
 #endif  // #ifdef ACCESSIBILITY
 
-  // Adjust our selection.
-  nsCOMPtr<nsITreeSelection> sel;
-  mView->GetSelection(getter_AddRefs(sel));
-  if (sel) sel->AdjustSelection(aIndex, aCount);
+  // Validate pointer. Bug 1677194
+  AutoWeakFrame weakFrame(this);
+  nsCOMPtr<nsITreeView> view = mView;
+
+  // Adjust our selection. Note the use of |view| instead of |mView| 
+  nsCOMPtr<nsITreeSelection> sel = nullptr; 
+  nsresult rc = view->GetSelection(getter_AddRefs(sel));
+
+  // Check
+  NS_ENSURE_STATE(weakFrame.IsAlive());
+
+  if (NS_SUCCEEDED(rc) && sel) sel->AdjustSelection(aIndex, aCount);
 
   if (mUpdateBatchNest) return NS_OK;
 
   mRowCount += aCount;
 #ifdef DEBUG
   int32_t rowCount = mRowCount;
   mView->GetRowCount(&rowCount);
   NS_ASSERTION(
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$

Shouldn't we add a similar check for |sel| just before the following line to see if |sel| is valid?
But I am not sure what that check is like.

  if (NS_SUCCEEDED(rc) && sel) sel->AdjustSelection(aIndex, aCount);

Well, if |sel| is invalid, there has been a path that screws up the internal data structure to return an invalid pointer value in |sel| though.

Oh, BTW, the switched use of |view->| and |mView->| do not make any differences as far as the ASAN error is concerned.

ISHIKAWA, Chiaki

Assignee

Comment 7

•

4 years ago

•

Edited

Attached file verbose log. — Details

So the problematic pointer value was used to read 4 bytes at
0x625001200e44.
BTW, at the time of the error.
this = 0x625001200d10

The memory area was allocated by the following chain of functions, and it was 8KB at the allocation time.
(Too bad, ASAN doesn't say when the area was poisoned after free?)
although the area is very close to the area pointed at by |this|,
it may not be related. I am not sure.


 1:50.87 GECKO(518898) 0x625001200e44 is located 5444 bytes inside of 8192-byte region [0x6250011ff900,0x625001201900)
 1:50.87 GECKO(518898) allocated by thread T0 here:
 1:50.93 GECKO(518898)     #0 0x55a5d5f1376d in malloc (/NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/bin/thunderbird+0x16176d)
 1:50.93 GECKO(518898)     #1 0x7f44b8ff2728 in AllocateChunk /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/mozilla/ArenaAllocator.h:171:15
 1:50.93 GECKO(518898)     #2 0x7f44b8ff2728 in InternalAllocate /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/mozilla/ArenaAllocator.h:205:25
 1:50.93 GECKO(518898)     #3 0x7f44b8ff2728 in mozilla::ArenaAllocator<8192ul, 8ul>::Allocate(unsigned long, std::nothrow_t const&) /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/mozilla/ArenaAllocator.h:67:12
 1:50.93 GECKO(518898)     #4 0x7f44c5ccf2c7 in Allocate /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/mozilla/ArenaAllocator.h:71:15
 1:50.93 GECKO(518898)     #5 0x7f44c5ccf2c7 in nsPresArena<8192ul, mozilla::ArenaObjectID, 176ul>::Allocate(mozilla::ArenaObjectID, unsigned long) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/base/nsPresArena.cpp:100:16
 1:50.93 GECKO(518898)     #6 0x7f44c670c13a in AllocateByObjectID /NEW-SSD/ASAN-OBJ-DIR/objdir-tb3/dist/include/mozilla/PresShell.h:260:32
 1:50.93 GECKO(518898)     #7 0x7f44c670c13a in operator new /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/painting/FrameLayerBuilder.h:103:39
 1:50.93 GECKO(518898)     #8 0x7f44c670c13a in mozilla::FrameLayerBuilder::StoreDataForFrame(nsPaintedDisplayItem*, mozilla::layers::Layer*, mozilla::LayerState, mozilla::DisplayItemData*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/painting/FrameLayerBuilder.cpp:5504:34
 1:50.93 GECKO(518898)     #9 0x7f44c670de1b in mozilla::FrameLayerBuilder::AddPaintedDisplayItem(mozilla::PaintedLayerData*, mozilla::AssignedDisplayItem&, mozilla::layers::Layer*) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/painting/FrameLayerBuilder.cpp:5370:14
 1:50.93 GECKO(518898)     #10 0x7f44c66dce8d in FinishPaintedLayerData<(lambda at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/painting/FrameLayerBuilder.cpp:3144:52)> /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/painting/FrameLayerBuilder.cpp:3501:20
 1:50.93 GECKO(518898)     #11 0x7f44c66dce8d in mozilla::PaintedLayerDataNode::PopAllPaintedLayerData() /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/painting/FrameLayerBuilder.cpp:3144:23
 1:50.93 GECKO(518898)     #12 0x7f44c66db420 in mozilla::PaintedLayerDataNode::Finish(bool) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/painting/FrameLayerBuilder.cpp:3113:3

   ....  see attached log for full trace ...

The attached log was produced by the following patch.

diff --git a/layout/xul/tree/nsTreeBodyFrame.cpp b/layout/xul/tree/nsTreeBodyFrame.cpp
--- a/layout/xul/tree/nsTreeBodyFrame.cpp
+++ b/layout/xul/tree/nsTreeBodyFrame.cpp
@@ -1593,20 +1593,36 @@ nsresult nsTreeBodyFrame::RowCountChange
   if (aCount == 0 || !mView) return NS_OK;  // Nothing to do.
 
 #ifdef ACCESSIBILITY
   if (PresShell::IsAccessibilityActive()) {
     FireRowCountChangedEvent(aIndex, aCount);
   }
 #endif  // #ifdef ACCESSIBILITY
 
-  // Adjust our selection.
-  nsCOMPtr<nsITreeSelection> sel;
-  mView->GetSelection(getter_AddRefs(sel));
-  if (sel) sel->AdjustSelection(aIndex, aCount);
+  // Validate pointer. Bug 1677194
+  AutoWeakFrame weakFrame(this);
+  nsCOMPtr<nsITreeView> view = mView;
+
+  // Adjust our selection. Note the use of |view| instead of |mView| 
+  nsCOMPtr<nsITreeSelection> sel = nullptr; 
+#if DEBUG
+  printf("RowCountChanged(): view = %p, sel = %p, this = %p\n", (void *) view, (void *) sel, (void *) this);
+  fflush(stdout);
+#endif
+  nsresult rc = view->GetSelection(getter_AddRefs(sel));
+  // Check
+  NS_ENSURE_STATE(weakFrame.IsAlive());
+
+#if DEBUG
+  printf("RowCountChanged(): view = %p, sel = %p, this = %p\n", (void *) view, (void *) sel, (void *) this);
+  fflush(stdout);
+#endif
+
+  if (NS_SUCCEEDED(rc) && sel) sel->AdjustSelection(aIndex, aCount);
 
   if (mUpdateBatchNest) return NS_OK;
 
   mRowCount += aCount;
 #ifdef DEBUG
   int32_t rowCount = mRowCount;
   mView->GetRowCount(&rowCount);
   NS_ASSERTION(

ISHIKAWA, Chiaki

Assignee

Comment 8

•

4 years ago

•

Edited

|this| is a valid pointer as far as weakFrame.IsAlive() check is concerned, I think.
Yet, pointer to (((char*)this) + 0x134) [ = 0x625001200e44.] points at an area already poisoned.

Wait. 0x134 (hex) = 308.
308 / 8 = 38.

According to the memory block status reported by ASAN (each byte signifies 8 bytes of user memory.),
we count back 38 bytes from the problematic location ([f7]).
From the status info, we find that |this| points at an already poisoned area.
weakFrame returns true |IsAlive()|.

Something is amiss here, isn't it?


 1:51.06 GECKO(518898) Shadow bytes around the buggy address:
 1:51.06 GECKO(518898)   0x0c4a80238170: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
 1:51.06 GECKO(518898)   0x0c4a80238180: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
 1:51.06 GECKO(518898)   0x0c4a80238190: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
 1:51.06 GECKO(518898)   0x0c4a802381a0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7  <--- |this| seems to point at somewhere here.
...............................................................xxx
 1:51.06 GECKO(518898)   0x0c4a802381b0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
 1:51.06 GECKO(518898) =>0x0c4a802381c0: f7 f7 f7 f7 f7 f7 f7 f7[f7]f7 f7 f7 f7 f7 f7 f7  <=== invalid read
 1:51.06 GECKO(518898)   0x0c4a802381d0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
 1:51.06 GECKO(518898)   0x0c4a802381e0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
 1:51.06 GECKO(518898)   0x0c4a802381f0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
 1:51.06 GECKO(518898)   0x0c4a80238200: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
 1:51.06 GECKO(518898)   0x0c4a80238210: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
 1:51.06 GECKO(518898) Shadow byte legend (one shadow byte represents 8 application bytes):

Emilio Cobos Álvarez (:emilio)

Comment 9

•

4 years ago

no, your patch is wrong, is the AdjustSelection call what kills this. So you need to NS_ENSURE_STATE(weakFrame.IsAlive()); after that.

ISHIKAWA, Chiaki

Assignee

Comment 10

•

4 years ago

•

Edited

Oh, I see. Let me try again.

Well, though, it looks to me that the incorrect access reported by ASAN already occurs by that time, so
I am not sure what we should do about it.
Aha, now I see it. The problem could also happen at the reference of mUpdateBatchNest. That can be solved by the check.

(If not, the symptom noticed here may have already been caused by some free/poison action without invalidating a pointer value in a a variable (or two?)

ISHIKAWA, Chiaki

Assignee

Comment 11

•

4 years ago

The added check after AdjustSelection() seems to eliminate the issue.
I will re-run the whole xpcshelltest for both mochitest and xpcshelltest of ASAN version of C-C TB to see if this introduces new error or something.

Daniel Veditz [:dveditz]

Comment 12

•

4 years ago

Please clear the sec-low to trigger a re-triage if you find that the freed/poisoned object is anything that is not an nsIFrame.

Keywords: sec-low

ISHIKAWA, Chiaki

Assignee

Updated

•

4 years ago

Assignee: nobody → ishikawa

ISHIKAWA, Chiaki

Assignee

Comment 13

•

4 years ago

(In reply to Daniel Veditz [:dveditz] from comment #12)

Please clear the sec-low to trigger a re-triage if you find that the freed/poisoned object is anything that is not an nsIFrame.

I think the problem was caused by the access to
|mUpdateBatchNest|, i.e. this ->mUpdateBatchNest.

It is a bit hard to figure out exactly where the pointer points at a block since the
ASAN compilation seems to align variables a bit differently from ordinary compilation
to make it easy to detect invalid access to
variables by separating them apart, etc. unless they are the members of an array.
valgrind would have made it easy to verify this, but
unfortunately, mochitest of TB does not run under valgrind currently.

ISHIKAWA, Chiaki

Assignee

Comment 14

•

4 years ago

Attached patch 1677194.patch patch to fix the issue. (obsolete) — Details — Splinter Review

Patch attached.

Push job tested on tryserver for C-C TB build and test with the patch
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=80a6150ff18ea26585282a11f4b25da5c9811613

I am not worried about windows failure because others get busted, too.
Please see the job status under comm-central repos. You will see windows not doing good there.

Attachment #9188712 - Flags: review?(emilio)

Emilio Cobos Álvarez (:emilio)

Comment 15

•

4 years ago

Comment on attachment 9188712 [details] [diff] [review]
1677194.patch patch to fix the issue.

Review of attachment 9188712 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I'd like to see the bits below fixed before granting r+, thanks!

::: layout/xul/tree/nsTreeBodyFrame.cpp
@@ +1597,5 @@
>      FireRowCountChangedEvent(aIndex, aCount);
>    }
>  #endif  // #ifdef ACCESSIBILITY
>  
> +  // Validate pointer. Bug 1677194

I'd just remove the comment. Blame is for this.

@@ +1599,5 @@
>  #endif  // #ifdef ACCESSIBILITY
>  
> +  // Validate pointer. Bug 1677194
> +  AutoWeakFrame weakFrame(this);
> +  nsCOMPtr<nsITreeView> view = mView;

I don't think you need this local fwiw. But anyhow no big deal.

@@ +1602,5 @@
> +  AutoWeakFrame weakFrame(this);
> +  nsCOMPtr<nsITreeView> view = mView;
> +
> +  // Adjust our selection. Note the use of |view| instead of |mView| 
> +  nsCOMPtr<nsITreeSelection> sel = nullptr; 

Trailing space, and useless `= nullptr`.

@@ +1603,5 @@
> +  nsCOMPtr<nsITreeView> view = mView;
> +
> +  // Adjust our selection. Note the use of |view| instead of |mView| 
> +  nsCOMPtr<nsITreeSelection> sel = nullptr; 
> +  nsresult rc = view->GetSelection(getter_AddRefs(sel));

no need to check `rc` (and also should be called `rv`). `sel` will never be non-null if it fails, so the code as it was was fine.

@@ +1607,5 @@
> +  nsresult rc = view->GetSelection(getter_AddRefs(sel));
> +
> +  if (NS_SUCCEEDED(rc) && sel) sel->AdjustSelection(aIndex, aCount);
> +
> +  // Check

Useless comment.

Attachment #9188712 - Flags: review?(emilio)

ISHIKAWA, Chiaki

Assignee

Comment 16

•

4 years ago

Will do.(In reply to Emilio Cobos Álvarez (:emilio) from comment #15)

Comment on attachment 9188712 [details] [diff] [review]
1677194.patch patch to fix the issue.

Review of attachment 9188712 [details] [diff] [review]:

Looks good, but I'd like to see the bits below fixed before granting r+,
thanks!

Will do.

ISHIKAWA, Chiaki

Assignee

Comment 17

•

4 years ago

•

Edited

(In reply to ISHIKAWA, Chiaki from comment #13)

(In reply to Daniel Veditz [:dveditz] from comment #12)

Please clear the sec-low to trigger a re-triage if you find that the freed/poisoned object is anything that is not an nsIFrame.

I think the problem was caused by the access to
|mUpdateBatchNest|, i.e. this ->mUpdateBatchNest.

It is a bit hard to figure out exactly where the pointer points at a block since the
ASAN compilation seems to align variables a bit differently from ordinary compilation
to make it easy to detect invalid access to
variables by separating them apart, etc. unless they are the members of an array.
valgrind would have made it easy to verify this, but
unfortunately, mochitest of TB does not run under valgrind currently.

Silly me, I can simply print out the address of |mUpdateBatchNest| and let the original bug to occur and recorded by ASAN binary, then compare the addresses.
Bingo.
The problematic address is indeed &mUpdateBatchNest.

 1:57.14 GECKO(727918) {streamdebug} closing: outputStream (= 0x606001459288 )->Close() 'before possible Close()' in ChangeFlags  at line 990 of /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsMsgBrkMBoxStore.cpp
 1:57.14 GECKO(727918) {streamdebug} value: outputStream = 0x606001459288 'setting to nullptr' in ChangeFlags  at line 996 of /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsMsgBrkMBoxStore.cpp
 1:57.14 GECKO(727918) &mUpdateBatchNest = 0x6250005745b4
 1:57.14 GECKO(727918) &mUpdateBatchNest = 0x6250018c559c
 1:57.14 GECKO(727918) =================================================================
 1:57.14 GECKO(727918) ==727918==ERROR: AddressSanitizer: use-after-poison on address 0x6250018c559c at pc 0x7f3a7d08c90a bp 0x7fffdacf1c10 sp 0x7fffdacf1c08
 1:57.14 GECKO(727918) READ of size 4 at 0x6250018c559c thread T0
 1:57.20 GECKO(727918) error: address range table at offset 0xd50 has an invalid tuple (length = 0) at offset 0xd60
 1:57.20 GECKO(727918)     #0 0x7f3a7d08c909 in nsTreeBodyFrame::RowCountChanged(int, int) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/layout/xul/tree/nsTreeBodyFrame.cpp:1614:7
 1:57.20 GECKO(727918)     #1 0x7f3a6e92c12b in nsMsgDBView::NoteChange(unsigned int, int, int) /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBView.cpp:5951:16

I am clearing sec-low keyword.

Keywords: sec-low

ISHIKAWA, Chiaki

Assignee

Comment 18

•

4 years ago

Ah, I misundestood the suggestion. I am resetting sec-low flag!

ISHIKAWA, Chiaki

Assignee

Updated

•

4 years ago

Keywords: sec-low

ISHIKAWA, Chiaki

Assignee

Comment 19

•

4 years ago

•

Edited

Attached patch Patch, take two. — Details — Splinter Review

Revsied patch.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #15)

Comment on attachment 9188712 [details] [diff] [review]
1677194.patch patch to fix the issue.

Review of attachment 9188712 [details] [diff] [review]:

Looks good, but I'd like to see the bits below fixed before granting r+,
thanks!

Thank you.

::: layout/xul/tree/nsTreeBodyFrame.cpp
@@ +1597,5 @@
 FireRowCountChangedEvent(aIndex, aCount);
}
#endif // #ifdef ACCESSIBILITY

// Validate pointer. Bug 1677194
I'd just remove the comment. Blame is for this.

Removed. I am from an older generation who needed to dealt with communication break down, or other problems often.
Having the comments IN THE SOURCE FILE was useful. But I removed the comment.

@@ +1599,5 @@

#endif // #ifdef ACCESSIBILITY

// Validate pointer. Bug 1677194

AutoWeakFrame weakFrame(this);

nsCOMPtr<nsITreeView> view = mView;

I don't think you need this local fwiw. But anyhow no big deal.

I just followed the previous comment and so left this as is.
I did not know what I was doing. :-)

@@ +1602,5 @@

AutoWeakFrame weakFrame(this);

nsCOMPtr<nsITreeView> view = mView;

// Adjust our selection. Note the use of |view| instead of |mView|

nsCOMPtr<nsITreeSelection> sel = nullptr;

Trailing space, and useless = nullptr.

Taken care of.

@@ +1603,5 @@

nsCOMPtr<nsITreeView> view = mView;

// Adjust our selection. Note the use of |view| instead of |mView|

nsCOMPtr<nsITreeSelection> sel = nullptr;

nsresult rc = view->GetSelection(getter_AddRefs(sel));

no need to check rc (and also should be called rv). sel will never be
non-null if it fails, so the code as it was was fine.
@@ +1607,5 @@

nsresult rc = view->GetSelection(getter_AddRefs(sel));

if (NS_SUCCEEDED(rc) && sel) sel->AdjustSelection(aIndex, aCount);

Reverted to the original.

// Check

Useless comment.

Took out.

tryserver run:
try-comm-central:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=4a86f4fcf258f3eb6083e0bc76f34eff9f52381a
(I am not worried with windows issues. They are in comm-central runs as well.)

It does not introduce new bugs, and it certainly does NOT trigger the ASAN error anymore.

Attachment #9188712 - Attachment is obsolete: true

Attachment #9188843 - Flags: review?(emilio)

Emilio Cobos Álvarez (:emilio)

Comment 20

•

4 years ago

Comment on attachment 9188843 [details] [diff] [review]
Patch, take two.

Review of attachment 9188843 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

::: layout/xul/tree/nsTreeBodyFrame.cpp
@@ +1600,5 @@
>  
> +  AutoWeakFrame weakFrame(this);
> +  nsCOMPtr<nsITreeView> view = mView;
> +
> +  // Adjust our selection. Note the use of |view| instead of |mView|

I don't think the distinction between `view` and `mView` is relevant, either should work as long as they're kept alive. So the second part of the comment doesn't look terribly useful either, but fine :)

Attachment #9188843 - Flags: review?(emilio) → review+

ISHIKAWA, Chiaki

Assignee

Comment 21

•

4 years ago

Thank you.

As you may have realized, I submit TB patches now and then, but I have not done M-C patch submission using the latest submission tool.
Will someone take care of the landing the patch?
Or I can possibly do it, but I am not sure where to start.

Flags: needinfo?(emilio)

Emilio Cobos Álvarez (:emilio)

Comment 22

•

4 years ago

Landed manually: https://hg.mozilla.org/integration/autoland/rev/d467ba314d3c3c167b3e32864dd601f523b868ff

The official workflow uses phabricator, see https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html#to-submit-a-patch

Flags: needinfo?(emilio)

ISHIKAWA, Chiaki

Assignee

Comment 23

•

4 years ago

(In reply to Emilio Cobos Álvarez (:emilio) from comment #22)

Landed manually: https://hg.mozilla.org/integration/autoland/rev/d467ba314d3c3c167b3e32864dd601f523b868ff

The official workflow uses phabricator, see https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html#to-submit-a-patch

Thank you.

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 24

•

4 years ago

https://hg.mozilla.org/mozilla-central/rev/d467ba314d3c

Group: layout-core-security → core-security-release

Status: NEW → RESOLVED

Closed: 4 years ago

status-firefox85: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → 85 Branch

Ryan VanderMeulen [:RyanVM]

Updated

•

3 years ago

status-firefox83: --- → wontfix

status-firefox-esr78: --- → wontfix

Brindusa Tot, Desktop QA

Updated

•

3 years ago

Whiteboard: [post-critsmash-triage]

Brindusa Tot, Desktop QA

Updated

•

3 years ago

Flags: qe-verify-

Ryan VanderMeulen [:RyanVM]

Updated

•

3 years ago

status-firefox84: affected → wontfix

Frederik Braun [:freddy]

Updated

•

3 years ago

Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main85+r]

Frederik Braun [:freddy]

Updated

•

3 years ago

Whiteboard: [post-critsmash-triage][adv-main85+r] → [post-critsmash-triage][adv-main85+]

Frederik Braun [:freddy]

Comment 25

•

3 years ago

Attached file advisory.txt — Details

Frederik Braun [:freddy]

Updated

•

3 years ago

Alias: CVE-2021-23962

Daniel Veditz [:dveditz]

Updated

•

2 years ago

Group: core-security-release

ASAN error log for use-after-poison 4 years ago ISHIKAWA, Chiaki 17.81 KB, text/plain		Details
verbose log. 4 years ago ISHIKAWA, Chiaki 20.32 KB, text/plain		Details
1677194.patch patch to fix the issue. 4 years ago ISHIKAWA, Chiaki 1.33 KB, patch		Details \| Diff \| Splinter Review
Patch, take two. 4 years ago ISHIKAWA, Chiaki 1.18 KB, patch	emilio : review+	Details \| Diff \| Splinter Review
advisory.txt 3 years ago Frederik Braun [:freddy] 217 bytes, text/plain		Details