454298 - infinite recursion in GetIsKilled

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[David :Bienvenu]

If the threading structure of a message is messed up, GetIsKilled can recurse until the stack overflows. In particular, if a is the parent of b, and b is the parent of a, then we will recurse forever. In general, GetIsKilled doesn't deal with bogus parenting. I will fix the cause of the broken parenting, but we need to be tolerant of it, for existing db's. The test case in bug 367896 exhibits this problem. I'll attach a patch that basically just counts how deeply we've recursed and errors out if we've gone deeper than the number of children in the thread.

Comment 1

[David :Bienvenu]

Attachment: proposed fix for crash

What do you think of this approach, jcranmer?

Comment 2

[Joshua Cranmer [:jcranmer]]

Comment on attachment 337543 [details] [diff] [review]
proposed fix for crash

>diff --git a/mailnews/db/msgdb/src/nsMsgHdr.cpp b/mailnews/db/msgdb/src/nsMsgHdr.cpp
>+          PRUint32 numChildren;
>+          thread->GetNumChildren(&numChildren);
>+          if (numChildren < depth)

I think a comment explaining why the check is being made would be helpful here; it wasn't immediately obvious to me at first why numChildren and depth should be related (I read the patch before the comments on the bug).

>+NS_IMETHODIMP nsMsgHdr::GetIsKilled(PRBool *isKilled)
>+{
>+  NS_ENSURE_ARG_POINTER(isKilled);
>+  *isKilled = IsAncestorKilled(0);
>   return NS_OK;
> }

It shouldn't matter, but I think 1 is the proper parameter here, since the header being queried is in fact the parent of the current header. That should cause failure if the current thread has 1 child and the header thinks it has a parent. In any case, all that happens is but a single extra call, so it's no big deal.

Comment 3

[David :Bienvenu]

Attachment: fix addressing comments

carrying forward jcranmer's review (thx, jcranmer), requesting sr.

Comment 4

[[e-mail]]

Comment on attachment 337566 [details] [diff] [review]
fix addressing comments

>         (void) m_mdb->GetThreadContainingMsgHdr(this, getter_AddRefs(thread));
>         if (thread)
>         {
>           nsCOMPtr<nsIMsgDBHdr> claimant;
>           (void) thread->GetChild(threadParent, getter_AddRefs(claimant));
>           if (!claimant)
>           {
>             NS_ASSERTION(PR_FALSE, "Borked message header, please fix!");
>-            return NS_OK;
>+            return PR_FALSE;
>+          }
>+          PRUint32 numChildren;
>+          thread->GetNumChildren(&numChildren);
>+
>+          // we can't have as many ancestors as there are messages 
>+          // in the thread, so if that seems to be the case, error out.
>+          if (depth >= numChildren)
>+          {
>+            NS_ASSERTION(PR_FALSE, "Borked thread, please fix!");
NS_ERROR

Rather than getting the number of children every time, why not get it once in GetIsKilled and count down to zero? (If you do this I would also suggest checking the depth earlier.)

Comment 5

[David :Bienvenu]

that's a good idea - I'll redo this.

Comment 6

[David :Bienvenu]

Attachment: get thread count only once

I didn't move the check for ancestors because it's only an error when we're about to recurse - otherwise, I'd have had to pass in numChildren instead of numChildren - 1 as the number of ancestors to check.

Comment 7

[[e-mail]]

Comment on attachment 337881 [details] [diff] [review]
get thread count only once

>+  if (!isKilled)
>   {
>     nsMsgKey threadParent;
>     GetThreadParent(&threadParent);
My point is that we're already thinking of recursing here but it's up to you exactly where you'd like to put the check.

>+          NS_ASSERTION(PR_FALSE, "cycle in parent relationship, please fix!");
NS_ERROR

Comment 8

[David :Bienvenu]

right, we're thinking of recursing, but the case where we don't recurse is ok, so in order to prevent false assertions, I need to put the assertion right before we'll actually recurse.

I've switched it to NS_ERROR

changeset:   305:c2932fcc8375