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

Unsafe use of fprintf() and printf() #553

Closed
dvzrv opened this issue Jan 28, 2022 · 7 comments
Closed

Unsafe use of fprintf() and printf() #553

dvzrv opened this issue Jan 28, 2022 · 7 comments

Comments

@dvzrv
Copy link
Contributor

dvzrv commented Jan 28, 2022

Environment

  • OS: Arch Linux
  • Giada version: 0.20.0

Describe the bug
The code does not compile with -Werror=format-security due to unsafe use of fprintf() and printf() in

giada/src/utils/log.h

Lines 71 to 94 in 0feebd6

/* print
A variadic printf-like logging function. Any `std::string` argument will be
automatically transformed into a C-string. */
template <typename... Args>
static void print(const char* format, Args&&... args)
{
if (mode == LOG_MODE_MUTE)
return;
if (mode == LOG_MODE_FILE && stat == true)
{
// Replace any std::string in the arguments by its C-string
std::fprintf(f, format, string_to_c_str(std::forward<Args>(args))...);
#ifdef _WIN32
fflush(f);
#endif
}
else
std::printf(format, string_to_c_str(std::forward<Args>(args))...);
}
} // namespace giada::u::log
#endif

To Reproduce
Compile giada with -Werror=format-security

[ 16%] Building CXX object CMakeFiles/giada.dir/src/core/plugins/pluginHost.cpp.o
/usr/bin/c++ -DJUCE_DEBUG=0 -DJUCE_GLOBAL_MODULE_SETTINGS_INCLUDED=1 -DJUCE_MODAL_LOOPS_PERMITTED=1 -DJUCE_MODULE_AVAILABLE_juce_gui_basics=1 -DJUCE_PLUGINHOST_AU=0 -DJUCE_PLUGINHOST_VST3=1 -DJUCE_STANDALONE_APPLICATION=1 -DJUCE_USE_CURL=0 -DJUCE_WEB_BROWSER=0 -DNDEBUG -DWITH_AUDIO_JACK -DWITH_VST -DWITH_VST3 -D__LINU
X_ALSA__ -D__LINUX_PULSE__ -D__UNIX_JACK__ -I/build/giada/src/giada-0.20.0-src -I/build/giada/src/giada-0.20.0-src/src -I/build/giada/src/giada-0.20.0-src/src/deps/juce/modules -I/build/giada/src/giada-0.20.0-src/src/deps/vst3sdk -I/usr/include/freetype2 -isystem /usr/include/rtmidi -march=x86-64 -mtune=generic -O2 -p
ipe -fno-plt -fexceptions         -Wp,-D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security         -fstack-clash-protection -fcf-protection -Wp,-D_GLIBCXX_ASSERTIONS -flto -Wall -Wextra -Wpedantic -pthread -D__UNIX_JACK__ -D__LINUX_ALSA__ -MD -MT CMakeFiles/giada.dir/src/core/plugins/pluginHost.cpp.o -MF CMakeFiles/gi
ada.dir/src/core/plugins/pluginHost.cpp.o.d -o CMakeFiles/giada.dir/src/core/plugins/pluginHost.cpp.o -c /build/giada/src/giada-0.20.0-src/src/core/plugins/pluginHost.cpp
In file included from /build/giada/src/giada-0.20.0-src/src/core/waveFx.cpp:30:
/build/giada/src/giada-0.20.0-src/src/utils/log.h: In instantiation of ‘void giada::u::log::print(const char*, Args&& ...) [with Args = {}]’:
/build/giada/src/giada-0.20.0-src/src/core/waveFx.cpp:219:16:   required from here
/build/giada/src/giada-0.20.0-src/src/utils/log.h:84:29: error: format not a string literal and no format arguments [-Werror=format-security]
   84 |                 std::fprintf(f, format, string_to_c_str(std::forward<Args>(args))...);
      |                 ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/build/giada/src/giada-0.20.0-src/src/utils/log.h:90:28: error: format not a string literal and no format arguments [-Werror=format-security]
   90 |                 std::printf(format, string_to_c_str(std::forward<Args>(args))...);
      |                 ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /build/giada/src/giada-0.20.0-src/src/core/kernelAudio.cpp:32:
/build/giada/src/giada-0.20.0-src/src/utils/log.h: In instantiation of ‘void giada::u::log::print(const char*, Args&& ...) [with Args = {}]’:
/build/giada/src/giada-0.20.0-src/src/core/kernelAudio.cpp:93:16:   required from here
/build/giada/src/giada-0.20.0-src/src/utils/log.h:84:29: error: format not a string literal and no format arguments [-Werror=format-security]
   84 |                 std::fprintf(f, format, string_to_c_str(std::forward<Args>(args))...);
      |                 ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/build/giada/src/giada-0.20.0-src/src/utils/log.h:90:28: error: format not a string literal and no format arguments [-Werror=format-security]
   90 |                 std::printf(format, string_to_c_str(std::forward<Args>(args))...);
      |                 ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Full build log:
giada-0.20.0-build.log

Expected behavior
Giada can be compiled.

Screenshots
n/a

Additional context
Allowing runtime generated chars as format string is exploitable and bad practice.
https://stackoverflow.com/questions/4419293/warning-format-not-a-string-literal-and-no-format-arguments

This issue blocks a build of 0.20.0 on Arch Linux as -Werror=format-security is part of our default build environment CFLAGS.

@keryell
Copy link
Contributor

keryell commented Jan 29, 2022

This is a false positive since the compiler cannot see the format through giada::u::log::print. Before #402 the compiler was not confused.
That said, a cleaner solution is to use the Pythonesque but type-safe C++20 std::format or in the meantime to depend on https://github.com/fmtlib/fmt
Of course this means modernizing all the debug messages. :-(
Perhaps is it possible to write a clang-tidy or clangd plugin or... to automate things?

@gvnnz
Copy link
Contributor

gvnnz commented Jan 29, 2022

Right. @keryell we'd love to switch to C++20 and std::format. Year 2022 might be the right time to do it...

@musicinmybrain
Copy link
Contributor

Previously: #447

@luzpaz
Copy link
Contributor

luzpaz commented Apr 8, 2022

Just a soft bump to find out if this in progress at all?

@gvnnz
Copy link
Contributor

gvnnz commented Apr 9, 2022

@luzpaz this is actually a duplicate of #447, still waiting to move to C++20. Not super high priority, but planned for v1.0.

@gvnnz gvnnz closed this as completed Apr 9, 2022
@oldcastlehq
Copy link

Why is the issue closed? The issue itself is still there. You can't compile it on Arch, which is a shame. Is there any workaround to make a package for Arch?

@musicinmybrain
Copy link
Contributor

Why is the issue closed? The issue itself is still there. You can't compile it on Arch, which is a shame. Is there any workaround to make a package for Arch?

It looks to me like it was closed simply because #447 is an older report with more discussion.

In Fedora Linux, we are patching in GCC pragmas to selectively disable -Werror=format-security for just this particular case, as suggested in #447 (comment).

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

No branches or pull requests

6 participants