(Coverity) Fix misc. Coverity reported problems in nsMsgSearchAdapter.cpp/h

RESOLVED FIXED in Thunderbird 53.0

Status

MailNews Core
Search
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

({coverity})

Trunk
Thunderbird 53.0
coverity

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: CID 1396366, CID 1396363)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 months ago
Patch coming.
(Assignee)

Comment 1

7 months ago
Created attachment 8816937 [details] [diff] [review]
1322182-Coverity-nsMsgSearchAdapter.patch

The Coverity report complains about m_forceAsciiSearch and it's pretty clear why.

The same Coverity report also complains that m_ORSearch is not initialised in the constructor nsMsgSearchNews::nsMsgSearchNews in nsMsgSearchNews.cpp

Now, class nsMsgSearchNews has a few protected members:
https://dxr.mozilla.org/comm-central/rev/85dd34e18d173d05872889f4b9bb3691b61be6f5/mailnews/base/search/src/nsMsgSearchNews.h#36
  nsCString m_encoding;
  bool m_ORSearch; // set to true if any of the search terms contains an OR for a boolean operator.

  nsTArray<nsMsgKey> m_candidateHits;
  nsTArray<nsMsgKey> m_hits;
which are happily used in nsMsgSearchNews.cpp without ever being initialised, just look at m_candidateHits:
https://dxr.mozilla.org/comm-central/search?q=m_candidateHits&redirect=false
Or is there a default constructor at work which initialises an empty array. Maybe that's it. And the same goes for nsCString.

So perhaps the fix is just to set m_ORSearch to false in the constructor.
Attachment #8816937 - Flags: review?(acelists)
excllent. You'll want to cite the ID(s) in whiteboard - see bug 1285101 for an example
Keywords: coverity
(Assignee)

Updated

7 months ago
Whiteboard: CID 1396366, CID 1396363

Comment 3

7 months ago
Comment on attachment 8816937 [details] [diff] [review]
1322182-Coverity-nsMsgSearchAdapter.patch

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

