1592880 - nsMsgDBView::RestoreSelection() accesses uninitialized variable (and passes further)

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[ISHIKAWA, Chiaki]

During the run of |mach xpcshell-tests| under valgrind, I noticed a flurry of uninitialized memory access errors that has the sequence of

nsMsgThreadedDEBView::Sort() -> nsMsgDBView::RestoreSelection()

as in

 8:14.56 pid:10840 ==10840== Conditional jump or move depends on uninitialised value(s)
 8:14.57 pid:10840 ==10840==    at 0x56A1C19: nsMsgDBView::RestoreSelection(unsigned int, nsTArray<unsigned int>&) (nsMsgDBView.cpp:906)
 8:14.57 pid:10840 ==10840==    by 0x57046CD: nsMsgThreadedDBView::Sort(int, int) (nsMsgThreadedDBView.cpp:373)
 8:14.57 pid:10840 ==10840==    by 0x5CF93F9: ??? (xptcinvoke_asm_x86_64_unix.S:106)
 8:14.57 pid:10840 ==10840==    by 0x6B63672: 
        ...

or

 8:14.60 pid:10840 ==10840== Conditional jump or move depends on uninitialised value(s)
 8:14.60 pid:10840 ==10840==    at 0x56B4FCF: nsMsgDBView::FindKey(unsigned int, bool) (nsTArray.h:1207)
 8:14.60 pid:10840 ==10840==    by 0x56A1C2E: nsMsgDBView::RestoreSelection(unsigned int, nsTArray<unsigned int>&) (nsMsgDBView.cpp:907)
 8:14.60 pid:10840 ==10840==    by 0x57046CD: nsMsgThreadedDBView::Sort(int, int) (nsMsgThreadedDBView.cpp:373)
 8:14.60 pid:10840 ==10840==    by 0x5CF93F9: ??? (xptcinvoke_asm_x86_64_unix.S:106)
   ... 

or

 jump or move depends on uninitialised value(s)
 8:14.73 pid:10840 ==10840==    at 0x5C14276: PLDHashTable::Search(void const*) const (PLDHashTable.cpp:493)
 8:14.73 pid:10840 ==10840==    by 0x58710BA: nsMsgDatabase::GetHdrFromUseCache(unsigned int, nsIMsgDBHdr**) (nsMsgDatabase.cpp:625)
 8:14.73 pid:10840 ==10840==    by 0x5871256: nsMsgDatabase::GetMsgHdrForKey(unsigned int, nsIMsgDBHdr**) (nsMsgDatabase.cpp:1724)
 8:14.73 pid:10840 ==10840==    by 0x56AC6AF: nsMsgDBView::GetKeyOfFirstMsgInThread(unsigned int) (nsMsgDBView.cpp:4703)
 8:14.73 pid:10840 ==10840==    by 0x56B508B: nsMsgDBView::FindKey(unsigned int, bool) (nsMsgDBView.cpp:4781)
 8:14.73 pid:10840 ==10840==    by 0x56A1C2E: nsMsgDBView::RestoreSelection(unsigned int, nsTArray<unsigned int>&) (nsMsgDBView.cpp:907)
 8:14.73 pid:10840 ==10840==    by 0x57046CD: nsMsgThreadedDBView::Sort(int, int) (nsMsgThreadedDBView.cpp:373)
 8:14.73 pid:10840 ==10840==    by 0x5CF93F9: ??? (xptcinvoke_asm_x86_64_unix.S:106)
    ...

Basically sort() creates uninitialized variable and passes it to the first argument of RestoreSelection().

Comment 1

[ISHIKAWA, Chiaki]

The Sort in nsMsgThreadedDEBView allocate |nsMsgKey preservedKey| as auto variable on the stack
without initialization.

The code skelton of Sort().

