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.

Sign up for GitHub

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

Prevent Application Crash when Numeric Config Entries Contain Dynamic String Interpolations #96

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Prevent Application Crash when Numeric Config Entries Contain Dynamic String Interpolations #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54dd828
Do not crash with `Poco::SyntaxException` if encountering a property,…
MadMartian Mar 11, 2025
800ac76
This property reference "device" seems incorrect, elsewhere in the re…
MadMartian Mar 11, 2025
1fa9a86
Enhancement, cover `SettingsWindow::IntegerSettingVec` integer retrie…
MadMartian Mar 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/AudioCapture.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ int AudioCapture::GetInitialAudioDeviceIndex(const AudioDeviceMap& deviceList)
// Check if configured device is a number or string
try
{
audioDeviceIndex = _config->getInt("device", -1);
audioDeviceIndex = _config->getInt("audio.device", -1);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found references to "audio.device" elsewhere, so I presumed "device" was not correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is correct, as the audio subsystem only gets a view of the audio config key, thus only the subkeys are used to access the data. See this line here:

frontend-sdl-cpp/src/AudioCapture.cpp

Line 20 in 9d93ead

_config = app.config().createView("audio");

if (deviceList.find(audioDeviceIndex) == deviceList.end())
{
poco_debug(_logger,
Expand All @@ -131,7 +131,7 @@ int AudioCapture::GetInitialAudioDeviceIndex(const AudioDeviceMap& deviceList)
}
catch (Poco::SyntaxException& ex)
{
auto audioDeviceName = _config->getString("device", "");
auto audioDeviceName = _config->getString("audio.device", "");

poco_debug_f1(_logger, R"(audio.device is set to non-numerical value. Searching for device name "%s".)",
audioDeviceName);
Expand All @@ -150,4 +150,4 @@ int AudioCapture::GetInitialAudioDeviceIndex(const AudioDeviceMap& deviceList)
deviceList.at(audioDeviceIndex), audioDeviceIndex);

return audioDeviceIndex;
}
}
6 changes: 3 additions & 3 deletions src/gui/ProjectMGUI.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,14 +124,14 @@ class ProjectMGUI : public Poco::Util::Subsystem

float _textScalingFactor{0.0f}; //!< The text scaling factor.

Poco::Logger& _logger{Poco::Logger::get("ProjectMGUI")}; //!< The class logger.

MainMenu _mainMenu{*this};
SettingsWindow _settingsWindow{*this}; //!< The settings window.
SettingsWindow _settingsWindow{*this, _logger}; //!< The settings window.
Comment on lines +127 to +130
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: Changing field order might introduce some unintended side-effects, depending on the compiler.

AboutWindow _aboutWindow{*this}; //!< The about window.
HelpWindow _helpWindow; //!< Help window with shortcuts and tips.

std::unique_ptr<ToastMessage> _toast; //!< Current toast to be displayed.

bool _visible{false}; //!< Flag for settings window visibility.

Poco::Logger& _logger{Poco::Logger::get("ProjectMGUI")}; //!< The class logger.
};
28 changes: 24 additions & 4 deletions src/gui/SettingsWindow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,14 @@
#include <Poco/NotificationCenter.h>

#include <Poco/Util/Application.h>
#include <Poco/Exception.h>

SettingsWindow::SettingsWindow(ProjectMGUI& gui)
SettingsWindow::SettingsWindow(ProjectMGUI& gui, Poco::Logger & logger)
: _gui(gui)
, _audioCapture(ProjectMSDLApplication::instance().getSubsystem<AudioCapture>())
, _userConfiguration(ProjectMSDLApplication::instance().UserConfiguration())
, _commandLineConfiguration(ProjectMSDLApplication::instance().CommandLineConfiguration())
Comment on lines +18 to 22
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass logger from parent

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to pass loggers around. Each class creates its own Logger instance with a distinct name. If multiple instances of a class exist, they share the same instance.

See this example:

frontend-sdl-cpp/src/AudioCapture.h

Line 87 in 9d93ead

Poco::Logger& _logger{ Poco::Logger::get("AudioCapture") }; //!< The class logger.

Copy link
Member

