CXX-813 Fix memory errors exposed by ASAN and Valgrind #441
Conversation
|
@hanumantmk @amidvidy @ajdavis I'd be interested in your thoughts on the changes around |
|
Ping? |
src/mongocxx/instance.cpp
Outdated
There was a problem hiding this comment.
Possible to error with a helpful message?
There was a problem hiding this comment.
Yes, good suggestion. Thanks.
|
One thing to note in this review is that it is still possible to fail to initialize the C driver correctly. For instance, if the first thing you constructed was a read_preference object, you might end up calling Another way to go with this change would be to unwind my auto-instance work in Thoughts? |
|
To the extent I understand C++, the code looks fine to me. But I dislike the philosophy of this approach: the driver should require users to explicitly call init and cleanup. The C Driver should require you to call mongoc_init and mongoc_cleanup, and on non-GCC it does require you to call them. (There is a misfeature that uses GCC tricks to do call init and cleanup automatically, I plan to remove that in the C Driver 2.0.) So, for the sake of correctness, clarity, simplicity, I propose requiring the user to init and cleanup the C++ driver explicitly. |
|
@ajdavis I think I agree. However, requiring them to drag around an object forever seems cruel. I think what I will do is to unwind the magic auto-initialization in |
|
Oh, I misunderstood, partly. I was arguing that the C++ driver should make the user do something manually that executes the C Driver's global initialization and cleanup routines, the same way the C Driver requires users to call mongoc_init and mongoc_cleanup themselves. I don't have an opinion about the "instance" object, I'm not familiar with what it does. |
|
@ajdavis The |
|
Updated, PTAL. No longer automatically creates an instance for you, so all the tests now call |
|
LGTM. In the docs, do you or will you recommend that C++ driver users make an Our examples show mongoc_init and mongoc_cleanup at the top and bottom of On Thu, Jan 21, 2016 at 3:39 PM, Andrew C. Morrow [email protected]
|
|
@ajdavis Yes that is the idea. I doubt we have it documented anywhere yet though. |
There was a problem hiding this comment.
Per discussion, we can't do with enums (or lots of other things to) because ABI. However, we haven't set an ABI yet, so I will put it in the right place.
|
A few nits, also consider adding a test that we throw if you attempt to use the driver without creating an instance, otherwise LGTM |
|
Oh actually why don't the examples also need |
|
LGTM after in-person discussion |
|
- We can't assume that libmongoc has automatically called init/cleanup for us. It only does that on some platforms. That makes it mandatory to have an instance object. Many of our tests didn't do that. - The collection::create_index method was not cleaning up the keys that had been allocated by libbson. Add the needed bson_free call. - The collection::distinct method was not cleaning up the temorary database object that it constructs. Add the needed database_destroy call. - Fix some mocks so that they write to the bson_error_t out parameter when returning a non-successful code, as otherwise these lead to read-from-uninit errors when we promote the uninitialized bson_error_t to an exception.
|
Eh, I'm fine without the test too |
3 participants
for us. It only does that on some platforms. That makes it mandatory
to have an instance object. Many of our tests didn't do that, and it
would be painful for users. Set things up so that an instance is
implicitly created as needed, and provide ways to retrieve the
instance. This required moving the tests for instance to its own
process, among other nuisances.
had been allocated by libbson. Add the needed bson_free call.
database object that it constructs. Add the needed database_destroy
call.
when returning a non-successful code, as otherwise these lead to
read-from-uninit errors when we promote the uninitialized bson_error_t
to an exception.