[CDRIVER-2516] assert fails formatting handshake metadata with long platform string Created: 21/Feb/18 Updated: 28/Oct/23 Resolved: 23/Feb/18 |
|
| Status: | Closed |
| Project: | C Driver |
| Component/s: | libmongoc |
| Affects Version/s: | 1.9.2 |
| Fix Version/s: | 1.10.0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Remi Collet | Assignee: | A. Jesse Jiryu Davis |
| Resolution: | Fixed | Votes: | 1 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Environment: |
Fedora 28 |
||
| Issue Links: |
|
||||||||
| Description |
|
Found in Fedora QA https://apps.fedoraproject.org/koschei/package/mongo-c-driver?collection=f28 Test suite fails (have been temporarily disabled) , , , Also raise the same issue with PHP extension (when moving from 7.2.2 to 7.2.3RC1, it seems the RC1 suffix raise the 32 char limit) Is it possible to increase this size ? |
| Comments |
| Comment by A. Jesse Jiryu Davis [ 23/Feb/18 ] | ||||||||||||||||||||
|
Thanks Remi. To make patch releases as low-risk as possible, I believe that 1.9.x releases should only fix bugs introduced since 1.9.0, and this bug was introduced in 1.5.0. We'll wait until the next minor release to fix this bug. | ||||||||||||||||||||
| Comment by Remi Collet [ 23/Feb/18 ] | ||||||||||||||||||||
|
No urgency (my ugly workaround is applied and does the trick) BTW, will be nice to have the fix applied in 1.9 branch, if a new 1.9.3 have to be released. (and in this case, I will probably switch to this solution, especially is F28, planed for May, is released before 1.9.3/1.10.0) | ||||||||||||||||||||
| Comment by A. Jesse Jiryu Davis [ 23/Feb/18 ] | ||||||||||||||||||||
|
Fixed on master. We now include as much of the "platform" string as possible in the metadata we send to the server. If the PHP Driver, or some other driver that wraps the C Driver, calls mongoc_handshake_data_append to add to the "platform" string but there is no space remaining, then the C Driver ignores the additional data and preserves the "platform" string as it is. I think this is better then reserving 25% of the space - when the "platform" field is initialized, we don't know if mongoc_handshake_data_append is ever going to be called or not. So it's better to save as much of the initial "platform" field as possible. Furthermore, I think it will be easier for tools that analyze the "platform" field on the server to understand a field that's been truncated once instead of possibly truncated twice. That's a worthwhile tradeoff in exchange for possibly ignoring the PHP Driver's entire "platform" string if there's no space left for it. Remi, does this need to be fixed right away in order to release the current C Driver and the PHP Driver for Fedora? Or can we wait a couple months to release this fix as part of 1.10.0? | ||||||||||||||||||||
| Comment by Githook User [ 23/Feb/18 ] | ||||||||||||||||||||
|
Author: {'email': 'jesse@mongodb.com', 'name': 'A. Jesse Jiryu Davis', 'username': 'ajdavis'}Message: | ||||||||||||||||||||
| Comment by Kevin Albertson [ 22/Feb/18 ] | ||||||||||||||||||||
|
remi Right, the first part being too long is causing this assertion. But there's no need to truncate the platform string at this point, but we will have to change how we append the platform string here. We'll truncate it later in the call to _append_platform_field when constructing the BSON handshake document. | ||||||||||||||||||||
| Comment by Remi Collet [ 22/Feb/18 ] | ||||||||||||||||||||
|
> It will get truncated when we append it to the handshake BSON document I don't think. *s = bson_strdup_printf ("%s / %.*s", prefix, space_for_suffix, suffix); In test_mongoc_handshake_data_append_success (which fails) const char *platform = "./configure -nottoomanyflags"; So this is not this one which is too long, but the default platform, generated in _set_platform_string If you remove the first assert, the printf will received a <0 length and the second assert will fail... | ||||||||||||||||||||
| Comment by A. Jesse Jiryu Davis [ 22/Feb/18 ] | ||||||||||||||||||||
|
Agreed. Fix incoming. | ||||||||||||||||||||
| Comment by Kevin Albertson [ 22/Feb/18 ] | ||||||||||||||||||||
|
remi I think you're right about the following being wrong:
That extra minus is likely a typo. Here's how I understand the handshake logic: The global handshake struct gMongocHandshake is initialized in mongoc_init, with values like:
Normally, we construct a handshake BSON document with _mongoc_handshake_build_doc_with_application when we need to send it. This appends the platform field with _append_platform_field, which will put as much of the platform string as it can inside the resulting document. So truncation of the platform string occurs here, just before we add it to the handshake BSON document. The mongoc_handshake_data_append function is used by other drivers to append to the strings in the gMongocHandshake struct. This is where we hit the assertion.
The call to _append_and_trucate asserts that max_size can fit the pre-existing platform string (and delimitter) before appending to it. But we can't and shouldn't assert that, since we haven't truncated the original platform string yet. It will get truncated when we append it to the handshake BSON document. So I think the solution is to not truncate the platform string here and just append to it. | ||||||||||||||||||||
| Comment by Remi Collet [ 22/Feb/18 ] | ||||||||||||||||||||
|
So, this issue was raised by new build options introduced in Fedora 28+ | ||||||||||||||||||||
| Comment by Remi Collet [ 22/Feb/18 ] | ||||||||||||||||||||
| Comment by Remi Collet [ 22/Feb/18 ] | ||||||||||||||||||||
|
I also don't really understand in
Why os_type len is added instead of removed ? | ||||||||||||||||||||
| Comment by Remi Collet [ 22/Feb/18 ] | ||||||||||||||||||||
|
A simple workaround in _set_platform_string
| ||||||||||||||||||||
| Comment by Remi Collet [ 22/Feb/18 ] | ||||||||||||||||||||
|
For readibility, using
Output:
| ||||||||||||||||||||
| Comment by A. Jesse Jiryu Davis [ 22/Feb/18 ] | ||||||||||||||||||||
|
Thanks, we must have an arithmetic mistake somewhere. I don't know whether we should increase the 32 char maximum size: the total size of platform information is a maximum of 512 bytes according to the specification, so increasing the size of one field requires us to decrease the size of another. I can't reproduce this ASSERT locally, could you help me out please? Update _append_and_truncate to include:
I'd like to see the output of that on a failing test. Thank you. | ||||||||||||||||||||
| Comment by Remi Collet [ 21/Feb/18 ] | ||||||||||||||||||||
|
Fedora bug tracker |