@kblaschke kblaschke Mar 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loggers can also have hierarchical names, e.g. GUI.SettingsWindow. In larger applications, this makes it possible to set specific logging options for a whole subsystem, as all loggers under GUI would get the same config if you set it for this path.

I'd recommend reading the documentation on loggers, subsystems and applications on pocoproject.com, it's quite sophisticated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, the logger in the SettingsWindow class should be declared in the header as:

    Poco::Logger& _logger{ Poco::Logger::get("SettingsWindow") }; //!< The class logger.

You can then refer to it as usual.

, _logger(logger)
{
}

Expand Down Expand Up @@ -387,7 +389,7 @@ void SettingsWindow::IntegerSetting(const std::string& property, int defaultValu
{
ImGui::TableSetColumnIndex(1);

auto value = _userConfiguration->getInt(property, defaultValue);
auto value = _safeInt(property, defaultValue);

if (ImGui::SliderInt(std::string("##integer_" + property).c_str(), &value, min, max))
{
Expand All @@ -408,8 +410,9 @@ void SettingsWindow::IntegerSettingVec(const std::string& property1, const std::
ImGui::TableSetColumnIndex(1);

int values[2] = {
_userConfiguration->getInt(property1, defaultValue1),
_userConfiguration->getInt(property2, defaultValue2)};
_safeInt(property1, defaultValue1),
_safeInt(property2, defaultValue2)
};
Comment on lines 412 to +415
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this "_safeInt" function, it'd be way easier to simply catch the exception and use the default values if something is wrong, like this:

    int values[2];
    try
    {
        values[0] = _userConfiguration->getInt(property1, defaultValue1);
        values[1] = _userConfiguration->getInt(property2, defaultValue2);
    }
    catch (Poco::SyntaxException&)
    {
        values[0] = defaultValue1;
        values[1] = defaultValue2;
    }

This will also make sure that both values are using the default if any property fails, and not an undefined mix of both.
Initialization isn't strictly necessary because either code path sets both values if successful.


if (ImGui::SliderInt2(std::string("##integer_" + property1 + property2).c_str(), values, min, max))
{
Expand Down Expand Up @@ -561,3 +564,20 @@ void SettingsWindow::OverriddenSettingMarker()
ImGui::EndTooltip();
}
}

int SettingsWindow::_safeInt(const std::string& property, int defaultValue) {
int value = defaultValue;

try {
value = _userConfiguration->getInt(property, defaultValue);
} catch (Poco::SyntaxException & ex) {
if (_suppressPropertyWarnings.find(property) == _suppressPropertyWarnings.end()) {
_suppressPropertyWarnings.insert(property);
_logger.warning(
"Encountered a non-integral property for '" + property + "', defaulting to zero and it will be clobbered if settings are saved."
);
}
}
return value;
}

7 changes: 6 additions & 1 deletion src/gui/SettingsWindow.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include <Poco/Util/MapConfiguration.h>
#include <Poco/Util/PropertyFileConfiguration.h>
#include <Poco/Logger.h>

#include <string>

Expand All @@ -15,7 +16,7 @@ class SettingsWindow
public:
SettingsWindow() = delete;

explicit SettingsWindow(ProjectMGUI& gui);
explicit SettingsWindow(ProjectMGUI& gui, Poco::Logger & logger);

/**
* @brief Displays the settings window.
Expand Down Expand Up @@ -131,9 +132,13 @@ class SettingsWindow

bool _visible{false}; //!< Window visibility flag.
bool _changed{false}; //!< true if the user changed any setting since the last save.
int _safeInt(const std::string& property, int defaultValue);

Poco::AutoPtr<Poco::Util::PropertyFileConfiguration> _userConfiguration;
Poco::AutoPtr<Poco::Util::MapConfiguration> _commandLineConfiguration;
std::set<std::string> _suppressPropertyWarnings;

Poco::Logger & _logger;
Comment on lines +139 to +141
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::set<std::string> _suppressPropertyWarnings;
Poco::Logger & _logger;
Poco::Logger& _logger{ Poco::Logger::get("SettingsWindow") }; //!< The class logger.


FileChooser _pathChooser{FileChooser::Mode::Directory}; //!< The file chooser dialog to select preset and texture paths.
};