Add printf format checking attribute to report_error #227
Conversation
On gcc and clang, add __attribute__((__format__)) checking to the report_error function. Cast faulting addresses to uintptr_t for formatting as 0x%08lx - this won't work on LLP64, but Win64 uses SEH anyway. Fix swapped si_code/si_addr (& si_band) format arguments. Add missing %s to format diagnostic information.
| @@ -1298,7 +1307,7 @@ execution_monitor::execute( boost::function<int ()> const& F ) | |||
| #if defined(BOOST_NO_TYPEID) || defined(BOOST_NO_RTTI) | |||
| "unknown boost::exception" ); } | |||
| #else | |||
| boost::diagnostic_information(ex).c_str() ); } | |||
| "%s", boost::diagnostic_information(ex).c_str() ); } | |||
There was a problem hiding this comment.
I am not sure to understand why this change would be needed.
There was a problem hiding this comment.
It's a security issue as well as a correctness issue:
gcc.compile.c++ ../../../bin.v2/libs/test/build/gcc-7/debug/link-static/threading-multi/visibility-hidden/execution_monitor.o
In file included from ../../../libs/test/src/execution_monitor.cpp:16:0:
../../../boost/test/impl/execution_monitor.ipp: In member function ‘int boost::execution_monitor::execute(const boost::function<int()>&)’:
../../../boost/test/impl/execution_monitor.ipp:1311:73: error: format not a string literal and no format arguments [-Werror=format-security]
boost::diagnostic_information(ex).c_str() ); }
^
cc1plus: some warnings being treated as errors
I'll add a test illustrating this.
| break; | ||
| case BUS_OBJERR: | ||
| report_error( execution_exception::system_fatal_error, | ||
| "memory access violation at address: 0x%08lx: object specific hardware error", | ||
| m_sig_info->si_addr ); | ||
| (uintptr_t) m_sig_info->si_addr ); |
There was a problem hiding this comment.
uintptr_t might be unavailable on some compilers
There was a problem hiding this comment.
You're already using uintptr_t in this file:
| @@ -242,6 +242,9 @@ extract( boost::exception const* ex ) | |||
| //____________________________________________________________________________// | |||
|
|
|||
| static void | |||
| #ifdef __GNUC__ | |||
| __attribute__((__format__ (__printf__, 3, 0))) | |||
There was a problem hiding this comment.
How can we ensure that this attribute is available for the target compiler?
There was a problem hiding this comment.
I'll change the check to __GNUC__ >= 3. Or would you prefer to set this only for specific compilers (I've tested with gcc 6-8 and clang 5-9)?
|
Hi, Thanks for the PR, tests are passing. However I have some comments. Would you please take the time to address those? Thanks |
If it is passed unescaped, the '%%' will be condensed to a single '%' and the test will fail.
Format attribute was introduced sometime during gcc 2.8, so 3 definitely has it. Same should apply for any compiler claiming GNUC compatibility
|
Thank you for having addressed all the comments and for the quality of the work. This is currently in next and should be merged to develop pretty soon. |
|
In master, closing. Thanks! |
On gcc and clang, add
__attribute__((__format__))checking to thereport_errorfunction.Note: clang warns
-Wformat-nonliteral(off by default, but good practice) when a format string argument not annotated by__attribute__((__format__))is passed tovprintfetc.; gcc does not. See https://clang.llvm.org/docs/AttributeReference.html#format .Plus fixes for bugs exposed by this change:
uintptr_tfor formatting as0x%08lx. If there is an LLP64 platform that uses Posix signals (i.e. not Win64, which uses SEH) this should be changed to usePRIxPTRformat specifier.si_code/si_addr(&si_band) format arguments.%sto format diagnostic information.