NS_IMETHODIMP
nsMsgThreadedDBView::Sort(nsMsgViewSortTypeValue sortType,
                          nsMsgViewSortOrderValue sortOrder) {
  nsresult rv;

  int32_t rowCountBeforeSort = GetSize();

  ... omission ....

  nsMsgKey preservedKey;     <--- 
  AutoTArray<nsMsgKey, 1> preservedSelection;
  SaveAndClearSelection(&preservedKey, preservedSelection); <---

  ... then later on it invokes the following in a few places.
  RestoreSelection(preservedKey, preservedSelection)

  return rv;
}

The only way |preservedKey| gets initalized is through the call to |SaveAndClearSelection(&preservedKey, preservedSelection)|.

Problem is that |SaveAndClearSelection(&preservedKey, preservedSelection)| does not always set the value to the first pointer location (!)

// If you call SaveAndClearSelection make sure to call RestoreSelection(),
// otherwise m_saveRestoreSelectionDepth will be incorrect and will lead to
// selection msg problems.
nsresult nsMsgDBView::SaveAndClearSelection(nsMsgKey *aCurrentMsgKey,
                                            nsTArray<nsMsgKey> &aMsgKeyArray) {
  // We don't do anything on nested Save / Restore calls.
  m_saveRestoreSelectionDepth++;
  if (m_saveRestoreSelectionDepth != 1) return NS_OK;

  if (!mTreeSelection || !mTree) return NS_OK;

  // First, freeze selection.
  mTreeSelection->SetSelectEventsSuppressed(true);

  // Second, save the current index.
  if (aCurrentMsgKey) {
    int32_t currentIndex;
    if (NS_SUCCEEDED(mTreeSelection->GetCurrentIndex(&currentIndex)) &&
        currentIndex >= 0 && uint32_t(currentIndex) < GetSize())
      *aCurrentMsgKey = m_keys[currentIndex];
    else
      *aCurrentMsgKey = nsMsgKey_None;
  }

  ... omission ...

  return NS_OK;
}

You can see that SaveAndClearSelectin() returns early without setting a value to |*aCurrentMsgKey|.

Thus, the preservedKey allocated in nsMsgThreadedDBView::Sort() never gets initialized and used in other functions.

Comment 2

[ISHIKAWA, Chiaki]

I think the intention of SaveAndClearSelection() probably returns *aCurrentMsgKey = nsMsgKey_None when early return is taken.

Let me try a patch.

Comment 3

[ISHIKAWA, Chiaki]

Attachment: Always return a value in the first pointer argument of SaveAndClearSelection().

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 9105461 [details] [diff] [review]
Always return a value in the first pointer argument of  SaveAndClearSelection().

Looks OK to me, perhaps Alta88 can confirm.

Comment 5

[ISHIKAWA, Chiaki]

