From 426231a22bfd93646baaa747f82d0b975804870b Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Fri, 4 Dec 2020 18:52:33 +0100 Subject: [PATCH] Avoid potential UB in loggers using composition This cast would work fine, but any other fields added to those loggers would be left uninitialized. --- meson.build | 2 +- src/common/communication/vst2.h | 3 ++- src/common/communication/vst3.h | 7 ++++--- src/common/logging/vst2.cpp | 16 +++++++++------- src/common/logging/vst2.h | 23 +++++++++++++++++++++-- src/common/logging/vst3.cpp | 19 +++++++++++++++++++ src/common/logging/vst3.h | 20 ++++++++++++++++++-- src/plugin/bridges/vst2.cpp | 4 +--- src/plugin/bridges/vst3.cpp | 4 +--- 9 files changed, 76 insertions(+), 22 deletions(-) create mode 100644 src/common/logging/vst3.cpp diff --git a/meson.build b/meson.build index 7a19cfa9..4c04cf4a 100644 --- a/meson.build +++ b/meson.build @@ -76,7 +76,7 @@ vst2_plugin_sources = [ vst3_plugin_sources = [ 'src/common/communication/common.cpp', 'src/common/logging/common.cpp', - 'src/common/logging/vst2.cpp', + 'src/common/logging/vst3.cpp', 'src/common/configuration.cpp', 'src/common/plugins.cpp', 'src/common/utils.cpp', diff --git a/src/common/communication/vst2.h b/src/common/communication/vst2.h index f3304449..26c038a6 100644 --- a/src/common/communication/vst2.h +++ b/src/common/communication/vst2.h @@ -254,7 +254,8 @@ class EventHandler : public AdHocSocketHandler { }; this->receive_multi( - logging ? std::optional(std::ref(logging->first)) : std::nullopt, + logging ? std::optional(std::ref(logging->first.logger)) + : std::nullopt, [&](boost::asio::local::stream_protocol::socket& socket) { process_event(socket, true); }, diff --git a/src/common/communication/vst3.h b/src/common/communication/vst3.h index ea363aae..d36fdbc2 100644 --- a/src/common/communication/vst3.h +++ b/src/common/communication/vst3.h @@ -174,9 +174,10 @@ class Vst3MessageHandler : public AdHocSocketHandler { write_object(socket, response); }; - this->receive_multi( - logging ? std::optional(std::ref(logging->first)) : std::nullopt, - process_message); + this->receive_multi(logging + ? std::optional(std::ref(logging->first.logger)) + : std::nullopt, + process_message); } }; diff --git a/src/common/logging/vst2.cpp b/src/common/logging/vst2.cpp index 0c2406c7..e03c7662 100644 --- a/src/common/logging/vst2.cpp +++ b/src/common/logging/vst2.cpp @@ -18,6 +18,8 @@ #include +Vst2Logger::Vst2Logger(Logger& generic_logger) : logger(generic_logger) {} + std::optional opcode_to_string(bool is_dispatch, int opcode) { if (is_dispatch) { // Opcodes for a plugin's dispatch function @@ -316,7 +318,7 @@ std::optional opcode_to_string(bool is_dispatch, int opcode) { } void Vst2Logger::log_get_parameter(int index) { - if (BOOST_UNLIKELY(verbosity >= Verbosity::most_events)) { + if (BOOST_UNLIKELY(logger.verbosity >= Logger::Verbosity::most_events)) { std::ostringstream message; message << ">> getParameter() " << index; @@ -325,7 +327,7 @@ void Vst2Logger::log_get_parameter(int index) { } void Vst2Logger::log_get_parameter_response(float value) { - if (BOOST_UNLIKELY(verbosity >= Verbosity::most_events)) { + if (BOOST_UNLIKELY(logger.verbosity >= Logger::Verbosity::most_events)) { std::ostringstream message; message << " getParameter() :: " << value; @@ -334,7 +336,7 @@ void Vst2Logger::log_get_parameter_response(float value) { } void Vst2Logger::log_set_parameter(int index, float value) { - if (BOOST_UNLIKELY(verbosity >= Verbosity::most_events)) { + if (BOOST_UNLIKELY(logger.verbosity >= Logger::Verbosity::most_events)) { std::ostringstream message; message << ">> setParameter() " << index << " = " << value; @@ -343,7 +345,7 @@ void Vst2Logger::log_set_parameter(int index, float value) { } void Vst2Logger::log_set_parameter_response() { - if (BOOST_UNLIKELY(verbosity >= Verbosity::most_events)) { + if (BOOST_UNLIKELY(logger.verbosity >= Logger::Verbosity::most_events)) { log(" setParameter() :: OK"); } } @@ -355,7 +357,7 @@ void Vst2Logger::log_event(bool is_dispatch, const EventPayload& payload, float option, const std::optional& value_payload) { - if (BOOST_UNLIKELY(verbosity >= Verbosity::most_events)) { + if (BOOST_UNLIKELY(logger.verbosity >= Logger::Verbosity::most_events)) { if (should_filter_event(is_dispatch, opcode)) { return; } @@ -442,7 +444,7 @@ void Vst2Logger::log_event_response( intptr_t return_value, const EventResultPayload& payload, const std::optional& value_payload) { - if (BOOST_UNLIKELY(verbosity >= Verbosity::most_events)) { + if (BOOST_UNLIKELY(logger.verbosity >= Logger::Verbosity::most_events)) { if (should_filter_event(is_dispatch, opcode)) { return; } @@ -512,7 +514,7 @@ void Vst2Logger::log_event_response( } bool Vst2Logger::should_filter_event(bool is_dispatch, int opcode) const { - if (verbosity >= Verbosity::all_events) { + if (logger.verbosity >= Logger::Verbosity::all_events) { return false; } diff --git a/src/common/logging/vst2.h b/src/common/logging/vst2.h index e02b1f16..2fe1d60c 100644 --- a/src/common/logging/vst2.h +++ b/src/common/logging/vst2.h @@ -33,10 +33,24 @@ std::optional opcode_to_string(bool is_dispatch, int opcode); /** - * Provides VST2 specific logging functionality for debugging plugins. + * Wraps around `Logger` to provide VST2 specific logging functionality for + * debugging plugins. This way we can have all the complex initialisation be + * performed in one place. */ -class Vst2Logger : public Logger { +class Vst2Logger { public: + Vst2Logger(Logger& generic_logger); + + /** + * @see Logger::log + */ + inline void log(const std::string& message) { logger.log(message); } + + /** + * @see Logger::log_trace + */ + inline void log_trace(const std::string& message) { logger.log(message); } + // The following functions are for logging specific events, they are only // enabled for verbosity levels higher than 1 (i.e. `Verbosity::events`) void log_get_parameter(int index); @@ -60,6 +74,11 @@ class Vst2Logger : public Logger { const EventResultPayload& payload, const std::optional& value_payload); + /** + * The underlying logger instance we're wrapping. + */ + Logger& logger; + private: /** * Determine whether an event should be filtered based on the current diff --git a/src/common/logging/vst3.cpp b/src/common/logging/vst3.cpp new file mode 100644 index 00000000..774954a2 --- /dev/null +++ b/src/common/logging/vst3.cpp @@ -0,0 +1,19 @@ +// yabridge: a Wine VST bridge +// Copyright (C) 2020 Robbert van der Helm +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +#include "vst3.h" + +Vst3Logger::Vst3Logger(Logger& generic_logger) : logger(generic_logger) {} diff --git a/src/common/logging/vst3.h b/src/common/logging/vst3.h index 8a22a848..229e9003 100644 --- a/src/common/logging/vst3.h +++ b/src/common/logging/vst3.h @@ -19,9 +19,25 @@ #include "common.h" /** - * Provides VST3-specific logging functionality for debugging plugins. + * Wraps around `Logger` to provide VST3 specific logging functionality for + * debugging plugins. This way we can have all the complex initialisation be + * performed in one place. */ -class Vst3Logger : public Logger { +class Vst3Logger { public: + Vst3Logger(Logger& generic_logger); + + /** + * @see Logger::log + */ + inline void log(const std::string& message) { logger.log(message); } + + /** + * @see Logger::log_trace + */ + inline void log_trace(const std::string& message) { logger.log(message); } + // TODO: Logging interface for VST3 plugins + + Logger& logger; }; diff --git a/src/plugin/bridges/vst2.cpp b/src/plugin/bridges/vst2.cpp index 70e64351..e4a07e60 100644 --- a/src/plugin/bridges/vst2.cpp +++ b/src/plugin/bridges/vst2.cpp @@ -55,9 +55,7 @@ Vst2PluginBridge::Vst2PluginBridge(audioMasterCallback host_callback) // bridge will crash otherwise plugin(), host_callback_function(host_callback), - // TODO: This is UB, use composition with `generic_logger` instead - logger(static_cast(Logger::create_from_environment( - create_logger_prefix(sockets.base_dir)))) { + logger(generic_logger) { log_init_message(); // This will block until all sockets have been connected to by the Wine VST diff --git a/src/plugin/bridges/vst3.cpp b/src/plugin/bridges/vst3.cpp index fbc5dc54..c8755d55 100644 --- a/src/plugin/bridges/vst3.cpp +++ b/src/plugin/bridges/vst3.cpp @@ -27,9 +27,7 @@ Vst3PluginBridge::Vst3PluginBridge() .string()), true); }), - // TODO: This is UB, use composition with `generic_logger` instead - logger(static_cast(Logger::create_from_environment( - create_logger_prefix(sockets.base_dir)))) { + logger(generic_logger) { log_init_message(); // This will block until all sockets have been connected to by the Wine VST