[CXX-1187] mongocxx::uri::database() segfaults if no database specified in URL Created: 10/Jan/17 Updated: 16/Jun/17 Resolved: 22/Feb/17 |
|
| Status: | Closed |
| Project: | C++ Driver |
| Component/s: | Implementation |
| Affects Version/s: | None |
| Fix Version/s: | 3.2.0-rc0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Adam Seering | Assignee: | J Rassi |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||
| Description |
|
Reproducer:
Expected:
Actual:
Analysis: The root of the problem appears to be in the deceptively-simple-looking "mongocxx::uri::database()":
libmongoc::uri_get_database() returns a char*. Because we are returning a std::string, C++ performs an implicit cast. If no database was specified in the URI, uri_get_database() returns NULL, not (for example) "". I think this is an entirely reasonable C API. However, the implicit std::string(const char*) constructor requires/assumes that the char* point to a valid string. When passed a NULL pointer, it dereferences that pointer and causes a segfault. A trivial solution would be to add a helper that handles NULL somehow. For example:
But there are a bunch of design decisions here. Also, just glancing at the code in this file, I suspect that some other accessors on this class may be affected by the same issue. |
| Comments |
| Comment by Githook User [ 16/Jun/17 ] | ||||||||||||||||||||||||||
|
Author: {u'username': u'jrassi', u'name': u'J. Rassi', u'email': u'rassi@10gen.com'}Message: | ||||||||||||||||||||||||||
| Comment by Githook User [ 16/Jun/17 ] | ||||||||||||||||||||||||||
|
Author: {u'name': u'Adam Seering', u'email': u'aseering@nutonian.com'}Message: Closes #581 | ||||||||||||||||||||||||||
| Comment by Githook User [ 22/Feb/17 ] | ||||||||||||||||||||||||||
|
Author: {u'username': u'jrassi', u'name': u'J. Rassi', u'email': u'rassi@10gen.com'}Message: | ||||||||||||||||||||||||||
| Comment by Githook User [ 22/Feb/17 ] | ||||||||||||||||||||||||||
|
Author: {u'name': u'Adam Seering', u'email': u'aseering@nutonian.com'}Message: Closes #581 | ||||||||||||||||||||||||||
| Comment by J Rassi [ 14/Feb/17 ] | ||||||||||||||||||||||||||
|
I've filed bug ticket | ||||||||||||||||||||||||||
| Comment by David Golden [ 09/Feb/17 ] | ||||||||||||||||||||||||||
|
PR posted here: https://github.com/mongodb/mongo-cxx-driver/pull/581 Thank you! | ||||||||||||||||||||||||||
| Comment by Adam Seering [ 09/Feb/17 ] | ||||||||||||||||||||||||||
|
Ah – I appear to have libmongoc 1.5.2. I've worked around the IPv6 problem for now, but will try updating at some point; hopefully that will fix things. I'm now able to run my tests for the ::database() issue. They need a little tweaking but they mostly pass. I'll try to post a PR soon. | ||||||||||||||||||||||||||
| Comment by David Golden [ 09/Feb/17 ] | ||||||||||||||||||||||||||
|
What version of the libmongoc? This sounds like | ||||||||||||||||||||||||||
| Comment by J Rassi [ 09/Feb/17 ] | ||||||||||||||||||||||||||
|
Ah, good catch! I failed to notice the "IP6" in the tcpdump output you posted. To get the server to listen for IPv6 connections, I believe you have to use the --ipv6 command-line option (or can set this in your config file with {net: {ipv6: true}})? If that doesn't work, removing the "bindIp" directive entirely, in addition. | ||||||||||||||||||||||||||
| Comment by Adam Seering [ 09/Feb/17 ] | ||||||||||||||||||||||||||
|
<looks at the output again> Ah, I see what's going on: "localhost" does not uniquely identify an interface on modern computers. The server is listening exclusively on 127.0.0.1:27017, as per the default MongoDB config file for Mac. The test is exclusively attempting to connect to [::1]:27017. So it's an IPv4 vs IPv6 issue. I tried updating the mongod.conf with:
But this doesn't seem to have caused the server to listen on IPv6. Any thoughts? Is it possible to point the tests at 127.0.0.1 explicitly? | ||||||||||||||||||||||||||
| Comment by J Rassi [ 09/Feb/17 ] | ||||||||||||||||||||||||||
|
The tcpdump output shows that test_driver is indeed sending a SYN to localhost:27017 and getting back an RST, which is unexpected if there is indeed a server listening on localhost:27017. Can you confirm that running tcpdump with the mongo shell instead shows the connection get established (Flags [S] incoming, followed by Flags [S.] outgoing, followed by Flags [.] incoming)? If so, the problem is almost certainly with some local firewall settings, as your operating system decides at the network layer which packets get through and which don't. Can you try temporarily disabling your Mac OS firewall? | ||||||||||||||||||||||||||
| Comment by Adam Seering [ 09/Feb/17 ] | ||||||||||||||||||||||||||
|
Sure:
| ||||||||||||||||||||||||||
| Comment by J Rassi [ 09/Feb/17 ] | ||||||||||||||||||||||||||
|
Hmm. It's somewhat strange that the "mongo" binary can connect but "test_driver" can't; I wonder if there's a misbehaving OS X firewall involved. In one terminal, can you run tcpdump as follows:
Then, in another terminal, can you run the "CRUD functionality" test as follows (from your "out" directory):
Please provide the output of running both commands. Thanks. | ||||||||||||||||||||||||||
| Comment by Adam Seering [ 09/Feb/17 ] | ||||||||||||||||||||||||||
|
Ah, yep, the problem was an issue with the library search path. I'm on a Mac; I had to set DYLD_FALLBACK_LIBRARY_PATH to find both libbson and libmongo_c. I'm now seeing a second problem. Tests are running, but a pile of them are failing with messages that look, for example, like:
I do have a database running locally, and am able to connect to it just fine by running the command-line client as "mongo --host localhost --port 27017". When I log in using the command-line client, I see a line in the server log logging the connection; when I run the test suite, I don't see any new log messages. Any thoughts? | ||||||||||||||||||||||||||
| Comment by J Rassi [ 08/Feb/17 ] | ||||||||||||||||||||||||||
|
We do have testing docs here: http://mongodb.github.io/mongo-cxx-driver/contributing/testing-mongocxx/, but they're rather incomplete. I just filed linked ticket "make test" is indeed the canonical way to run all driver tests, so you're doing the right thing. The only other required step for setup is that you must bring up a MongoDB deployment on localhost:27017 for the integration tests targets to pass (test_driver, test_specs). Any supported server version (2.4+) or server configuration will do; during development we'll often just run the latest stable version (currently 3.4.2) locally in standalone mode. I've most often all tests fail with OTHER_FAULT when the test binaries can't start due to a missing required shared library file (e.g. if the tests are linking dynamically against libbson, but the user has installed libbson to a directory not in the ld.so search path), though I'd have to see the verbose output before making a conclusion. Can you provide the output of running "ctest -VV", in the same directory you're running "make test"? ctest will also print out the full paths of the test executables (test_bson, etc), which you can also run individually. | ||||||||||||||||||||||||||
| Comment by Adam Seering [ 08/Feb/17 ] | ||||||||||||||||||||||||||
|
@rassi : sorry for the radio silence; I was pulled off to other things for a while and just got back to this. I have a tentative patch, but I'm wondering if you have any docs on how to run the mongocxx test suite? If I simply run "make test", I get:
| ||||||||||||||||||||||||||
| Comment by J Rassi [ 18/Jan/17 ] | ||||||||||||||||||||||||||
|
adam.seering: just letting you know we kicked this ticket out to the following sprint (which starts Feb. 10) during our sprint planning meeting today. So, we'd happily accept a pull request for this if you submit one before Feb. 10; if you don't, no worries, we'll just assign this internally on that date. | ||||||||||||||||||||||||||
| Comment by J Rassi [ 13/Jan/17 ] | ||||||||||||||||||||||||||
|
Oh, we'd happily accept a pull request for this issue if you'd like to submit one. Above, I just meant that someone should think about whether any other broken checks on return values from libmongoc::uri_* methods should change the way we handle this one, but that person could be you if you'd like! If you do submit a pull request, please fix the libmongoc error handling for this whole file, and add appropriate test coverage to src/mongocxx/test/uri.cpp. We'll probably want to merge a fix for this sometime in the next few weeks, so we'll just post a comment here that we're starting work on this if you don't happen to get around to it by the time it makes it to the front of our queue. | ||||||||||||||||||||||||||
| Comment by Adam Seering [ 12/Jan/17 ] | ||||||||||||||||||||||||||
|
Thanks for the quick triage, and for the update on your plan! It sounds like things are scheduled? Let me know if there's anything I can do to help. I'd be glad to submit a PR if you'd like, though it might be faster for you to just do it than for you to describe your design decisions to me and review my code For what it's worth, I've worked around this for now by parsing my URL using the C API. Clunky, but it works. | ||||||||||||||||||||||||||
| Comment by J Rassi [ 12/Jan/17 ] | ||||||||||||||||||||||||||
|
Per recent changes to our versioning policy, I've updated this ticket to target 3.2.0-rc0, and created | ||||||||||||||||||||||||||
| Comment by J Rassi [ 10/Jan/17 ] | ||||||||||||||||||||||||||
|
Thanks for the report, adam.seering! I've triaged this into 3.1.2. Your suggestion of returning the empty string in this case matches my intuition, but we'll decide for certain after taking a look at other defective methods in this class. Note that that we've scheduled a full audit of the libmongoc/libbson integration in Relatedly, it seems like the documentation for mongoc_uri_t could use improvement. The eventual assignee of this ticket should make sure to file tickets in CDRIVER for any mongoc_uri_t API methods that are incorrectly documented to return non-null. | ||||||||||||||||||||||||||
| Comment by Adam Seering [ 10/Jan/17 ] | ||||||||||||||||||||||||||
|
Hm, I guess I didn't format my code examples properly... I don't know how to edit this issue description? Let me know if the examples are unclear; I can re-post them. |