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

High impact Coverity finding in include/fmt/format.h #3694

Closed
carlsmedstad opened this issue Oct 31, 2023 · 2 comments · Fixed by #3695
Closed

High impact Coverity finding in include/fmt/format.h #3694

carlsmedstad opened this issue Oct 31, 2023 · 2 comments · Fixed by #3695

Comments

@carlsmedstad
Copy link
Contributor

Coverity Static Analysis v2023.6.1 is reporting the following finding in include/fmt/format.h#L1959:

template <typename Char, typename OutputIt>
auto write_escaped_char(OutputIt out, Char v) -> OutputIt {
  *out++ = static_cast<Char>('\'');
// 1. Condition fmt::v10::detail::needs_escape(static_cast<uint32_t>(v)), taking true branch.
// 2. Condition v != '"' /* static_cast<char>('"') */, taking true branch.
  if ((needs_escape(static_cast<uint32_t>(v)) && v != static_cast<Char>('"')) ||
      v == static_cast<Char>('\'')) {
// 3. address_of Taking address with &v yields a singleton pointer.
// CID 145888: (#1 of 1): Out-of-bounds access (ARRAY_VS_SINGLETON)
// 4. ptr_arith Using &v as an array. This might corrupt or misinterpret adjacent memory locations.
    out = write_escaped_cp(
        out, find_escape_result<Char>{&v, &v + 1, static_cast<uint32_t>(v)});
  } else {
    *out++ = v;
  }
  *out++ = static_cast<Char>('\'');
  return out;
}

Found the following fix but not confident it is upstreamable:

diff --git a/include/fmt/format.h b/include/fmt/format.h
index a98e41d9..4104d91f 100644
--- a/include/fmt/format.h
+++ b/include/fmt/format.h
@@ -1958,11 +1958,12 @@ auto write_escaped_string(OutputIt out, basic_string_view<Char> str)

 template <typename Char, typename OutputIt>
 auto write_escaped_char(OutputIt out, Char v) -> OutputIt {
+  Char v_array[1] = {v};
   *out++ = static_cast<Char>('\'');
   if ((needs_escape(static_cast<uint32_t>(v)) && v != static_cast<Char>('"')) ||
       v == static_cast<Char>('\'')) {
     out = write_escaped_cp(
-        out, find_escape_result<Char>{&v, &v + 1, static_cast<uint32_t>(v)});
+        out, find_escape_result<Char>{v_array, v_array + 1, static_cast<uint32_t>(v)});
   } else {
     *out++ = v;
   }

Thought I'd create an issue at least. Thanks!

@vitaut
Copy link
Contributor

vitaut commented Oct 31, 2023

The change looks reasonable. Could you submit a PR?

@carlsmedstad
Copy link
Contributor Author

Sure! See #3695.

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 a pull request may close this issue.

2 participants