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

Add printf format checking attribute to report_error #227

Closed
wants to merge 4 commits into from

Conversation

ecatmur
Copy link
Contributor

@ecatmur ecatmur commented Jun 14, 2019

On gcc and clang, add __attribute__((__format__)) checking to the report_error function.

Note: clang warns -Wformat-nonliteral (off by default, but good practice) when a format string argument not annotated by __attribute__((__format__)) is passed to vprintf etc.; gcc does not. See https://clang.llvm.org/docs/AttributeReference.html#format .

Plus fixes for bugs exposed by this change:

  • Cast faulting addresses to uintptr_t for formatting as 0x%08lx. If there is an LLP64 platform that uses Posix signals (i.e. not Win64, which uses SEH) this should be changed to use PRIxPTR format specifier.
  • Fix swapped si_code/si_addr (& si_band) format arguments.
  • Add missing %s to format diagnostic information.

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.
@raffienficiaud raffienficiaud added this to the 1.71 milestone Jun 19, 2019
@@ -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() ); }
Copy link
Member

@raffienficiaud raffienficiaud Jul 21, 2019

Choose a reason for hiding this comment

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

I am not sure to understand why this change would be needed.

Copy link
Contributor Author

@ecatmur ecatmur Jul 22, 2019

Choose a reason for hiding this comment

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

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 );
Copy link
Member

@raffienficiaud raffienficiaud Jul 21, 2019

Choose a reason for hiding this comment

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

uintptr_t might be unavailable on some compilers

Copy link
Contributor Author

@ecatmur ecatmur Jul 22, 2019

Choose a reason for hiding this comment

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

You're already using uintptr_t in this file:

uintptr_t /* reserved */)

typedef unsigned uintptr_t;

typedef void* uintptr_t;

@@ -242,6 +242,9 @@ extract( boost::exception const* ex )
//____________________________________________________________________________//

static void
#ifdef __GNUC__
__attribute__((__format__ (__printf__, 3, 0)))
Copy link
Member

@raffienficiaud raffienficiaud Jul 21, 2019

Choose a reason for hiding this comment

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

How can we ensure that this attribute is available for the target compiler?

Copy link
Contributor Author

@ecatmur ecatmur Jul 22, 2019

Choose a reason for hiding this comment

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

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)?

@raffienficiaud
Copy link
Member

@raffienficiaud raffienficiaud commented Jul 21, 2019

Hi,

Thanks for the PR, tests are passing. However I have some comments. Would you please take the time to address those?

Thanks

Edward Catmur added 3 commits Jul 22, 2019
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
@raffienficiaud raffienficiaud added 1.72 and removed 1.71 labels Aug 3, 2019
@raffienficiaud raffienficiaud removed this from the 1.71 milestone Aug 3, 2019
@raffienficiaud raffienficiaud added this to the 1.72 milestone Aug 3, 2019
@raffienficiaud
Copy link
Member

@raffienficiaud raffienficiaud commented Oct 5, 2019

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.

@raffienficiaud raffienficiaud added the next label Oct 5, 2019
@raffienficiaud raffienficiaud self-assigned this Oct 5, 2019
@raffienficiaud raffienficiaud added develop and removed next labels Oct 6, 2019
@raffienficiaud
Copy link
Member

@raffienficiaud raffienficiaud commented Oct 21, 2019

In master, closing. Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants