fmtlib / fmt

A modern formatting library
https://fmt.dev
Other
20.76k stars 2.5k forks source link

Possible use-after-move in join() #4231

Open mattgodbolt opened 3 hours ago

mattgodbolt commented 3 hours ago

The code at:

https://github.com/fmtlib/fmt/blob/720da57baba83b3b1829e20133575e57aa1a8a4f/include/fmt/ranges.h#L661-L666

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 commented 3 hours ago

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 commented 3 hours ago

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