[SERVER-35511] OplogApplier batching logic erroneously reads oplog document size after std::moving. Created: 08/Jun/18  Updated: 29/Oct/23  Resolved: 08/Jun/18

Status: Closed
Project: Core Server
Component/s: Replication
Affects Version/s: None
Fix Version/s: 4.0.0-rc5, 4.1.1

Type: Bug Priority: Major - P3
Reporter: Daniel Gottlieb (Inactive) Assignee: Daniel Gottlieb (Inactive)
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Backports
Depends
Related
Backwards Compatibility: Fully Compatible
Operating System: ALL
Backport Requested:
v4.0
Sprint: Storage NYC 2018-06-18
Participants:
Linked BF Score: 15

 Description   

After moving the oplog entry object, it is accessed to determine the object size. After the move, my machine, says the size of the entry is 5 bytes. This messes up the byte size limit in the batching logic.



 Comments   
Comment by Spencer Brody (Inactive) [ 11/Jun/18 ]

That makes sense, thanks for clarifying!

Comment by Andrew Morrow (Inactive) [ 11/Jun/18 ]

The state of a moved from object is "valid, but unspecified". In the case of BSONObj the moved from state is equivalent to a default constructed BSONObj, which is equivalent to an the "5 byte empty document" BSON representation. So in fact a moved from BSONObj is in a fully valid state. Since the state is unspecified, static analysis tools can't really decide that you are doing the wrong thing to access a moved from object, unfortunately. That said, I think the fact that our default constructed BSONObj has a representation his highly questionable, but given that those were the semantics, it made some degree of sense that the moved from state was made equivalent to the default constructed state. Had we the decision to make over again from the beginning, I would have preferred that a default constructed BSONObj was nullish like unique_ptr, and that we provided a designated static BSONObj::kEmptyDocument to represent the empty document. Then we could have made a moved from BSONObj still have the semantics of returning to a default constructed state, but it would have been an immediate runtime error to use it.

Comment by Spencer Brody (Inactive) [ 11/Jun/18 ]

Is there a reason this wasn't caught by static analysis (coverity)? acm milkie

Comment by Benety Goh [ 11/Jun/18 ]

3.4 and 3.6 are not affected by issue raised in SERVER-35511

Comment by Benety Goh [ 11/Jun/18 ]

daniel.gottlieb, you're right. Updating backport fields.

Comment by Githook User [ 08/Jun/18 ]

Author:

{'username': 'dgottlieb', 'name': 'Daniel Gottlieb', 'email': 'daniel.gottlieb@mongodb.com'}

Message: SERVER-35511: Access oplog entry before destroying its state with std::move.

(cherry picked from commit 359232eed65c26d28dc36edb5cb189ce37b988be)
Branch: v4.0
https://github.com/mongodb/mongo/commit/dbb8a02faa82c2e31307582030f20899c200af29

Comment by Githook User [ 08/Jun/18 ]

Author:

{'username': 'dgottlieb', 'name': 'Daniel Gottlieb', 'email': 'daniel.gottlieb@mongodb.com'}

Message: SERVER-35511: Access oplog entry before destroying its state with std::move.
Branch: master
https://github.com/mongodb/mongo/commit/359232eed65c26d28dc36edb5cb189ce37b988be

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