Patch #819 for dynamic arg set doesn't work correctly

[do-m-en]

I did not realize that the patch was accepted a while back so I'm creating a new bug report for this.

Patch #819 doesn't work as expected.

using ctx = fmt::format_context;
std::vector<fmt::basic_format_arg<ctx>> args;
args.emplace_back(fmt::internal::make_arg<ctx>(42));
args.emplace_back(fmt::internal::make_arg<ctx>(43));

std::cout << fmt::vformat("{} {}", fmt::basic_format_args<ctx>(args.data(), static_cast<unsigned int>(args.size()))) << '\n';

std::cout << fmt::vformat("{} {}", fmt::basic_format_args<ctx>(args.data(), static_cast<unsigned int>(888))) << '\n' << std::flush;

result is 42 43 in both cases so the size is ignored and only the count of {} in template string is used.

In the above case it's not relevant (just a bit more typing as it's just a tag to call a different constructor...) but this causes issues with named arguments:

using ctx = fmt::format_context;
std::vector<fmt::basic_format_arg<ctx>> args;
int fourty_two = 42;
std::string and_the_name = "content";
auto content = fmt::arg(and_the_name, fourty_two);
args.emplace_back(fmt::internal::make_arg<ctx>(content));
int fourty_three = 43;
std::string and_the_name_2 = "x2";
auto x2 = fmt::arg(and_the_name_2, fourty_three);
args.emplace_back(fmt::internal::make_arg<ctx>(x2));

std::cout << fmt::vformat("{content} {x2}", fmt::basic_format_args<ctx>(args.data(), static_cast<unsigned int>(args.size()))) << '\n' << std::flush;

Patch introduces types_(-(int64_t)count) which triggers asan triggers in format.h:1449 once it tries to dereference args.args_[2] for the 3rd parameter even though there are only 2 present.
Changing the line to use basic_format_args::type instead returns 15 (enum type::custom_type)
for third parameter instead of 0 (none_type) so it causes an infinite loop.

The basic_format_args::type is not used on line 1449 - I've just changed the line from args.args_[i].type_ to args.type(i) for testing... The args.args_[i] triggers asan, args.type(i) start cycling as it always gets some non 0 (none_type) result. Sry that confusing part - the rest is as I've written.

The fix for this case is to just add the terminator but it's confusing since args.size() was already provided:

args.emplace_back(fmt::internal::make_arg<ctx>(x2));
args.emplace_back(fmt::basic_format_arg<ctx>()); // add the terminator...

std::cout << fmt::vformat("{content} {x2}", fmt::basic_format_args<ctx>(args.data(), static_cast<unsigned int>(args.size()))) << '\n' << std::flush;

Using the above example with non named parameters and three {}:

std::cout << fmt::vformat("{} {} {}", fmt::basic_format_args<ctx>(args.data(), static_cast<unsigned int>(888))) << '\n' << std::flush;

Returns 42 42 43 which proves that the size parameter is not being used.
And also as a side note I'd expect the output to be 42 43 43 - repeating the last not the first item which makes me wonder if the {} are scanned frist and replaced later instead of just streaming and replacing on the fly when encountered in case of the dynamic vformat version (I could be wrong but my guess would be that it performs slower because of that). I would also expect that 42 43 43 would be the intuitive output (or that the {} count miss match would cause an exception or is this considered ub?).

[vitaut]

Dynamic argument support is a contributed experimental API. It does not affect the rest of fmt, but PRs to fix/improve it are welcome.

if the {} are scanned frist and replaced later

Each replacement field is processed as soon as it is parsed, there is no separate whole format string parsing phase (other than compile-time checks). I guess the behavior that you observe is some artefact of dynamic argument support being incomplete.

[Malayka]

Hi! Does bcf3fcd solves the problem? I could miss smth, but now because of setting internal::is_unpacked_bit in types_ here

basic_format_args(const format_arg *args, size_type count)
    : types_(internal::is_unpacked_bit | count) {
    set_data(args);
  }

the args.is_packed() check would always fail here

template <typename Context>
void arg_map<Context>::init(const basic_format_args<Context> &args) {
  if (map_)
    return;
  map_ = new entry[args.max_size()];
  if (args.is_packed()) {
    for (unsigned i = 0;/*nothing*/; ++i) {
      internal::type arg_type = args.type(i);
      switch (arg_type) {
...

so types would be get from format_arg struct, not types_ and everything looks OK.
Original piece of code of the issue works fine in 5.3.0 version.

[vitaut]

Does bcf3fcd solves the problem?

Looks like it does, thanks for checking. I guess the issue can be closed now.