Info

A new defect has been detected and assigned to acm in Coverity Connect http://coverity.mongodb.com//sourcebrowser.htm?projectId=10001#mergedDefectId=15708 The defect was flagged by checker FORWARD_NULL in file /src/mongo/db/ops/update.cpp function mongo::update(const mongo::UpdateRequest &, mongo::OpDebug *, mongo::UpdateDriver *) and this ticket was created by matt.kangas@10gen.com

Top User Comments

acm commented on Tue, 12 Nov 2013 00:48:16 +0000: Asya has changed the coverity settings to prune branches based on dasserts, which seems to have made this false positive go away. Marking as resolved. acm commented on Mon, 11 Nov 2013 12:45:03 +0000: Asya - If coverity is doing some level of inter-procedural analysis, it may be the case that it is able to prove that the same conditions that give rise to a NULL collection pointer prevent entering the loop body, even without an explicit dassert in this file. acm commented on Sun, 10 Nov 2013 23:29:31 +0000: For 2.2 and 2.4, you want --dd, not --d. For 2.5: you want --dbg=on or --dd. We have deprecated --dd in 2.5. asya commented on Sun, 10 Nov 2013 23:14:06 +0000: Ok, this is interesting. I just built mongo master with _DEBUG via "--dbg on" flag and two defects disappeared as a result. This deref after null check was one of them - though I don't see any dasserts in the code. I need to look at the two that went away and the 6 new ones that were flagged as a result before setting this live. Just to confirm - my understanding is that -d option should work both with master and 2.2 and 2.4 to turn on _DEBUG? acm commented on Fri, 8 Nov 2013 02:53:44 +0000: I agree about avoiding littering source code annotations everywhere to the extent possible. I'm curious though why we don't do the coverity build with _DEBUG. For its part, the clang static analysis tool explicitly recommends building in debug mode exactly so that debug assertions can be used to prune paths that are known by the programmer to be impossible. I really think a dassert(collection != NULL) right at the beginning of the loop would be cleanest: this is one of those cases where dassert is, to my mind, justified, since it is our belief that a NULL 'collection' pointer is a logical impossibility in the loop, and the dassert could trip only if the surrounding code was changed in some invalid fashion. I definitely wouldn't want to put an all the time assertion here: this is a hot path and we know that it can't be null. Can we consider having coverity run in the equivalent of --dbg=on mode? asya commented on Thu, 7 Nov 2013 23:50:16 +0000: There are several ways this can be done. I prefer not to use source code annotations if possible, and marking this False Positive in Coverity UI will take care of not flagging it again. However, if you feel that there are other places that might be flagged because of the same "root" cause, then you can add an assert which guarantees that collection is not null at that point - our analysis is configured to understand our asserts, but dassert is a tricky one as we don't build with _DEBUG by default for Coverity analysis. I could add configuration which undefs dassert so that I can define it as a regular assert, that may clean up a bunch of other FPs, or it might introduce new ones. acm commented on Thu, 7 Nov 2013 14:51:42 +0000: After Eliot's latest commit here, we believe that this is a false positive. The body of the loop where collection is dereferenced will never be entered if the collection is null, since the runner will not return RUNNER_ADVANCED if the collection does not exist. However, coverity is correct that this is not obvious from the flow of control within this function. We should probably add a coverity annotation (and a dassert?) inside the loop body that declares that 'collection' cannot be NULL. asya Do you have some information on how to write coverity annotations? acm commented on Mon, 4 Nov 2013 21:40:16 +0000: Actually, looks like the relevant lines are these: https://github.com/mongodb/mongo/blob/b42d0215a515a7851bee818595bc67c2e9a955c5/src/mongo/db/ops/update.cpp#L126 and https://github.com/mongodb/mongo/blob/b42d0215a515a7851bee818595bc67c2e9a955c5/src/mongo/db/ops/update.cpp#L131-L132 Coverity is seeing these and saying "OK, you think 'collection' might be NULL, but then you pass it to theDataFileMgr.updateRecord, which dereferences it. The line that is capturing 'collection' and not checking to see if it was NULL was added in this commit by Eliot: https://github.com/mongodb/mongo/commit/f3e324f10697bd4f0c9bdebced4a1e69d91cdd89 If, in fact, the return value of 'getCollection' can be NULL, then the code should change to deal with that outcome. If getCollection cannot return NULL, then the check for it being NULL before calling refreshIndexKeys should be removed, and Coverity will no longer interpret the existence of that check as implying that getCollection can return NULL, which should placate it.

Steps to Reproduce