::: mailnews/base/search/src/nsMsgSearchNews.cpp
@@ +34,5 @@
>  
>  
>  nsMsgSearchNews::nsMsgSearchNews (nsMsgSearchScopeTerm *scope, nsIArray *termList) : nsMsgSearchAdapter (scope, termList)
>  {
> +  m_ORSearch = false;

Well, can we pretend the default is false? I would rather fix the place where the member value is determined (https://dxr.mozilla.org/comm-central/rev/8881e860371794a64312b206e086ec10f7f9a827/mailnews/base/search/src/nsMsgSearchNews.cpp#235) and make it initialize the variable in all cases. Why would there be a situation when we don't know? For any list of terms we must know if they are OR or AND, no undefined.
(Assignee)

Comment 4

7 months ago
OK, you want to initialise it in nsMsgSearchNews::Encode().

How do I know this runs before it's read in nsMsgSearchNews::CollateHits()?
https://dxr.mozilla.org/comm-central/search?q=m_ORSearch&redirect=false

Comment 5

7 months ago
(In reply to Jorg K (GMT+1) from comment #4)
> OK, you want to initialise it in nsMsgSearchNews::Encode().

Yes.

> How do I know this runs before it's read in nsMsgSearchNews::CollateHits()?

I don't know if you can be sure.
Only logically we should first have the search terms encoded/analyses before we run them and collect hits. But you do not solve the problem by initializing it in the constructor. You pretend the value is false even if you can't know that as you didn't analyze the terms. You would be lying to CollateHits, which I do not like. The proposal makes coverity happy but introduces a logic bug.

Maybe you could check if m_encoding is non-empty at the start of CollateHits().
(Assignee)

Comment 6

6 months ago
Created attachment 8818090 [details] [diff] [review]
1322182-Coverity-nsMsgSearchAdapter.patch (v2).

There is no honest way of tracking whether a boolean has been set or not. So I'm making this an enum.
Attachment #8816937 - Attachment is obsolete: true
Attachment #8816937 - Flags: review?(acelists)
Attachment #8818090 - Flags: review?(acelists)

Comment 7

6 months ago
Jcranmer, would you have an opinion here?
Flags: needinfo?(Pidgeot18)
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Comment 8

6 months ago
(In reply to :aceman from comment #5)
> Maybe you could check if m_encoding is non-empty at the start of
> CollateHits().
Why? How are m_encoding and m_ORSearch correlated? m_encoding is never used in this code, it's only returned by nsMsgSearchNews::GetEncoding(), most likely to some JS caller.

My solution tracks the variable in question by initialising it to a defined recognisable value, and when we come to read it later and still find the initial value, we know that the logic is wrong. We even crash in debug mode on the MOZ_ASSERT(). What else could you want?

Given that the code seems to be working now, we shouldn't lose any sleep on this issue reported by Coverity which can't tell what happens at runtime.

obj::func1 () {
  m_member = 1;
}

obj::func2 () {
  if (m_member)
    print "huhu";
}

Coverity will complain here, since it doesn't know that we always call function 1 before function 2. That's why it insists on proper initialisation in the constructor. Most of the "fancy" types (arrays, strings, etc.) will just initialise with their constructor, but scalar types don't. Try to track |nsTArray<nsMsgKey> m_candidateHits;|. Also quite impossible, but at least it's properly initialised ;-)
(Assignee)

Comment 9

6 months ago
Comment on attachment 8818090 [details] [diff] [review]
1322182-Coverity-nsMsgSearchAdapter.patch (v2).

Let's not wait until Christmas 2078.
Flags: needinfo?(Pidgeot18)
Attachment #8818090 - Flags: review?(rkent)
(Assignee)

Comment 10

6 months ago
Comment on attachment 8818090 [details] [diff] [review]
1322182-Coverity-nsMsgSearchAdapter.patch (v2).

I can't convince Aceman and I don't have more time to spend on this minor issue.
Attachment #8818090 - Flags: review?(acelists) → review?(mkmelin+mozilla)

Comment 11

6 months ago
Comment on attachment 8818090 [details] [diff] [review]
1322182-Coverity-nsMsgSearchAdapter.patch (v2).

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

r+=me with suggested changes. Generally though I agree that the right approach is minimal effort here.

::: mailnews/base/search/public/nsMsgSearchAdapter.h
@@ +36,5 @@
>  
>    nsIMsgSearchScopeTerm        *m_scope;
>    nsCOMPtr<nsIArray>  m_searchTerms;       /* linked list of criteria terms */
>  
>    bool m_abortCalled;

The CID also complained that m_abortCalled is uninitialized. AFAICT this is unused, so please remove this unless you disagree with my analysis.

::: mailnews/base/search/src/nsMsgSearchNews.cpp
@@ +321,5 @@
>    m_candidateHits.Sort();
>  
>    // For an OR search we only need to count the first occurrence of a candidate.
>    uint32_t termCount = 1;
> +  MOZ_ASSERT(m_searchType != uninitialised, "m_searchType accessed without being set");

MOZ_ASSERT to me means either "The program cannot reasonably continue so we must abort" or "This issue is so important that we intend to annoy all developers with crashes until it is fixed." This issue does not meet either of these criteria, so please change this to a warning instead with a default value of false. Yes we will ignore it, but that is because we are working on more important issues. I tend to agree that we should not put much effort into this at the moment.

::: mailnews/base/search/src/nsMsgSearchNews.h
@@ +8,5 @@
>  #include "MailNewsTypes.h"
>  #include "nsTArray.h"
>  
> +typedef enum search_type {
> +  uninitialised,

enums become effectively #defines in the program, so I tend to follow define-like naming. That is, I would abbreviate the enum type as ST, and use caps like UNINITIALIZED. So this first term is ST_UNINITIALIZED, others are ST_OR_SEARCH and ST_AND_SEARCH. If you can agree with this, please rename accordingly.
Attachment #8818090 - Flags: review?(rkent) → review+
(Assignee)

Comment 12

6 months ago
Thanks for the review.

(In reply to Kent James (:rkent) from comment #11)
> > +  MOZ_ASSERT(m_searchType != uninitialised, "m_searchType accessed without being set");
> MOZ_ASSERT to me means either "The program cannot reasonably continue so we
> must abort" or "This issue is so important that we intend to annoy all
> developers with crashes until it is fixed." This issue does not meet either
> of these criteria, so please change this to a warning instead with a default
> value of false. Yes we will ignore it, but that is because we are working on
> more important issues.
I don't really agree here. If the search type in uninitialised, that's a logic error we should detect.

We will not annoy any developer, since this code only runs on demand, so it doesn't crash if the developer is not working in this area. This is much different to the recent IMAP thread crash where a debug-TB just crashed spontaneously if an IMAP account was included in the test profile.

Can I please leave the MOZ_ASSERT()? First time you crash there, I'll fix it.
Flags: needinfo?(rkent)

Comment 13

6 months ago
I have my personal opinion of MOZ_ASSERT but I don't think it is shared widely, and I don't feel I have the authority to force my opinion here.

Bienvenu used to insert everywhere #ifdef DEBUG_DavidBienvenu for code that he wanted to include in his debug builds. I would have no objection to you including such a qualifier in code with your name on it if you want to crash here. If you want an ASSERT when you run, feel free to start including a #define in your builds, and landing code with your name on it like he did. Maybe that would be a compromise?

I suppose a third category of ASSERT would be "This error is important but really hard to trace out, so if any developer experiences it I'm going to force their cooperation in debugging it." Are you claiming this here?

But as I said, I will not force my will. We can agree to disagree and you can land with the ASSERT if it is important to you, over my objections but with my r+
Flags: needinfo?(rkent)
(Assignee)

Comment 14

6 months ago
I think that this will never assert since our logic should be right. Otherwise there would be wrong search results, left, right and centre.

Should I be wrong, I'd like to hear about it ;-) And I will. Any developer annoyed by this can just comment this out if it really affects them.

I'll land it with the other issues addressed once I get off my current bug. Thanks again.
(Assignee)

Comment 15

6 months ago
Created attachment 8818642 [details] [diff] [review]
1322182-Coverity-nsMsgSearchAdapter.patch (v2b)

Fixed review issues apart from the MOZ_ASSERT().
Assignee: nobody → jorgk
Attachment #8818090 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8818090 - Flags: review?(mkmelin+mozilla)
Attachment #8818642 - Flags: review+
(Assignee)

Comment 16

6 months ago
https://hg.mozilla.org/comm-central/rev/30f5aad102e4decb52bc3fedf6da6ebcca9fc79d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
You need to log in before you can comment on or make changes to this bug.