fails to compile with format-security compiler flags

[ghost]

Environment

• OS: NixOS 20.05pre
• Giada version: 0.17.1

Describe the bug I ran into the following error while compiling giada:

/build/source/src/utils/log.h:87:15: error: format not a string literal and no format arguments [8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-security-Werror=format-security8;;]
   87 |   std::fprintf(f, format, string_to_c_str(std::forward<Args>(args))...);
      |   ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/build/source/src/utils/log.h:93:14: error: format not a string literal and no format arguments [8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-security-Werror=format-security8;;]
   93 |   std::printf(format, string_to_c_str(std::forward<Args>(args))...);
      |   ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

To Reproduce Steps to reproduce the behavior:

1. Try to compile giada 0.17.1 with -Wformat-security -Werror=format-security

Expected behavior The program compiles

Screenshots N/A

Additional context I'm the maintainer of the NixOS giada package. I ran into this error while updating to 0.17.1 (from 0.16.x).

[gvnnz]

Hi @petabyteboy , thanks for reporting. This is a kind of known issue after we C++17-ified the log function. I'll try to summon @keryell to see if we can suppress that warning.

[keryell]

It is not obvious how to fix this because as the documentation pointed says:

-Wformat-security

If -Wformat is specified, also warn about uses of format functions that represent possible security problems. At present, this warns about calls to printf and scanf functions where the format string is not a string literal and there are no format arguments, as in printf (foo); This may be a security hole if the format string came from untrusted input and contains ‘%n’. (This is currently a subset of what -Wformat-nonliteral warns about, but in future warnings may be added to -Wformat-security that are not included in -Wformat-nonliteral.)

It is a "possible security problem" but I think it is a false positive. The real fix is to avoid using unsafe C printf at the first place since it is not type safe. I looked at how was the code before: there was a vfprintf(f, format, args); instead of a std::fprintf(), which is even less secure and even more difficult to statically analyze, so this is why I guess that g++ does not even dare to barf about it...

Can we avoid using this -Wformat-security here by removing it for the file or just this line with a #pragma? Otherwise removing the printf and use C++20 std::format? :-) Or calling explicitly vfprintf as before to be even more insecure but without a warning?

[gvnnz]

Otherwise removing the printf and use C++20 std::format? :-)

This would be great, unfortunately we are not ready for the C++20 upgrade yet.

Or calling explicitly vfprintf as before to be even more insecure but without a warning?

Sounds like a good temporary trade-off to me...

[musicinmybrain]

Fedora Linux also compiles with -Werror=format-security.

If you are confident this is a false positive, you can suppress the diagnostic with a GCC pragma. I think the compilers with -Wformat-security are mostly the same ones that implement GCC diagnostic pragmas.

diff -Naur giada-0.17.1-src-original/src/utils/log.h giada-0.17.1-src/src/utils/log.h
--- giada-0.17.1-src-original/src/utils/log.h	2021-02-01 12:41:42.000000000 -0500
+++ giada-0.17.1-src/src/utils/log.h	2021-02-19 15:59:03.741888609 -0500
@@ -84,6 +84,8 @@
 
 	if (mode == LOG_MODE_FILE && stat == true) {
 		// Replace any std::string in the arguments by its C-string
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wformat-security"
 		std::fprintf(f, format, string_to_c_str(std::forward<Args>(args))...);
 #ifdef _WIN32
 		fflush(f);
@@ -91,6 +93,7 @@
 	}
 	else
 		std::printf(format, string_to_c_str(std::forward<Args>(args))...);
+#pragma GCC diagnostic pop
 }
 } // giada::u::log

[keryell]

I have just modernized the code around the printf, but not enough to get rid of the printf completely unfortunately. There are already cleaner alternatives like https://github.com/gabime/spdlog so I do not think more developments here are valuable. The #pragma way is also understood by Clang in the meantime... https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas

[gvnnz]

On a second thought, better avoid trade-offs and fix this one with the proper std::format. Postponed to 1.0.

[keryell]

Yes. And perhaps it is possible to write a small script to update most of the current log API to something more modern and adjust manually the remainers? Or dive into https://clang.llvm.org/extra/clang-tidy/ ? Look at "Using clang-tidy for customized checkers and large scale source tree refactoring - V. Bridger" on https://llvm.org/devmtg/2020-09/program/ if you are motivated. :-)

[gvnnz]

Recently we added fmt as a new dependency, so we don't really need to wait for the official std::format support to fix this.

[keryell]

Great!