[SERVER-36962] ViewCatalog::resolveView() may return invalid resolved view on encountering invalid namespace in view definition Created: 31/Aug/18  Updated: 27/Oct/23  Resolved: 27/Sep/19

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

Type: Bug Priority: Major - P3
Reporter: James Wahlin Assignee: Haley Connelly
Resolution: Gone away Votes: 0
Labels: query-44-grooming, read-only-views
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Operating System: ALL
Steps To Reproduce:

Discovered via code inspection. Not sure if this can be reproduced externally.

Sprint: Execution Team 2019-10-07
Participants:

 Description   

The ViewCatalog::_lookup_inlock() method will return a null shared_ptr if either:

  1. A view definition is not found for the given namespace
  2. A view catalog reload is needed and the given namespace is not a valid collection name

ViewCatalog::resolveView() however interprets the null shared_ptr as an indication that the namespace is a collection and returns the resolved view. This is incorrect in the case that ViewCatalog::_lookup_inlock() returns due to invalid collection name



 Comments   
Comment by Haley Connelly [ 27/Sep/19 ]

The current version of ViewCatalog::resolveView() calls _lookup(), which no longer checks to see if the namespace is a valid collection name. Due to changes in the code, and since we still allow for views to be created on nonexistent collection namespaces, we believe this is no longer an issue and are closing the ticket.

Comment by Kyle Suarez [ 04/Sep/18 ]

James and I had an in-person conversation about this and threw around a few ideas:

  • We thought about changing the statement in ViewCatalog::_lookup_inlock() from if (!NamespaceString::validCollectionName(ns)) to invariant(NamespaceString::validCollectionName(ns). However, that probably won't be acceptable because it might cause the server to fail to start up in the presence of invalid views. CRUD commands like find and aggregate only perform namespace validation on the namespace of the originating command; they will not validate any other namespaces encountered by the ViewCatalog.
  • A more "correct" fix might be to have ViewCatalog::_lookup_inlock() return nullptr iff the name is not found in the ViewCatalog. If the namespace is invalid, we throw with a special error code that callers can handle on a case-by-case basis. However, that will probably involve lots of changes to the AutoGet* helpers and might not be worth the time.
  • My suggestion in the comment above to throw if the namespace is invalid and the request came from a user connection seems a little hacky but should work for most purposes.

Since we haven't seen this code result in an actual bug for users or in our CI system, I would say it's best to not spend too much time on this if at all.

Comment by Kyle Suarez [ 04/Sep/18 ]

james.wahlin, I believe that view resolution only happens as part of user commands (that is, I don't think ViewCatalog::resolveView() is ever called during the server startup path). Do you think it would be sufficient for ViewCatalog::_lookup_inlock() to throw if the namespace is invalid and the OperationContext indicates that the request comes from a user connection?

Comment by James Wahlin [ 31/Aug/18 ]

As part of this we should audit other callers of ViewCatalog::_lookup_inlock() for potential issues.

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