(In reply to Jorg K (GMT+2) from comment #4)

Comment on attachment 9105461 [details] [diff] [review]
Always return a value in the first pointer argument of
SaveAndClearSelection().

Looks OK to me, perhaps Alta88 can confirm.

Thank you. The intention is to figure out if there is a focused message or something and so if we are not interested, returning
nsMsgKey_None seems prudent.

I have a feeling that this could be ONE of the reasons of strange/erratic/unreproducible selection behaviors observed over the years.

TIA

Comment 6

[ISHIKAWA, Chiaki]

Attachment: Always return a value in the first pointer argument of SaveAndClearSelection() if the pointer is not nullptr (take-2).

Updated patch.

I noticed a somewhat messy state of affairs.

(1). nsMsgDBView::SelectMsgByKey() calls SaveAndClearSelection with nullptr in the first
argument (!)

So we have to wrap the change in if statement.

Below are some other call sites to SaveAndClearSelection().

SaveAndClearSelection() never fails.:

nsMsgDBView::SelectMsgByKey() [case 1 below] returns
when SaveAndClearSelection() fails.
EXCEPT the current code of SaveAndClearSelection() always return
NS_OK (!?) It NEVER fails. Fun...

---

case - 1:

nsMsgDBView::SelectMsgByKey(nsMsgKey aKey) - very similar to
nsMsgSearchDBView::Sort().
But note that the processing returns when SaveAndClearSelection()
fails. However, SaveAndClearSelection() never fails.

I think the NS_ENSURE_SUCCESS() and checking of the value is bogus.
OR the current implementaiton of SaveAndClearSelection() is bogus in
that it never returns failure. Hard to tell.
Majority rules and I think the extra check in SelectMsgByKey() is bogus.

nsMsgDBView::SelectMsgByKey(nsMsgKey aKey) {
  NS_ASSERTION(aKey != nsMsgKey_None, "bad key");
  if (aKey == nsMsgKey_None) return NS_OK;

  // Use SaveAndClearSelection()
  // and RestoreSelection() so that we'll clear the current selection
  // but pass in a different key array so that we'll
  // select (and load) the desired message.

  AutoTArray<nsMsgKey, 1> preservedSelection;
  nsresult rv = SaveAndClearSelection(nullptr, preservedSelection);
  NS_ENSURE_SUCCESS(rv, rv);

---

case - 2:

nsMsgGroupView::RebuildView()

// Or, if we're switching between grouping and threading in a cross-folder
// saved search. In that case, we needed to build an enumerator based on the
// old view type, and internally close the view based on its old type, but
// rebuild the new view based on the new view type. So we pass the new
// view flags to OpenWithHdrs.
nsresult nsMsgGroupView::RebuildView(nsMsgViewFlagsTypeValue newFlags) {
  nsCOMPtr<nsISimpleEnumerator> headers;
  if (NS_SUCCEEDED(GetMessageEnumerator(getter_AddRefs(headers)))) {
    int32_t count;
    m_dayChanged = false;
    AutoTArray<nsMsgKey, 1> preservedSelection;
    nsMsgKey curSelectedKey;
    SaveAndClearSelection(&curSelectedKey, preservedSelection);
    InternalClose();
    int32_t oldSize = GetSize();
     ....

---

case - 3:

nsMsgSearchDBView::MoveThreadAt()

This function takes care that there is indeed a selection to start
with. Only when there is a selection (and a focused message),
SaveAndClearSelection() is called.

Obtained |preservedKey| is also reused only when there was a selection.

I wonder if other callers of SaveAndClearSelection() need to be as
careful.
(Come to think of it, this function needs to RESTORE the selection if there is
any. So I understand it has to be careful.)

// This method removes the thread at threadIndex from the view
// and puts it back in its new position, determined by the sort order.
// And, if the selection is affected, save and restore the selection.
void nsMsgSearchDBView::MoveThreadAt(nsMsgViewIndex threadIndex) {
  bool updatesSuppressed = mSuppressChangeNotification;
  // Turn off tree notifications so that we don't reload the current message.
  if (!updatesSuppressed) SetSuppressChangeNotifications(true);

  nsCOMPtr<nsIMsgDBHdr> threadHdr;
  GetMsgHdrForViewIndex(threadIndex, getter_AddRefs(threadHdr));

  uint32_t saveFlags = m_flags[threadIndex];
  bool threadIsExpanded = !(saveFlags & nsMsgMessageFlags::Elided);
  int32_t childCount = 0;
  nsMsgKey preservedKey;
  AutoTArray<nsMsgKey, 1> preservedSelection;
  int32_t selectionCount;
  int32_t currentIndex;
  bool hasSelection =
      mTree && mTreeSelection &&
      ((NS_SUCCEEDED(mTreeSelection->GetCurrentIndex(&currentIndex)) &&
        currentIndex >= 0 && (uint32_t)currentIndex < GetSize()) ||
       (NS_SUCCEEDED(mTreeSelection->GetRangeCount(&selectionCount)) &&
        selectionCount > 0));
  if (hasSelection) SaveAndClearSelection(&preservedKey, preservedSelection);

  ... omission ...

 // Unfreeze selection.
  if (hasSelection) RestoreSelection(preservedKey, preservedSelection);

---

case -4 void nsMsgThreadedDBView::MoveThreadAt()

Very similar to case 3 above.
It checks if there was a selection and only
calls

// This method removes the thread at threadIndex from the view
// and puts it back in its new position, determined by the sort order.
// And, if the selection is affected, save and restore the selection.
void nsMsgThreadedDBView::MoveThreadAt(nsMsgViewIndex threadIndex) {
  // We need to check if the thread is collapsed or not...
  // We want to turn off tree notifications so that we don't
  // reload the current message.
  // We also need to invalidate the range between where the thread was
  // and where it ended up.
  bool changesDisabled = mSuppressChangeNotification;
  if (!changesDisabled) SetSuppressChangeNotifications(true);

  nsCOMPtr<nsIMsgDBHdr> threadHdr;

  GetMsgHdrForViewIndex(threadIndex, getter_AddRefs(threadHdr));
  int32_t childCount = 0;

  nsMsgKey preservedKey;
  AutoTArray<nsMsgKey, 1> preservedSelection;
  int32_t selectionCount;
  int32_t currentIndex;
  bool hasSelection =
      mTree && mTreeSelection &&
      ((NS_SUCCEEDED(mTreeSelection->GetCurrentIndex(&currentIndex)) &&
        currentIndex >= 0 && (uint32_t)currentIndex < GetSize()) ||
       (NS_SUCCEEDED(mTreeSelection->GetRangeCount(&selectionCount)) &&
        selectionCount > 0));
  if (hasSelection) SaveAndClearSelection(&preservedKey, preservedSelection);

  ... omission ...

  // Unfreeze selection.
  if (hasSelection) RestoreSelection(preservedKey, preservedSelection);

---

case - 5
nsMsgThreadedDBView::Sort()

This function takes it easy and never bothers to check if there was a
selection already before calling SaveAndClearSelection() and then
later calls RestoreSelection(). If this loose handling is allowed,
then extra checking in nsMsgSearchDBView::MoveThreadAt() is
superflous. Or is it?

NS_IMETHODIMP
nsMsgThreadedDBView::Sort(nsMsgViewSortTypeValue sortType,
                          nsMsgViewSortOrderValue sortOrder) {
  nsresult rv;

  int32_t rowCountBeforeSort = GetSize();

  if (!rowCountBeforeSort) {
    // Still need to setup our flags even when no articles - bug 98183.
    m_sortType = sortType;
    if (sortType == nsMsgViewSortType::byThread &&
        !(m_viewFlags & nsMsgViewFlagsType::kThreadedDisplay)) {
      SetViewFlags(m_viewFlags | nsMsgViewFlagsType::kThreadedDisplay);
    }

    SaveSortInfo(sortType, sortOrder);
    return NS_OK;
  }

  if (!m_checkedCustomColumns && CustomColumnsInSortAndNotRegistered())
    return NS_OK;

  // Sort threads by sort order.
  bool sortThreads = m_viewFlags & (nsMsgViewFlagsType::kThreadedDisplay |
                                    nsMsgViewFlagsType::kGroupBySort);

  // If sort type is by thread, and we're already threaded, change sort type
  // to byId.
  if (sortType == nsMsgViewSortType::byThread &&
      (m_viewFlags & nsMsgViewFlagsType::kThreadedDisplay) != 0) {
    sortType = nsMsgViewSortType::byId;
  }

  nsMsgKey preservedKey;
  AutoTArray<nsMsgKey, 1> preservedSelection;
  SaveAndClearSelection(&preservedKey, preservedSelection);
  // If the client wants us to forget our cached id arrays, they
  // should build a new view. If this isn't good enough, we
  // need a method to do that.
  if (sortType != m_sortType || !m_sortValid || sortThreads) {
    SaveSortInfo(sortType, sortOrder);
    if (sortType == nsMsgViewSortType::byThread) {
      m_sortType = sortType;
      m_viewFlags |= nsMsgViewFlagsType::kThreadedDisplay;
      m_viewFlags &= ~nsMsgViewFlagsType::kGroupBySort;
      if (m_havePrevView) {
        // Restore saved id array and flags array.
        m_keys = m_prevKeys;
        m_flags = m_prevFlags;
        m_levels = m_prevLevels;
        m_sortValid = true;

        // The sort may have changed the number of rows
        // before we restore the selection, tell the tree
        // do this before we call restore selection
        // this is safe when there is no selection.
        rv = AdjustRowCount(rowCountBeforeSort, GetSize());

        RestoreSelection(preservedKey, preservedSelection);
        if (mTree) mTree->Invalidate();
        ... omission ...

---

Comment 7

[ISHIKAWA, Chiaki]

Oops wrong patch.

Comment 8

[ISHIKAWA, Chiaki]

Attachment: Always return a value in the first pointer argument of SaveAndClearSelection() if the pointer is not nullptr (take-2).

Correct patch.

Can you kindly review this?

Comment 9

[alta88]

Comment on attachment 9105519 [details] [diff] [review]
Always return a value in the first pointer argument of SaveAndClearSelection() if the pointer is not nullptr (take-2).

It seems if the uninitialized storage has some dirty bits then the test in SaveAndClearSelection() would pass, so it would indeed be random and difficult to reproduce behavior, but likely the warnings are the real issue. Thanks for the patch.

As far as incorrect selection, the problem is much bigger. For multifolder saved searches and the search view, using msgKey is a fail due to duplicate keys cross folder. There are bugs/patches (I even had one long ago) to remove anything to do with selection or find by key. In multifolder views, the msgHdr is used (which includes the unique folder), but it's not consistent for every operation. This failure to reselect the right selection is easily reproducible in a saved search folder - make sure to have 2 folders, select one message with a key (order received) that dupes another message, sort a bit, and see the selection restored to the wrong message.

Anyway, there isn't any interest in devoting resources to these views, and nsITree is "going away".

Comment 10

[Jorg K (CEST = GMT+2)]

I'll fix the comment a bit before landing. It's not "variable" but "parameter" or "argument".

Comment 11

[ISHIKAWA, Chiaki]

(In reply to alta88 from comment #9)

Comment on attachment 9105519 [details] [diff] [review]
Always return a value in the first pointer argument of
SaveAndClearSelection() if the pointer is not nullptr (take-2).

It seems if the uninitialized storage has some dirty bits then the test in
SaveAndClearSelection() would pass, so it would indeed be random and
difficult to reproduce behavior, but likely the warnings are the real issue.
Thanks for the patch.

As far as incorrect selection, the problem is much bigger. For multifolder
saved searches and the search view, using msgKey is a fail due to duplicate
keys cross folder. There are bugs/patches (I even had one long ago) to
remove anything to do with selection or find by key. In multifolder views,
the msgHdr is used (which includes the unique folder), but it's not
consistent for every operation. This failure to reselect the right selection
is easily reproducible in a saved search folder - make sure to have 2
folders, select one message with a key (order received) that dupes another
message, sort a bit, and see the selection restored to the wrong message.

Anyway, there isn't any interest in devoting resources to these views, and
nsITree is "going away".

Thank you for the detailed explanation. I did not realize such problems existed.

(In reply to Jorg K (GMT+2) from comment #10)

I'll fix the comment a bit before landing. It's not "variable" but "parameter" or "argument".

Right. Thank you for taking care of the fix.

Comment 12

[ISHIKAWA, Chiaki]

I obsoleted the incorrect patch which I uploaded by mistake to avoid confusion.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/faa5b18414e5
always return a value in the first parameter of nsMsgDBView::SaveAndClearSelection(). r=alta88 DONTBUILD

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9c3fdd4aaf21
Follow-up: Reformat. rs=reformat DONTBUILD