[SERVER-56892] Coverity analysis defect 120053: Dereference after null check Created: 12/May/21  Updated: 12/May/21  Resolved: 12/May/21

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

Type: Bug Priority: Major - P3
Reporter: Coverity Collector User Assignee: Daniel Gottlieb (Inactive)
Resolution: Won't Fix Votes: 0
Labels: coverity
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Operating System: ALL
Participants:

 Description   

Dereference after null check

Either the check against null is unnecessary, or there may be a null pointer dereference. Pointer is checked against null but then dereferenced anyway
/src/mongo/db/op_observer_impl.cpp:749: FORWARD_NULL 120053 Comparing "args.deletedDoc" to null implies that "args.deletedDoc" might be null.



 Comments   
Comment by Daniel Gottlieb (Inactive) [ 12/May/21 ]

IIUC milkie, it's ok to just close this as "won't fix", but please advise if that wasn't correct.

Comment by Daniel Gottlieb (Inactive) [ 12/May/21 ]

My understanding is that coverity is seeing this block to learn that args.deletedDoc can be null:

        if (args.deletedDoc && !oplogEntry.getNeedsRetryImage()) {
            // If we have a deletedDoc preImage and we're not writing it to
            // `config.image_collection`, instead write it to the oplog.
            deletedDocForOplog = {*(args.deletedDoc)};
        }

However the dereference here does not also check whether the pointer is null:

        if (oplogEntry.getNeedsRetryImage()) {
            writeToImageCollection(opCtx,
                                   *opCtx->getLogicalSessionId(),
                                   opTime.writeOpTime.getTimestamp(),
                                   repl::RetryImageEnum::kPreImage,
                                   *(args.deletedDoc));
        }

The flaw in coverity's logic is that args.deletedDoc is dependent on oplogEntry.getNeedsRetryImage. We can add an invariant(args.deletedDoc); (which would presumably satisfy coverity) inside the previous `then` block, but that was intentionally omitted. It was deemed redundant with the fact that the nullptr derefence would also (appropriately) crash the system.

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