Skip to content

Conversation

@eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Aug 15, 2025

A small precursor change to CXX-3234 which partially reverts #984 in advance of the upcoming v_noabi refactor, which will adopt v1's "last known element key" behavior instead.


Note

The CXX-2120 implementation is private API whose resulting (undocumented) behavior is the format of error messages, which we do not (want to) consider part of stable API compatibility requirements (although this intention is not explicitly documented by our API Versioning policy... it should really be documented in a yet-to-be-written "Compatibility Guidelines" document). Therefore, this change in behavior is not considered to be an API breaking change.

As noted by comments in prior PRs (here and here), v1 implements an alternative approach to CXX-2120 which is not prone to dangling issues caused by the internal string view. Although (hopefully) unlikely, this dangling issue can be demonstrated by the following code:

// Current API behavior:
bsoncxx::document::value doc = bsoncxx::from_json(R"({"x": 1})");

// Intended behavior: improved exception messages.
try {
  auto type = doc["y"].type();
} catch (bsoncxx::exception const& ex) {
  CHECK_THAT( // Lookup key is present. :)
    ex.what(),
    Catch::Matchers::ContainsSubstring(R"("y")")
  );
}

// Problem: behavior is not extended to `element` access.
try {
  auto type = doc["x"]["y"].type();
} catch (bsoncxx::exception const& ex) {
  CHECK_THAT( // Neither 'x' nor 'y' are present. :(
    ex.what(),
    !Catch::Matchers::ContainsSubstring(R"("x")") && !Catch::Matchers::ContainsSubstring(R"("y")")
  );
}

// Problem: element object may be long-lived (by design).
{
  auto y = doc[std::string("y")]; // Undocumented internal long-term storage as stdx::string_view.
  //           ^~~~~~~~~~~~~~~~   // Temporary object!

  // AddressSanitizer: stack-use-after-scope
  //     # 0 in strlen
  //     # ...
  //     # 2 in std::string::string(char const*)
  //     # 3 in bsoncxx::v_noabi::document::element::type() const
  //     # ...
  CHECK_THROWS(y.type());
}

Due to this potential dangling issue (+ the optional<T>'s impact on element's otherwise semitrivialty), this PR reverts these changes in advance of upcoming changes as part of CXX-3234 which will adopt v1's "last known element key" behavior instead:

// API behavior following CXX-3234:
bsoncxx::document::value doc = bsoncxx::from_json(R"({"x": 1})");

try {
  auto type = doc["y"].type();
} catch (bsoncxx::exception const& ex) {
  CHECK_THAT( // Unfortunately no lookup key... :(
    ex.what(),
    !Catch::Matchers::ContainsSubstring(R"("y")")
  );
}

try {
  auto type = doc["x"]["y"].type();
} catch (bsoncxx::exception const& ex) {
  CHECK_THAT( // ... but does have "last known element key" instead. :)
    ex.what(),
    Catch::Matchers::ContainsSubstring(R"("x")")
  );
}

{
  auto y = doc[std::string("y")]; // No internal reference semantics to the key argument.
  CHECK_THROWS(y.type()); // OK: no undefined behavior.
}

The five ContainsSubstring() assertions modified by this PR will be modified once more by the CXX-3234 PR. These will be the only assertion modifications made by CXX-3234: there will be no other observable change in v_noabi behavior.

@eramongodb eramongodb requested a review from kevinAlbs August 15, 2025 18:48
@eramongodb eramongodb self-assigned this Aug 15, 2025
@eramongodb eramongodb requested a review from a team as a code owner August 15, 2025 18:48
@eramongodb eramongodb merged commit 19bf883 into mongodb:master Aug 18, 2025
20 of 22 checks passed
@eramongodb eramongodb deleted the cxx-2120 branch August 18, 2025 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants