[SERVER-54418] mongos collMod command does not validate its reply Created: 09/Feb/21 Updated: 29/Oct/23 Resolved: 10/Feb/21 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Replication |
| Affects Version/s: | None |
| Fix Version/s: | 4.9.0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | A. Jesse Jiryu Davis | Assignee: | Moustafa Maher |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Backwards Compatibility: | Minor Change |
| Operating System: | ALL |
| Sprint: | Repl 2021-02-22 |
| Participants: |
| Description |
|
I think there's a bug in cluster_collection_mod_cmd.cpp:
That will not call Reply::parse if the reply includes a "raw" field. Thus I think Reply::parse is never called? |
| Comments |
| Comment by Githook User [ 10/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Moustafa Maher', 'email': 'm.maher@10gen.com', 'username': 'moustafamaher'}Message: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by A. Jesse Jiryu Davis [ 09/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Sounds good to me. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Moustafa Maher [ 09/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Yes you are right, there is no harm to move the else block, as the collModReply doesn't have any non-optional fields that would have failed parsing, although it will add more value instead. I don't think we should add "raw" field to the idl file, as it is a custom field that we add from mongos only, and shouldn't be defined in the stable API, and we should add it to "ignorableFields", what do you think ? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by A. Jesse Jiryu Davis [ 09/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Let's validate the whole reply, and ensure it conforms to the CollModReply IDL definition:
For example, if mongos always adds the "raw" field, and we changed mongos in the future to add a "hidden_new" field of type "int", that would violate this IDL definition but the current code won't catch this violation because it doesn't call Reply::parse if "raw" is present. I propose:
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Moustafa Maher [ 09/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
But we can always call this: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Moustafa Maher [ 09/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
top-level result-obj doesn't have anything else than "raw" if there are no errors: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by A. Jesse Jiryu Davis [ 09/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Do we need to verify the top-level resultObj or not? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Moustafa Maher [ 09/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
if the reply includes a "raw" field, we will go through each shard reply and call Reply::parse on it. |