redpanda-data / redpanda

Redpanda is a streaming data platform for developers. Kafka API compatible. 10x faster. No ZooKeeper. No JVM!
https://redpanda.com
9.65k stars 589 forks source link

security: convert gss::buffer to string for printing #23470

Closed dotnwat closed 1 month ago

dotnwat commented 1 month ago

It seems that in fmt v8 either a copy of gss::buffer wasn't being made, or the explicit conversion operator to std::string_view was being made prior to a copy of the fmt argument.

Now in fmt v9 it looks like fmt is trying to make a copy before any conversion. So this PR just invokves the conversion up front and passes a std::string_view down into fmt.

external/fmt~/include/fmt/core.h:1735:47: error: call to deleted constructor of 'security::gss::buffer'
 1735 |   const auto& arg = arg_mapper<Context>().map(FMT_FORWARD(val));
      |                                               ^~~~~~~~~~~~~~~~
external/fmt~/include/fmt/core.h:204:26: note: expanded from macro 'FMT_FORWARD'
  204 | #define FMT_FORWARD(...) static_cast<decltype(__VA_ARGS__)&&>(__VA_ARGS__)
      |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
external/fmt~/include/fmt/core.h:1777:10: note: in instantiation of function template specialization 'fmt::detail::make_value<fmt::basic_format_context<fmt::appender, char>, security::gss::buffer &>' requested here
 1777 |   return make_value<Context>(val);
      |          ^
external/fmt~/include/fmt/core.h:1899:23: note: in instantiation of function template specialization 'fmt::detail::make_arg<true, fmt::basic_format_context<fmt::appender, char>, fmt::detail::type::custom_type, security::gss::buffer &, 0>' requested here
 1899 |         data_{detail::make_arg<
      |                       ^
external/fmt~/include/fmt/core.h:1918:10: note: in instantiation of function template specialization 'fmt::format_arg_store<fmt::basic_format_context<fmt::appender, char>, vlog::file_line, std::string_view, security::gss::buffer>::format_arg_store<vlog::file_line &, std::string_view &, security::gss::buffer &>' requested here
 1918 |   return {FMT_FORWARD(args)...};
      |          ^
external/fmt~/include/fmt/core.h:3235:36: note: in instantiation of function template specialization 'fmt::make_format_args<fmt::basic_format_context<fmt::appender, char>, vlog::file_line &, std::string_view &, security::gss::buffer &>' requested here
 3235 |   return vformat_to(out, fmt, fmt::make_format_args(args...));
      |                                    ^
external/_main~non_module_dependencies~seastar/include/seastar/util/log.hh:301:33: note: in instantiation of function template specialization 'fmt::format_to<seastar::internal::log_buf::inserter_iterator, vlog::file_line, std::string_view &, security::gss::buffer &, 0>' requested here
  301 |                     return fmt::format_to(it, fmt::runtime(fmt.format), std::forward<Args>(args)...);
      |                                 ^
external/_main~non_module_dependencies~seastar/include/seastar/util/log.hh:423:9: note: in instantiation of function template specialization 'seastar::logger::log<vlog::file_line, std::string_view &, security::gss::buffer &>' requested here
  423 |         log(log_level::info, std::move(fmt), std::forward<Args>(args)...);
      |         ^
src/v/security/gssapi_authenticator.cc:67:25: note: in instantiation of function template specialization 'seastar::logger::info<vlog::file_line, std::string_view &, security::gss::buffer &>' requested here
   67 |             vlog(seclog.info, "GSS_API error {}: {}", m, msg);
      |                         ^
bazel-out/k8-fastbuild/bin/src/v/security/_virtual_includes/security/security/gssapi.h:56:5: note: 'buffer' has been explicitly marked deleted here
   56 |     buffer(const buffer&) = delete;
      |     ^

Backports Required

Release Notes

vbotbuildovich commented 1 month ago

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55063#019225e8-8793-49f6-a2c6-0f035deb28f4

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55063#019225e3-165e-4346-8c38-423b2a9fbd46

dotnwat commented 1 month ago

Change looks fine, but it's a little concerning that there's suddenly a (seemingly unnecessary) copy happening here...and that it was detectable only because the struct in question has a deleted copy constructor.

do we have any way to detect similar cases for copyable structs? should we be using a different fmt API inside logger?

I don't think it was being copied before--the copy constructors are deleted. I think for some reason it's not able to convert into string_view in fmt9 like before, and it's trying some backup code path.

dotnwat commented 1 month ago

@BenPope would still appreciate a look at this when you get a chance.

oleiman commented 1 month ago

I don't think it was being copied before

Ya, that's clear. I'm wondering whether there could be other (copyable) structs quietly taking a copy-incurring backup path in fmt9. The intermediate string_view seems kind of unusual anyway, so maybe it's a non-issue.

BenPope commented 1 month ago

@BenPope would still appreciate a look at this when you get a chance.

Looks good to me.

dotnwat commented 1 month ago

I don't think it was being copied before

Ya, that's clear. I'm wondering whether there could be other (copyable) structs quietly taking a copy-incurring backup path in fmt9. The intermediate string_view seems kind of unusual anyway, so maybe it's a non-issue.

ahh, i see. yeh i dunno. it's one reason why we tend to delete copy constructors on most types so copies will be caught.