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.

Sign up for GitHub

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

Jump to bottom

Fix UB in format_arg_store implementation. #3833

Merged
merged 1 commit into from
Jan 31, 2024
Merged

Fix UB in format_arg_store implementation. #3833

merged 1 commit into from
Jan 31, 2024

Conversation

ivafanas
Copy link
Contributor

format_arg_store struct contains pointers to its own members, so autogenerated copy constructor and copy assignment is not correct. Need to reassign pointers to |this| internals.

Compiler won't autogenerate incorrect move constructor and move assignment, because user-provided copy constructor and copy assignment member functions exist.

vitaut
vitaut requested changes Jan 31, 2024
View reviewed changes
Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. format_arg_store is not supposed to be copied so we should delete copy and assignment operators instead.

@ivafanas
Copy link
Contributor Author

Ok.
PR is updated.

@ivafanas
Copy link
Contributor Author

Many reports from CI:

https://github.com/fmtlib/fmt/actions/runs/7728340058/job/21068957883?pr=3833

/home/runner/work/fmt/fmt/include/fmt/xchar.h:91:51: error: use of deleted function ‘fmt::v10::detail::format_arg_store<Context, NUM_ARGS, NUM_NAMED_ARGS, DESC>::format_arg_store(fmt::v10::detail::format_arg_store<Context, NUM_ARGS, NUM_NAMED_ARGS, DESC>&&) [with Context = fmt::v10::generic_context<fmt::v10::basic_appender<wchar_t>, wchar_t>; long unsigned int NUM_ARGS = 3; long unsigned int NUM_NAMED_ARGS = 3; long long unsigned int DESC = 4611686018427388364]’
   91 |   return make_format_args<wformat_context>(args...);

https://github.com/fmtlib/fmt/actions/runs/7728340053/job/21068956966?pr=3833

/Users/runner/work/fmt/fmt/include/fmt/xchar.h:91:10: error: call to deleted constructor of 'decltype(make_format_args<fmt::wformat_context>(args, args, args))' (aka 'format_arg_store<fmt::generic_context<fmt::basic_appender<wchar_t>, wchar_t>, sizeof...(T), 3UL, 4611686018427388364ULL>')
  return make_format_args<wformat_context>(args...);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

https://github.com/fmtlib/fmt/actions/runs/7728340056/job/21068959707?pr=3833

D:\a\fmt\fmt\include\fmt/xchar.h(91): error C2280: 'fmt::v10::detail::format_arg_store<fmt::v10::wformat_context,3,3,4611686018427388364>::format_arg_store(fmt::v10::detail::format_arg_store<fmt::v10::wformat_context,3,3,4611686018427388364> &&)': attempting to reference a deleted function [D:\a\fmt\build\test\xchar-test.vcxproj]

etc.

So, copy constructor must exist. Copy assignment operator might be deleted, I suggest.

@vitaut
Copy link
Contributor

vitaut commented Jan 31, 2024

So, copy constructor must exist. Copy assignment operator might be deleted, I suggest.

Yeah, let's go with that.

phprus
phprus reviewed Jan 31, 2024
View reviewed changes
include/fmt/base.h Outdated
@@ -1636,7 +1636,9 @@ template <typename Context, size_t NUM_ARGS, size_t NUM_NAMED_ARGS,
struct format_arg_store {
// args_[0].named_args points to named_args to avoid bloating format_args.
// +1 to workaround a bug in gcc 7.5 that causes duplicated-branches warning.
arg_t<Context, NUM_ARGS> args[1 + (NUM_ARGS != 0 ? NUM_ARGS : +1)];
static constexpr int ARGS_ARR_SIZE = 1 + (NUM_ARGS != 0 ? NUM_ARGS : +1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe size_t (and i in copy constructor)?
Like size_t NUM_ARGS, size_t NUM_NAMED_ARGS template arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like loops optimization prevention for unsigned iterator due to allowed overflow is not a case here, because loop bounds are known at compile time.

Yep, size_t is a good option here.

@ivafanas ivafanas force-pushed the fix_ub_in_format_arg_store branch 2 times, most recently from 955f509 to 311ffbd Compare January 31, 2024 16:41
vitaut
vitaut reviewed Jan 31, 2024
View reviewed changes
include/fmt/base.h Outdated
Comment on lines 1655 to 1660
format_arg_store(const format_arg_store& rhs) {
args[0] = {named_args, NUM_NAMED_ARGS};
for (size_t i = 1; i < ARGS_ARR_SIZE; ++i) args[i] = rhs.args[i];
for (size_t i = 0; i < NUM_NAMED_ARGS; ++i)
named_args[i] = rhs.named_args[i];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we only need move ctor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@vitaut vitaut merged commit 1b54ba4 into fmtlib:master Jan 31, 2024
41 checks passed
@vitaut
Copy link
Contributor

vitaut commented Jan 31, 2024

Thank you!

happymonkey1 pushed a commit to happymonkey1/fmt that referenced this pull request Apr 7, 2024
@ivafanas @happymonkey1
Fix UB in format_arg_store implementation. (fmtlib#3833)
4409d8f
Tradias
Tradias reviewed Jul 1, 2024
View reviewed changes
include/fmt/base.h
@@ -1649,6 +1651,17 @@ struct format_arg_store {
0,
(init_named_arg(named_args, arg_index, named_arg_index, values), 0)...};
}

format_arg_store(format_arg_store&& rhs) {
Copy link

Choose a reason for hiding this comment

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

This move constructor should be noexcept

@BrewTestBot BrewTestBot mentioned this pull request Jul 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phprus phprus phprus left review comments

@Tradias Tradias Tradias left review comments

@vitaut vitaut vitaut approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ivafanas @vitaut @phprus @Tradias