[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: |
|
||||||||||||
| 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 |
| 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: (cherry picked from commit 359232eed65c26d28dc36edb5cb189ce37b988be) |
| Comment by Githook User [ 08/Jun/18 ] |
|
Author: {'username': 'dgottlieb', 'name': 'Daniel Gottlieb', 'email': 'daniel.gottlieb@mongodb.com'}Message: |