[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:
Related
is related to CDRIVER-1325 Implement The MongoDB Handshake Protocol Closed

 Description   

Found in Fedora QA

https://apps.fedoraproject.org/koschei/package/mongo-c-driver?collection=f28

Test suite fails (have been temporarily disabled)
+ make check
...

{ "status": "PASS", "test_file": "/MongoDB/handshake/appname_in_uri", "seed": "3183281156", "start": 3563529.973239884, "end": 3563529.973289591, "elapsed": 0.000049707 }

,

{ "status": "PASS", "test_file": "/MongoDB/handshake/appname_frozen_single", "seed": "3611459046", "start": 3563529.973296380, "end": 3563529.973334671, "elapsed": 0.000038291 }

,

{ "status": "PASS", "test_file": "/MongoDB/handshake/appname_frozen_pooled", "seed": "1085007876", "start": 3563529.973341565, "end": 3563529.973360169, "elapsed": 0.000018604 }

,
src/mongoc/mongoc-handshake.c:498 _append_and_truncate(): precondition failed: space_for_suffix >= 0
make: *** [Makefile:5650: test] Aborted (core dumped)

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: CDRIVER-2516 fix math for handshake metadata size
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/c2c551e25450bc55bc2437e454ca7a0670f6a725

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.
It will truncate the second part (from , but in this case the first part is already too long.

*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:

max_size =
      HANDSHAKE_MAX_SIZE -
      -_mongoc_strlen_or_zero (_mongoc_handshake_get ()->os_type) -
      ...

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:

{
    os_type = "Darwin",
    os_name = "macOS",
    ...
    platform = "cfg=0x8d68265 posix=200112 stdc=201112 CC=clang 8.1.0 (clang-802.0.42) CFLAGS="-Werror=declaration-after-statement -Wall -Waggregate-return -Wdeclaration-after-statement -Wempty-body -Wformat -Wformat-nonliteral -Wformat-security -Winit-self -Winline -Wmissing-include-dirs -Wno-strict-aliasing -Wno-uninitialized -Wredundant-decls -Wreturn-type -Wshadow -Wswitch-default -Wswitch-enum -Wundef -Wuninitialized" LDFLAGS="
}

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.

   max_size =
      HANDSHAKE_MAX_SIZE -
      _mongoc_strlen_or_zero (_mongoc_handshake_get ()->os_type) -
      _mongoc_strlen_or_zero (_mongoc_handshake_get ()->os_name) -
      _mongoc_strlen_or_zero (_mongoc_handshake_get ()->os_version) -
      _mongoc_strlen_or_zero (_mongoc_handshake_get ()->os_architecture) -
      _mongoc_strlen_or_zero (_mongoc_handshake_get ()->driver_name) -
      _mongoc_strlen_or_zero (_mongoc_handshake_get ()->driver_version);
   _append_and_truncate (
      &_mongoc_handshake_get ()->platform, platform, max_size);

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 ]

=> https://github.com/mongodb/mongo-c-driver/pull/488

Comment by Remi Collet [ 22/Feb/18 ]

I also don't really understand in

max_size =
      HANDSHAKE_MAX_SIZE -
      -_mongoc_strlen_or_zero (_mongoc_handshake_get ()->os_type) -
      ...

Why os_type len is added instead of removed ?

Comment by Remi Collet [ 22/Feb/18 ]

A simple workaround in _set_platform_string

 
   if (str->len > 450) {
      bson_string_truncate (str, 450);
      bson_string_append (str, "…");
   }
   handshake->platform = bson_string_free (str, false);

Comment by Remi Collet [ 22/Feb/18 ]

For readibility, using

fprintf (stderr, "prefix(%d) \"%s\"\ns(%d) \"%s\"\nsuffix(%d) \"%s\"\nmax_len %d\n", (int)strlen(prefix), prefix, (int)strlen(*s), *s, (int)strlen(suffix), suffix, max_len);

Output:

    { "status": "PASS", "test_file": "/MongoDB/handshake/appname_frozen_pooled", "seed": "2992438224", "start": 2186.685494448, "end": 2186.691106773, "elapsed": 0.005612325  },
prefix(502) "cfg=0x15eb8f9 posix=200809 stdc=201710 CC=GCC 8.0.1 20180218 (Red Hat 8.0.1-0.14) CFLAGS="-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -mcet -fcf-protection" LDFLAGS="-Wl,-z,relro  -specs=/usr/lib/rpm/redhat/redhat-hardened-ld""
s(502) "cfg=0x15eb8f9 posix=200809 stdc=201710 CC=GCC 8.0.1 20180218 (Red Hat 8.0.1-0.14) CFLAGS="-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -mcet -fcf-protection" LDFLAGS="-Wl,-z,relro  -specs=/usr/lib/rpm/redhat/redhat-hardened-ld""
suffix(28) "./configure -nottoomanyflags"
max_len 465
src/mongoc/mongoc-handshake.c:502 _append_and_truncate(): precondition failed: space_for_suffix >= 0
    { "status": "FAIL", "test_file": "/MongoDB/handshake/success", "seed": "3129179474", "start": 2186.691139400, "end": 2186.699168636, "elapsed": 0.008029236  },
    { "status": "PASS", "test_file": "/MongoDB/handshake/failure", "seed": "590805413", "start": 2186.699193384, "end": 2186.704377344, "elapsed": 0.005183960  },
prefix(502) "cfg=0x15eb8f9 posix=200809 stdc=201710 CC=GCC 8.0.1 20180218 (Red Hat 8.0.1-0.14) CFLAGS="-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -mcet -fcf-protection" LDFLAGS="-Wl,-z,relro  -specs=/usr/lib/rpm/redhat/redhat-hardened-ld""
s(502) "cfg=0x15eb8f9 posix=200809 stdc=201710 CC=GCC 8.0.1 20180218 (Red Hat 8.0.1-0.14) CFLAGS="-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -mcet -fcf-protection" LDFLAGS="-Wl,-z,relro  -specs=/usr/lib/rpm/redhat/redhat-hardened-ld""
suffix(511) "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
max_len 492
src/mongoc/mongoc-handshake.c:502 _append_and_truncate(): precondition failed: space_for_suffix >= 0
    { "status": "FAIL", "test_file": "/MongoDB/handshake/too_big", "seed": "1488662757", "start": 2186.704407456, "end": 2186.709587479, "elapsed": 0.005180023  },

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:

   space_for_suffix = max_len - strlen (prefix) - delim_len;
 
   if (space_for_suffix < 0) {
      fprintf (stderr, "prefix \"%s\", s \"%s\", suffix \"%s\", max_len %d\n", prefix, *s, suffix, max_len);
   }
 
   BSON_ASSERT (space_for_suffix >= 0);

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
https://bugzilla.redhat.com/show_bug.cgi?id=1547618

Generated at Wed Feb 07 21:15:28 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.