[CXX-1066] Audit all libmongoc and libbson calls for error handling Created: 04/Oct/16  Updated: 05/Jul/17  Resolved: 24/May/17

Status: Closed
Project: C++ Driver
Component/s: Implementation
Affects Version/s: None
Fix Version/s: 3.2.0-rc0

Type: Task Priority: Major - P3
Reporter: David Golden Assignee: Samuel Rossi (Inactive)
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
depends on CDRIVER-2095 Document what false means as return v... Closed
Related
related to CXX-1134 Add bsoncxx generic internal error code Closed
related to CDRIVER-1916 bson_iter_init_find() and bson_iter_i... Closed
is related to CXX-1187 mongocxx::uri::database() segfaults i... Closed
is related to CXX-1347 Calls to bson_append_* are not checke... Closed
is related to CDRIVER-2169 Return value for bson_decimal128_from... Closed
is related to CXX-984 Improve use of bson_init_static Closed

 Description   

We've seen several cases where libmongoc or libbson calls aren't checked for errors in the return value or in out parameters. We should audit every single call to confirm that if errors are possible that we are handling them.



 Comments   
Comment by Samuel Rossi (Inactive) [ 24/May/17 ]

From discoveries made while auditing the calls in bsoncxx, I filed CDRIVER-2169 and CXX-1347. Since all of the code has now been audited, I'm closing this ticket.

Comment by David Golden [ 14/Apr/17 ]

Remaining work: review bsoncxx calls to libbson and file tickets for any issues found.

Comment by David Golden [ 23/Mar/17 ]

Filed CDRIVER-2095 and made it a dependency.

Comment by J Rassi [ 11/Nov/16 ]

Note for the eventual assignee of this ticket: bsoncxx::document::view::find() initializes its bson_iter_t twice, and bsoncxx::array::view::find() does as well. These second initializations are redundant, and should potentially be removed as part of implementing this ticket.

Comment by J Rassi [ 11/Nov/16 ]

I filed an improvement request for bson_iter_init_find() and bson_iter_init_find_case() at CDRIVER-1916.

Comment by David Golden [ 07/Nov/16 ]

We'll do that when we do the audit.

Comment by A. Jesse Jiryu Davis [ 07/Nov/16 ]

Feel free to open a ticket with specific functions you'd like improved docs for.

Comment by Bernie Hackett [ 07/Nov/16 ]

driver-c@mongodb.com, is there a CDRIVER ticket to document error handling for the public API?

Comment by David Golden [ 07/Nov/16 ]

I think we should move away from ambiguous functions like bson_iter_init_find and use the separate underlying calls.

Thus, I think we need to check every mongoc function individually to ensure that we understand failure vs error and whether we are handling them appropriately.

Having the C driver better documented about what failure means would help. As written, bson_iter_init should only return false if the provided bson_t is less than the minimum size. If we could rely on that to always be the case for that function, we could maintain that invariant ourselves.

Comment by J Rassi [ 04/Nov/16 ]

You're totally right, I did misunderstand. Ugh, that's a rather unhelpful contract for bson_iter_init_find() to have. Imagine if the read() system call collapsed a return value of -1 and 0 into the same value. I wonder if we should ask the C driver team to provide stronger guarantees of the circumstances under which bson_iter_init() / bson_init_static() / etc will succeed, or if we should ask them to change the return types of these to void and to assert that initialization succeeded with BSON_ASSERT(). We could also avoid using bson_iter_init_find(), but I'm not sure how many other API methods have a similarly ambiguous return value.

Comment by David Golden [ 03/Nov/16 ]

I think you're confusing my two thoughts.

  • There are some cases where we can't tell if a false return value is an error or a failure. Consider bson_iter_init_find. It could return false because initialization failed (an error) or because the key was not found (a failure). We need to be careful about blanket application of something like wrappers because of ambiguities of this type. I think we have to audit calls individually and make sure we know exactly what "error" and "failure" are for each call.
  • If you feel strongly about checking always vs once, then I suggest we find a way to cache the objects so we're not calling repeatedly.
Comment by J Rassi [ 02/Nov/16 ]

Here are a couple of thoughts about how to reduce the amount of boilerplate required for handling libbson errors:

  1. Provide our own throwing wrappers around libbson API calls.
  2. Assert with developer invariants that certain libbson API calls always succeed (for example, if a previous call to bson_init_static() succeeded with the same arguments). We currently don't have any developer invariant utility functions in the C++ driver as far as I can tell, but I think it would be a good idea to introduce them. Note also that libbson calls abort() when the condition passed to any BSON_ASSERT() statement evaluates so false, so there would be precedent for the C++ driver to call std::abort() when a developer invariant is violated.
Comment by J Rassi [ 02/Nov/16 ]

I disagree that "failure" and "error" can be reasonably distinguished here. Even if a call to bson_init_static() with some three arguments returned true at one point, a subsequent call to bson_init_static() with the exact same three arguments could return false, even in the current stable version of libbson (for example, if the data pointed to by the second parameter was modified inbetween calls, perhaps as part of an exploit of some application vulnerability). In addition, if any future version of libbson changes the implementation of bson_init_static() such that a new error branch is introduced, it would not be a backwards-breaking change according to its documented behavior, and we wouldn't necessarily hear about it.

More generally, it's an important secure programming practice to handle any documented error conditions from API calls (mitre.org: Unchecked Return Value). Note also that doing so will avoid us having to maintain a lengthy warnings whitelist once we integrate a static analysis tool into our CI process (CXX-526).

Comment by David Golden [ 02/Nov/16 ]

I suspect you're right, but we need to be careful to distinguish when their return values are "failure" vs "error". I also think there are some cases where we should check once on initialization and then not check again (possibly caching the libbson data structures). E.g. calling bson_static_init over and over isn't going to give a different answer, but I hate seeing it unchecked because I don't know for certain whether and where it's been checked.

Comment by J Rassi [ 02/Nov/16 ]

Implementation thoughts: I suggest we should throw an exception if calls to libbson API functions return an error, such as bson_init_static() or bson_iter_init(). If one of these function calls fails, we currently tell the user that the operation was successful (in the case of document::view::begin() for example, we give the user the incorrect impression that the document is empty). Throwing an exception in these cases will increase user application correctness, and increase diagnosability of libbson issues or data corruption issues.

Generated at Wed Feb 07 22:01:17 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.