Possible use-after-move in join()

[mattgodbolt]

The code at:

fmt/include/fmt/ranges.h

Lines 661 to 666 in 720da57

	auto format(view_ref& value, FormatContext& ctx) const
	-> decltype(ctx.out()) {
	auto it = std::forward<view_ref>(value).begin;
	auto out = ctx.out();
	if (it == value.end) return out;
	out = value_formatter_.format(*it, ctx);

Does a

auto it = std::forward<view_ref>(value).begin;

and later accesses value.end and value.sep.

In the case the iterator is not copy constructible then the view_ref is an rvalue-reference:

  using view_ref = conditional_t<std::is_copy_constructible<It>::value,
                                 const join_view<It, Sentinel, Char>&,
                                 join_view<It, Sentinel, Char>&&>;

which effectively turns the forward<> into a move.

clang-tidy warns on the accesses of end and sep as they are accesses on a moved-from object.

I'm not sure that this is really a problem in practice but it seems like something that should be avoided. I can't see an easy way to do so though. I'm not sure we can cast just the iterator easily? Taking a copy of sep works but for the same reasons as above we can't copy end out without a move either.

[mattgodbolt]

I'm a bit out of my depth but maybe something like:

    auto value_copy = std::forward<view_ref>(value);
    auto it = std::move(value_copy.begin);
    // rest of the code uses value_copy

though this forces a copy even in the good case.

[mattgodbolt]

CE link: https://compiler-explorer.com/z/8q9oj5v31

[Arghnews]

I had a look at this.

I believe this specialization is only used for exactly fmt:join_views.

If you try and pass an lvalue fmt::join_view, you hit a static assert saying we can only pass rvalue views (https://compiler-explorer.com/z/9jjPodzzo)

So now we can be sure that this code path only deals with receiving fmt::joins that are rvalues.

This format function is called from here, line 2224:

fmt/include/fmt/base.h

Lines 2214 to 2225 in 720da57

	// Formats an argument of a custom type, such as a user-defined class.
	template <typename T, typename Formatter>
	static void format_custom(void* arg, parse_context<char_type>& parse_ctx,
	Context& ctx) {
	auto f = Formatter();
	parse_ctx.advance_to(f.parse(parse_ctx));
	using qualified_type =
	conditional_t<has_formatter<const T, char_type>(), const T, T>;
	// format must be const for compatibility with std::format and compilation.
	const auto& cf = f;
	ctx.advance_to(cf.format(*static_cast<qualified_type*>(arg), ctx));
	}

qualified_type on line 2220 in that block is always non const (for a fmt::join_view). See:
https://compiler-explorer.com/z/cK3h3EzeP

This means that line 2224's first argument passed to format is:
*static_cast<qualified_type*>(arg) which will always deduce to fmt::join_view&

 

A minimal fix is to change fmt::join_view::format to only take lvalues, since as it stands, that's all it will ever receive, and change it to it&:

  template <typename FormatContext>
  auto format(join_view<It, Sentinel, Char>& value, FormatContext& ctx) const
      -> decltype(ctx.out()) {
    auto& it = value.begin;

This also saves a copy/move since we now take it by reference. The reference being into the original rvalue argument that is passed in the println call.

 

Note this code was originally put in in 10508a30ecd91e5d09a27e4c6c0a01a89fd4edc7 and changed to std::forward in d2473b7b73c0af2a3ed34c99e50ace0a1040581a

If I've missed anything or my assumptions or wrong, please point it out

 

I can put in a PR for this if vitaut agrees this is a good change to make
@vitaut

[vitaut]

Thanks @mattgodbolt for reporting and @Arghnews for investigating the issue.

A minimal fix is to change fmt::join_view::format to only take lvalues, since as it stands, that's all it will ever receive, and change it to it&

I think this is a correct fix.

I can put in a PR for this if vitaut agrees this is a good change to make

Please do.

[mattgodbolt]

Thank you both!