Skip to content

CDRIVER-5669 improve string function handling of large strings #1696

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Aug 12, 2024

Conversation

kevinAlbs
Copy link
Collaborator

@kevinAlbs kevinAlbs commented Aug 12, 2024

See CDRIVER-5669 for a detailed description of the issue. Verified by this patch build.

Additional improvements:

  • Document expected max length for bson_string_t functions.
  • Add tests to help with local verification. Tests are not run in Evergreen due to observed 25s runtime on ASan and failure to allocate on TSan.
  • Add a private bson_next_power_of_two_u32 to avoid casts between uint32_t and size_t.

To clarify assert is intended guarantee cast is within bounds.
To check in case range of `size_t` is smaller than `uint32_t` (unlikely?)
Skip test by default. Large allocations fail on TSan tasks, and are slow on ASan tasks. Test may be used to verify behavior locally.
@kevinAlbs kevinAlbs marked this pull request as ready for review August 12, 2024 15:59
Copy link
Contributor

@rcsanchez97 rcsanchez97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (based on Ezra's suggestions), with only some minor additional grammar nits.

@kevinAlbs kevinAlbs requested a review from eramongodb August 12, 2024 19:37
@kevinAlbs kevinAlbs merged commit 84ac397 into mongodb:master Aug 12, 2024
46 of 47 checks passed
kevinAlbs added a commit that referenced this pull request Aug 12, 2024
* add `bson_string_ensure_space` to check addition and casts.

* add private `bson_next_power_of_two_u32` to avoid casts.

* document length expectations

* add test for maximum capacity of `bson_string_t`

Skip test by default. Large allocations fail on TSan tasks, and are slow on ASan tasks. Test may be used to verify behavior locally.

---------

Co-authored-by: Ezra Chung <[email protected]>
Co-authored-by: Roberto C. Sánchez <[email protected]>
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.

3 participants