[SERVER-49229] use-after-move: 'options' argument in StorageEngineImpl constructor Created: 01/Jul/20 Updated: 29/Oct/23 Resolved: 22/Jul/20 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Storage |
| Affects Version/s: | None |
| Fix Version/s: | 4.7.0 |
| Type: | Bug | Priority: | Minor - P4 |
| Reporter: | Benety Goh | Assignee: | Bynn Lee |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | neweng | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||
| Operating System: | ALL | ||||||||||||
| Sprint: | Execution Team 2020-07-27 | ||||||||||||
| Participants: | |||||||||||||
| Description |
|
The uassert check in the StorageEngineImpl constructor reads the moved 'options' variable. This seems to be a violation of the use-after-move rule in C++. This check is only relevant to storage engines without --directoryperdb support. |
| Comments |
| Comment by Andrew Morrow (Inactive) [ 22/Jul/20 ] | ||
|
benety.goh, yes, of course. | ||
| Comment by Githook User [ 22/Jul/20 ] | ||
|
Author: {'name': 'Bynn Lee', 'email': 'bynn.lee@mongodb.com', 'username': 'bynn'}Message: | ||
| Comment by Benety Goh [ 22/Jul/20 ] | ||
|
We tried running patch build a week ago before assigning this ticket and found numerous violations of the clan-tidy rule. We'll include the link to the patch build in a separate comment. | ||
| Comment by Andrew Morrow (Inactive) [ 22/Jul/20 ] | ||
|
benety.goh - You had mentioned above that you would prefer not to enable the clang-tidy check as part of this ticket but instead to file a new ticket, so I think we should file that. On the other hand, it might be trivial to try to patch build adding the new check once the above CR lands? If it patch builds green, you could just commit it right away. If it doesn't patch build green, you could look at where the errors are reported and that might indicate a team who should pick up the ball. Overall, I want the set of checks to be something that we view has having common ownership: if a bug is found, and clang-tidy could have caught it, I'd prefer that the work to resolve the defect include eliminating remaining instances of that detectable defect along with locking in detection by enabling the new check. Otherwise, it will only be the SDP team trying to move this forward, and that won't scale. | ||
| Comment by Andrew Morrow (Inactive) [ 02/Jul/20 ] | ||
|
benety.goh - Fine by me to do it under a different ticket if there is a reasonable path to actually getting it done promptly. I was very excited that we got clang-tidy stood up in CI under | ||
| Comment by Benety Goh [ 01/Jul/20 ] | ||
|
acm, I'm in favor of adding this to the list of checks in clang-tidy in a separate SERVER ticket. | ||
| Comment by Andrew Morrow (Inactive) [ 01/Jul/20 ] | ||
|
benety.goh - Our toolchain clang-tidy has an explicit checker for this error:
Perhaps, rather than just making the limited change to fix this one instance, it would be worthwhile to expand the list of checks to include this, and see if 1) clang-tidy would have caught this bug and 2) if so, whether there are other instances that should be repaired. Doing so would allow us to permanently add that check to our clang-tidy pass in CI. | ||
| Comment by Benety Goh [ 01/Jul/20 ] | ||
|
This issue was introduced in |