[CXX-1184] Prevent misuse of client temporaries Created: 05/Jan/17  Updated: 27/Mar/18  Resolved: 08/Mar/18

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

Type: New Feature Priority: Major - P3
Reporter: David Golden Assignee: Unassigned
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
depends on CDRIVER-1980 Incorrect lifecycle documentation nee... Closed
Related

 Description   

(Original title: "find/cursor documentation doesn't warn about client lifetime")

From https://groups.google.com/forum/#!topic/mongodb-user/girvbR21AzM:

auto c = pool->acquire()->database()[collection].find({});
// loop c

pool->acquire() returns an unique_ptr, and cursor object c rely on this unique_ptr, before c destroy, unique_ptr need stay.
This is dangerous, however user may never find this bug until they release into production env.
Maybe add an assert to check this, let it crash early.



 Comments   
Comment by Jamie Lannister [ 27/Mar/18 ]

I'm sorry, Please ignore my previous comments. I think I have fixed my problem.

the issue is that I have a main project which includes another sub project with git submodule.

within both the main project and the sub project, there are MongoDb wrappers defined using the same name.

This must have confused the compiler/linker and caused some thing uninitialized. I have renamed one of the wrapper to use a different name and the issue seemed to be fixed.

Comment by Andrew Morrow (Inactive) [ 27/Mar/18 ]

It is impossible to define copy or copy-assignment operators for that class exactly because it contains a unique_ptr. Similarly, the compiler generated move constructor and move assignment operator should be automatically correct.

I suspect something else is going wrong.

As this ticket is closed, I'd prefer that you do one of the following so we can continue to investigate:

  • Email mongodb-user with a reproducing test case and details on your platform, C++ Driver version, and toolchain
  • Open a question on stackoverflow with the mongo-cxx-driver tag, providing the same info
  • Open a new ticket in the CXX project on JIRA, providing the same info

That will make it easier for us to assign someone to work with you to understand the root cause of this issue.

Comment by Jamie Lannister [ 27/Mar/18 ]

it is a crash, a segmentation fault caused by assignment of the object mongocxx::pool::entry

the issue is because of the unique pointer used in that class:

should a copy constructor and a operator= be defined for this class?

        using unique_client = std::unique_ptr<client, std::function<void MONGOCXX_CALL(client*)>>;
 
        MONGOCXX_PRIVATE explicit entry(unique_client);
 
        unique_client _client;

Comment by Andrew Morrow (Inactive) [ 27/Mar/18 ]

serjaimelannister - What is the error message your compiler emits?

Comment by Jamie Lannister [ 27/Mar/18 ]

The change for this bug, commit https://github.com/mongodb/mongo-cxx-driver/commit/423d3b9613c5fa0c95d649f528c064107df86ef0

seems to break my code.

In my code, I have a mongodb wrapper class, in which there is a getClient() that returns a client to use.

the function is defined:

mongocxx::pool::entry MongodbManager::getClient()
{
    return m_pool.acquire();
}

the client, as returned by pool.acquire() is not returnable because of the unique_ptr member.

Comment by Githook User [ 17/Mar/18 ]

Author:

{'email': 'jesse@mongodb.com', 'name': 'A. Jesse Jiryu Davis', 'username': 'ajdavis'}

Message: CXX-1184 Mark pool::entry for export
Branch: master
https://github.com/mongodb/mongo-cxx-driver/commit/1c4bbc84149f750ddc1dc3300d9108cc8638910d

Comment by Githook User [ 08/Mar/18 ]

Author:

{'email': 'jesse@mongodb.com', 'name': 'A. Jesse Jiryu Davis', 'username': 'ajdavis'}

Message: CXX-1184 prohibit calling cursor-returning fns on temporaries
Branch: master
https://github.com/mongodb/mongo-cxx-driver/commit/423d3b9613c5fa0c95d649f528c064107df86ef0

Comment by A. Jesse Jiryu Davis [ 24/Feb/18 ]

Demo (this segfaults):

#include <iostream>
 
#include <bsoncxx/json.hpp>
#include <mongocxx/pool.hpp>
#include <mongocxx/client.hpp>
#include <mongocxx/instance.hpp>
 
int main() {
    mongocxx::instance inst{};
 
    auto pool = new mongocxx::pool{mongocxx::uri{"mongodb://localhost/?minPoolSize=1"}};
    {
        auto c = pool->acquire()->database("db")["collection"].find({});
        auto tmp = pool->acquire(); // the cursor's client
        auto tmp2 = pool->acquire(); // another client
        tmp = nullptr; // return the cursor's client
        tmp2 = nullptr; // return the other client, the cursor's client is destroyed
 
        for (auto &&d : c) {
            std::cout << bsoncxx::to_json(d) << std::endl;
        }
    }
 
    delete pool;
 
    return 0;
}

Swapping the line tmp = nullptr; with the next line allows the program to complete.

Comment by Andrew Morrow (Inactive) [ 05/Jan/17 ]

My point was more that the expression as written contains two bugs, not just one.

As for lifecycle contracts, this was extensively debated during the design phase of the C++11 driver, and the conclusion was that we shouldn't try to guarantee them - C++ isn't Java, and managing lifetimes to honor contracts is an expected part of using the language and its associated libraries. What we can, and should, do, is document those constraints and attempt to make apparently safe idioms not compile. We can do that for above case by disallowing invocation of 'find' or other cursor returning objects on collection temporaries.

Note also that it is almost certainly possible to build a set of shared pointer managed types that sit above client/database/collection/cursor and do the right thing.

Comment by David Golden [ 05/Jan/17 ]

Note: per Slack conversation, mongoc_cursor_t only really depends on mongoc_client_t. We should wait until CDRIVER-1980 (fixing lifecycle docs) is done before finalizing a solution.

Comment by David Golden [ 05/Jan/17 ]

From http://mongoc.org/libmongoc/current/mongoc_collection_t.html:

It is an error to call mongoc_collection_destroy() on a collection that has operations pending. It is required that you release mongoc_cursor_t structures before calling mongoc_collection_destroy().

From http://mongoc.org/libmongoc/current/mongoc_database_t.html

It is an error to call mongoc_database_destroy() on a database that has operations pending. It is required that you release mongoc_cursor_t structures before calling mongoc_database_destroy.

From http://mongoc.org/libmongoc/current/mongoc_client_t.html:

It is an error to call mongoc_client_destroy on a client that has operations pending. It is required that you release mongoc_collection_t and mongoc_database_t structures before calling mongoc_client_destroy.

We need to think more broadly how to guarantee those lifecycle contracts because right now we don't guarantee the destruction order that libmongoc mandates. While I know we've avoided shared pointers, having everything reference count upwards would certainly do the trick. That has a cost, sure, but I'm not sure how else we can satisfy the contract in code. We could document the same constraints for our users, but then we're sort of doing RAII in name only, as they still need to manage destruction order manually.

Other ideas?

Comment by Andrew Morrow (Inactive) [ 05/Jan/17 ]

Is it legal to use the cursor result of mongocxx::collection::find after the mongocxx::collection has been destroyed? If not, there may actually be two bugs in the above statement, but it is not clear from the underlying mongoc docs whether or not it is OK.

Independently of that, it may in fact be possible to make the above bug illegal, but returning a proxy object rather than a bare unique_ptr from mongocxx::pool::acquire. The proxy object would implement operator-> but it would have an explicitly deleted operator-> for the && case, much like mongocxx::client::database.

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