Use GCC extension for printf-like functions #154
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit annotates rcutils_format_string_limit, rcutils_log
and rcutils_snprintf with the GCC extension for printf-like
statements.
Annotating this function allows GCC to emit warnings when dangerous
patterns are used through the whole codebase.
As those functions are wrappers around different flavors of printf,
surfacing those warnings prevents dangerous printf statements.
For instance:
const char* message = // some dynamically evaluated logging message
printf(message);
...is unsafe. Instead, we should be using:
printf("%s", message);
...to avoid crashes if the variable "message" contains a sequence
of characters printf may recognize (like "%d").
printf-related warnings are enabled by '-Wformat*' flags ('-Wformat',
'-Wformat-security', etc.).
The complete list of flags and detected patterns is available in
the GCC documentation:
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
Note: clang does also support this attribute but not MSVC. On
Windows, no additional safety is provided by this commit.
Signed-off-by: Thomas Moulard <tmoulard@amazon.com>
thomas-moulard
commented
Apr 27, 2019
| @@ -113,15 +113,15 @@ TEST(CLASSNAME(TestLogging, RMW_IMPLEMENTATION), test_logging) { | |||
| EXPECT_EQ("name3", g_last_log_event.name); | |||
| EXPECT_EQ("message 33", g_last_log_event.message); | |||
|
|
|||
| rcutils_log(NULL, RCUTILS_LOG_SEVERITY_WARN, "", ""); | |||
| rcutils_log(NULL, RCUTILS_LOG_SEVERITY_WARN, "", "%s", ""); | |||
There was a problem hiding this comment.
This prevents GCC emitting warning (see -Wformat-zero-length)
dirk-thomas
reviewed
Apr 29, 2019
Signed-off-by: Thomas Moulard <tmoulard@amazon.com>
|
Signed-off-by: Thomas Moulard <tmoulard@amazon.com>
|
|
@dirk-thomas Good to go on my side 🚢 |
dirk-thomas
added
enhancement
|
Thanks for the improvement. |
nuclearsandwich
added a commit
to ros/pluginlib
that referenced
this pull request
May 1, 2019
With ros2/rcutils#154 warnings are emited for "dangerous patterns" in format strings. %p expects a void * argument so a void * argument we shall provide. Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>
nuclearsandwich
added a commit
to ros/pluginlib
that referenced
this pull request
May 1, 2019
With ros2/rcutils#154 warnings are emitted for "dangerous patterns" in format strings. %p expects a void * argument so a void * argument we shall provide. Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>
This was referenced May 1, 2019
|
@thomas-moulard This patch broke all nightly builds. Please run more through tests in the future. ros/pluginlib#152 will hopefully fix the builds. |
nuclearsandwich
added a commit
to ros2/message_filters
that referenced
this pull request
May 1, 2019
This resolves warnings which popped up as a result of ros2/rcutils#154.
clalancette
pushed a commit
to ros2/message_filters
that referenced
this pull request
May 1, 2019
* Make format string agree with argument type. This resolves warnings which popped up as a result of ros2/rcutils#154. * Switch to PRId64. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
thomas-moulard
pushed a commit
to thomas-moulard/rosbag2
that referenced
this pull request
May 2, 2019
GCC now warns us that log message should not be passed as format string. This is a dangerous pattern because if the log message contains a sequence of characters printf will interpret such as "%d", the program may crash. Signed-off-by: Thomas Moulard <tmoulard@amazon.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit annotates rcutils_format_string_limit, rcutils_log
and rcutils_snprintf with the GCC extension for printf-like
statements.
Annotating this function allows GCC to emit warnings when dangerous
patterns are used through the whole codebase.
As those functions are wrappers around different flavors of printf,
surfacing those warnings prevents dangerous printf statements.
For instance:
...is unsafe. Instead, we should be using:
...to avoid crashes if the variable "message" contains a sequence
of characters printf may recognize (like "%d").
printf-related warnings are enabled by '-Wformat*' flags ('-Wformat',
'-Wformat-security', etc.).
The complete list of flags and detected patterns is available in
the GCC documentation:
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
Note: clang does also support this attribute but not MSVC. On
Windows, no additional safety is provided by this commit.
Signed-off-by: Thomas Moulard tmoulard@amazon.com