[SERVER-24272] StageBuilder should have an explicit invariant that calls to findIndexByKeyPattern() return non-null Created: 24/May/16  Updated: 27/Aug/16  Resolved: 03/Jun/16

Status: Closed
Project: Core Server
Component/s: Querying
Affects Version/s: None
Fix Version/s: 3.3.8

Type: Bug Priority: Major - P3
Reporter: Coverity Collector User Assignee: David Storch
Resolution: Done Votes: 0
Labels: coverity
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Backwards Compatibility: Fully Compatible
Operating System: ALL
Sprint: Query 16 (06/24/16)
Participants:

 Description   

Return value of function which returns null is dereferenced without checking

Defect 99343 (STATIC_C)
Checker NULL_RETURNS (subcategory none)
File: /src/mongo/db/query/stage_builder.cpp
Function mongo::buildStages(mongo::OperationContext *, mongo::Collection *, const mongo::CanonicalQuery &, const mongo::QuerySolution &, const mongo::QuerySolutionNode *, mongo::WorkingSet *)
/src/mongo/db/query/stage_builder.cpp, line: 308
Assigning: "params.descriptor" = null return value from "findIndexByKeyPattern".

            params.descriptor =

File: /src/mongo/db/query/stage_builder.cpp
Function mongo::buildStages(mongo::OperationContext *, mongo::Collection *, const mongo::CanonicalQuery &, const mongo::QuerySolution &, const mongo::QuerySolutionNode *, mongo::WorkingSet *)
/src/mongo/db/query/stage_builder.cpp, line: 324
Assigning: "params.descriptor" = null return value from "findIndexByKeyPattern".

            params.descriptor =



 Comments   
Comment by Githook User [ 03/Jun/16 ]

Author:

{u'username': u'dstorch', u'name': u'David Storch', u'email': u'david.storch@10gen.com'}

Message: SERVER-24272 add invariant() that IndexDescriptors are non-null in StageBuilder::build()
Branch: master
https://github.com/mongodb/mongo/commit/681372e942244307067b39c5fe2b27069f5fbbe7

Comment by David Storch [ 03/Jun/16 ]

Since query planning and stage building must happen under the collection lock (without yielding locks in between or during), any index that was available during planning must also be available when constructing PlanStages. Therefore, I believe Coverity's concern about a possible null dereference is a false positive. A null dereference would only be possible if a bug in the query planner resulted in us attempting to use a non-existent index, or if a problem with locking allowed an index to be dropped out from under us.

However, there are a few places in stage_builder.cpp were we check for a null IndexDescriptor where null should be impossible:

        params.descriptor =
            collection->getIndexCatalog()->findIndexByKeyPattern(txn, ixn->indexKeyPattern);
        if (params.descriptor == NULL) {
            warning() << "Can't find index " << ixn->indexKeyPattern.toString() << "in namespace "
                      << collection->ns() << endl;
            return NULL;
        }

Our code coverage tool confirms that our tests do not exercise the code inside this if block, increasing my confidence that it is in fact not reachable. These should be converted into explicit calls to invariant() ensuring that the IndexDescriptor is non-null.

Generated at Thu Feb 08 04:05:52 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.