Skip to content
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

Fix possible type confusion with boost::locale::collator #216

Merged
merged 10 commits into from
Feb 8, 2024

Conversation

Flamefire
Copy link
Collaborator

@Flamefire Flamefire commented Feb 7, 2024

The class derived from std::collate which is always present in std::locale.
So checks for the boost::locale::collator facet may return wrongly true.

As a fix make this an independent facet with its own ID.
Use an adapter such that a std::collate derived class can use its abilities.

Add tests to detect the issue and verify the fix.

Fixes #215

Reproducer for crash due to type confusin as reported in
boostorg#210 (comment)
The value is always non-NULL so return a reference instead.
The default level is "identical" not "primary".
Also fix some typos in the documentation.
As documented this should throw on construction not on use.
- Remove single-use local variables
- Use nullptr instead of 0
- Assert return value
The class derived from `std::collate` which is always present in `std::locale`.
So checks for the `boost::locale::collator` facet may return wrongly true.
As a fix make this an independent facet with its own ID.
Use an adapter such that a std::collate derived class can use its abilities.
Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (941f853) 95.66% compared to head (1bd9b6c) 95.71%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #216      +/-   ##
===========================================
+ Coverage    95.66%   95.71%   +0.05%     
===========================================
  Files          115      116       +1     
  Lines         9885     9919      +34     
===========================================
+ Hits          9456     9494      +38     
+ Misses         429      425       -4     
Files Coverage Δ
include/boost/locale/collator.hpp 100.00% <100.00%> (ø)
src/boost/locale/shared/ids.cpp 100.00% <100.00%> (ø)
src/boost/locale/shared/mo_lambda.cpp 100.00% <100.00%> (ø)
src/boost/locale/shared/std_collate_adapter.hpp 100.00% <100.00%> (ø)
src/boost/locale/win32/api.hpp 91.05% <100.00%> (+2.77%) ⬆️
test/test_generator.cpp 99.60% <100.00%> (+0.05%) ⬆️
test/test_message.cpp 100.00% <100.00%> (ø)
src/boost/locale/icu/collator.cpp 94.20% <94.11%> (+0.08%) ⬆️
src/boost/locale/win32/collate.cpp 82.50% <85.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 941f853...1bd9b6c. Read the comment docs.

@Flamefire Flamefire merged commit d709f92 into boostorg:develop Feb 8, 2024
46 of 47 checks passed
@Flamefire Flamefire deleted the collator-fix branch February 8, 2024 18:03
@artyom-beilis
Copy link
Member

Small but important note: if the facet isn't derived from std::collate the std::locale::operator() can no longer perform its function. https://en.cppreference.com/w/cpp/locale/locale/operator()

the std::locale can be used as replacement for std::less for string comparison.

@Flamefire
Copy link
Collaborator Author

Yes but we have 2 things here: std::collate implementation with "proper unicode support" and boost::locale::collator which is similar to std::collate but provides support for collation levels, used by e.g. comparator

I split those 2 into 2 classes because not all backends implement boost::locale::collator. Where they do an adapter class is used deriving from std::collate to perform the function you mention.

@artyom-beilis
Copy link
Member

Yes I understand that boost::locale::collator is much better. But the fact that the facet is derived from std::collate allows to use something that is provided by standard library (even lesser so):

// standard
std::locale loc = std::locale("ru_RU.UTF-8"); 
// boost
boost::locale::generator gen;
std::locale loc = gen("ru_RU.UTF-8"); 
// same for both variants
std::sort(v.begin(), v.end(), loc);

Something that was direct upgrade path from existing locales (BTW collation is something that actually works with standard locale on Linux quite ok)

I don't know how many users will actually be affected but this is something to think about - if it is worth breaking the code.

@Flamefire
Copy link
Collaborator Author

But the fact that the facet is derived from std::collate allows to use something that is provided by standard library (even lesser so):

Being derived from std::collate implementing additional virtual methods leads to actual, subtile bugs -> See #215
Hence the change to make this safer

// standard
std::locale loc = std::locale("ru_RU.UTF-8"); 
// boost
boost::locale::generator gen;
std::locale loc = gen("ru_RU.UTF-8"); 
// same for both variants
std::sort(v.begin(), v.end(), loc);

This still works as mentioned before. See especially https://github.com/boostorg/locale/pull/216/files#diff-9061a93b241603967564b57e006fdb1956988bbc54d82af2d5d3228e99634914

Something that was direct upgrade path from existing locales (BTW collation is something that actually works with standard locale on Linux quite ok)

What exactly "works with standard locale on Linux quite ok"? Is that affected by this PR anyhow?

I don't know how many users will actually be affected but this is something to think about - if it is worth breaking the code.

I don't think this affects many users. The only ones where code will be broken is if they assume boost::locale::collator derives from std::collator which will now fail to compile.
Other code where there previously was a type confusion will now correctly throw a std::bad_cast as expected.
All other code should continue to work as before as confirmed by the test_collate and test_winapi_collate tests

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.

Use of boost::locale::collator crashes when not using ICU/WinAPI
2 participants