[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: |
|
||||||||||||||||||||||||||||||||||||
| 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 |
| 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 |
| 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 |
| 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.
|
| 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:
|
| 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 ( |
| 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. |