Bug 80063 - gcc/asan.c: PVS-Studio: Incorrect Block Delimitation (CWE-483)
Summary: gcc/asan.c: PVS-Studio: Incorrect Block Delimitation (CWE-483)
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: sanitizer (show other bugs)
Version: 7.0.1
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2017-03-16 08:14 UTC by Phillip Khandeliants
Modified: 2017-03-20 13:53 UTC (History)
7 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-03-16 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Phillip Khandeliants 2017-03-16 08:14:36 UTC
We have found a weakness (CWE-483) using PVS-Studio tool. PVS-Studio is a static code analyzer for C, C++ and C#: https://www.viva64.com/en/pvs-studio/

Analyzer warning: V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. asan.c 2582

void initialize_sanitizer_builtins (void)
{
  ....
  #define DEF_SANITIZER_BUILTIN(ENUM, NAME, TYPE, ATTRS) \
  decl = add_builtin_function ("__builtin_" NAME, TYPE, ENUM,		\
			       BUILT_IN_NORMAL, NAME, NULL_TREE);	\
  set_call_expr_flags (decl, ATTRS);					\
  set_builtin_decl (ENUM, decl, true);

  #include "sanitizer.def"

  /* -fsanitize=object-size uses __builtin_object_size, but that might
     not be available for e.g. Fortran at this point.  We use
     DEF_SANITIZER_BUILTIN here only as a convenience macro.  */
  if ((flag_sanitize & SANITIZE_OBJECT_SIZE)
      && !builtin_decl_implicit_p (BUILT_IN_OBJECT_SIZE))
    DEF_SANITIZER_BUILTIN (BUILT_IN_OBJECT_SIZE, "object_size",         // <=
			   BT_FN_SIZE_CONST_PTR_INT,
			   ATTR_PURE_NOTHROW_LEAF_LIST)
  ....
}

The conditional operator covers only the first expression of the macro, the other two expressions will always be executed. Perhaps this is a mistake, the macro should be enclosed in braces.
Comment 1 Martin Sebor 2017-03-16 20:56:33 UTC
This does look wrong.  Macros shouldn't expand to multiple statements.  The conditional was introduced in r218084, the macro itself in r194103.  I CC the author of the former.
Comment 2 Manuel López-Ibáñez 2017-03-17 01:10:57 UTC
This is also a missing warning from -Wmisleading-indentation
Comment 3 Marek Polacek 2017-03-20 13:06:11 UTC
Guess we should
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -2566,11 +2566,12 @@ initialize_sanitizer_builtins (void)
 #undef DEF_BUILTIN_STUB
 #define DEF_BUILTIN_STUB(ENUM, NAME)
 #undef DEF_SANITIZER_BUILTIN
-#define DEF_SANITIZER_BUILTIN(ENUM, NAME, TYPE, ATTRS) \
+#define DEF_SANITIZER_BUILTIN(ENUM, NAME, TYPE, ATTRS) do {        \
   decl = add_builtin_function ("__builtin_" NAME, TYPE, ENUM,      \
                   BUILT_IN_NORMAL, NAME, NULL_TREE);   \
   set_call_expr_flags (decl, ATTRS);                   \
-  set_builtin_decl (ENUM, decl, true);
+  set_builtin_decl (ENUM, decl, true);                 \
+} while (0);
 
 #include "sanitizer.def"
Comment 4 Jakub Jelinek 2017-03-20 13:18:49 UTC
Please reformat it properly:
#define DEF_SANITIZER_BUILTIN(ENUM, NAME, TYPE, ATTRS) \
  do {                                                              \
    decl = add_builtin_function ("__builtin_" NAME, TYPE, ENUM,     \
                                 BUILT_IN_NORMAL, NAME, NULL_TREE); \
    set_call_expr_flags (decl, ATTRS);                              \
    set_builtin_decl (ENUM, decl, true);                            \
  } while (0);
Ok with that change.
Comment 5 Jakub Jelinek 2017-03-20 13:21:05 UTC
As for the warning, we should open an enhancement request, though not sure it is something for -Wmisleading-indentation.  I'd say that we just should warn whenever a macro defines several statements and the macro is used as a body of a conditional, so only the first statement from the macro is conditional.
Comment 6 Marek Polacek 2017-03-20 13:32:00 UTC
Author: mpolacek
Date: Mon Mar 20 13:31:28 2017
New Revision: 246278

URL: https://gcc.gnu.org/viewcvs?rev=246278&root=gcc&view=rev
Log:
	PR sanitizer/80063
	* asan.c (DEF_SANITIZER_BUILTIN): Use do { } while (0).

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/asan.c
Comment 7 Marek Polacek 2017-03-20 13:34:14 UTC
Fixed.
Comment 8 Marek Polacek 2017-03-20 13:53:28 UTC
I created PR80116 to track the warning addition.