analyzer: add warnings about writes to constant regions [PR95007]
authorDavid Malcolm <dmalcolm@redhat.com>
Wed, 7 Oct 2020 22:34:09 +0000 (18:34 -0400)
committerDavid Malcolm <dmalcolm@redhat.com>
Mon, 12 Oct 2020 16:03:07 +0000 (12:03 -0400)
This patch adds two new warnings:
  -Wanalyzer-write-to-const
  -Wanalyzer-write-to-string-literal
for code paths where the analyzer detects a write to a constant region.

As noted in the documentation part of the patch, the analyzer doesn't
prioritize detection of such writes, in that the state-merging logic
will blithely lose the distinction between const and non-const regions.
Hence false negatives are likely to arise due to state-merging.

However, if the analyzer does happen to spot such a write, it seems worth
reporting, hence this patch.

gcc/analyzer/ChangeLog:
* analyzer.opt (Wanalyzer-write-to-const): New.
(Wanalyzer-write-to-string-literal): New.
* region-model-impl-calls.cc (region_model::impl_call_memcpy):
Call check_for_writable_region.
(region_model::impl_call_memset): Likewise.
(region_model::impl_call_strcpy): Likewise.
* region-model.cc (class write_to_const_diagnostic): New.
(class write_to_string_literal_diagnostic): New.
(region_model::check_for_writable_region): New.
(region_model::set_value): Call check_for_writable_region.
* region-model.h (region_model::check_for_writable_region): New
decl.

gcc/ChangeLog:
* doc/invoke.texi: Document -Wanalyzer-write-to-const and
-Wanalyzer-write-to-string-literal.

gcc/testsuite/ChangeLog:
PR c/83347
PR middle-end/90404
PR analyzer/95007
* gcc.dg/analyzer/write-to-const-1.c: New test.
* gcc.dg/analyzer/write-to-string-literal-1.c: New test.

gcc/analyzer/analyzer.opt
gcc/analyzer/region-model-impl-calls.cc
gcc/analyzer/region-model.cc
gcc/analyzer/region-model.h
gcc/doc/invoke.texi
gcc/testsuite/gcc.dg/analyzer/write-to-const-1.c [new file with mode: 0644]
gcc/testsuite/gcc.dg/analyzer/write-to-string-literal-1.c [new file with mode: 0644]

index a4d384211f37655b971f40c85467a409156b37dd..c9df6dc76737bb110a8e1f15fda225e6ca5f25ad 100644 (file)
@@ -114,6 +114,14 @@ Wanalyzer-use-of-pointer-in-stale-stack-frame
 Common Var(warn_analyzer_use_of_pointer_in_stale_stack_frame) Init(1) Warning
 Warn about code paths in which a pointer to a stale stack frame is used.
 
+Wanalyzer-write-to-const
+Common Var(warn_analyzer_write_to_const) Init(1) Warning
+Warn about code paths which attempt to write to a const object.
+
+Wanalyzer-write-to-string-literal
+Common Var(warn_analyzer_write_to_string_literal) Init(1) Warning
+Warn about code paths which attempt to write to a string literal.
+
 Wanalyzer-too-complex
 Common Var(warn_analyzer_too_complex) Init(0) Warning
 Warn if the code is too complicated for the analyzer to fully explore.
index 009b8c3ecb075210509dd36b90aff16557c41a7e..ef84e638992107ed13299b98ef1b4ebccb64f8c1 100644 (file)
@@ -305,6 +305,8 @@ region_model::impl_call_memcpy (const call_details &cd)
        return;
     }
 
+  check_for_writable_region (dest_reg, cd.get_ctxt ());
+
   /* Otherwise, mark region's contents as unknown.  */
   mark_region_as_unknown (dest_reg);
 }
@@ -346,6 +348,8 @@ region_model::impl_call_memset (const call_details &cd)
        }
     }
 
+  check_for_writable_region (dest_reg, cd.get_ctxt ());
+
   /* Otherwise, mark region's contents as unknown.  */
   mark_region_as_unknown (dest_reg);
   return false;
@@ -397,6 +401,8 @@ region_model::impl_call_strcpy (const call_details &cd)
 
   cd.maybe_set_lhs (dest_sval);
 
+  check_for_writable_region (dest_reg, cd.get_ctxt ());
+
   /* For now, just mark region's contents as unknown.  */
   mark_region_as_unknown (dest_reg);
 }
index a88a295a24177b273a5ecd94813e010f4d5afdd8..480f25a3a4b0d2135e17216408bbf93ab2a153f1 100644 (file)
@@ -1532,16 +1532,131 @@ region_model::deref_rvalue (const svalue *ptr_sval, tree ptr_tree,
   return m_mgr->get_symbolic_region (ptr_sval);
 }
 
+/* A subclass of pending_diagnostic for complaining about writes to
+   constant regions of memory.  */
+
+class write_to_const_diagnostic
+: public pending_diagnostic_subclass<write_to_const_diagnostic>
+{
+public:
+  write_to_const_diagnostic (const region *reg, tree decl)
+  : m_reg (reg), m_decl (decl)
+  {}
+
+  const char *get_kind () const FINAL OVERRIDE
+  {
+    return "write_to_const_diagnostic";
+  }
+
+  bool operator== (const write_to_const_diagnostic &other) const
+  {
+    return (m_reg == other.m_reg
+           && m_decl == other.m_decl);
+  }
+
+  bool emit (rich_location *rich_loc) FINAL OVERRIDE
+  {
+    bool warned = warning_at (rich_loc, OPT_Wanalyzer_write_to_const,
+                             "write to %<const%> object %qE", m_decl);
+    if (warned)
+      inform (DECL_SOURCE_LOCATION (m_decl), "declared here");
+    return warned;
+  }
+
+  label_text describe_final_event (const evdesc::final_event &ev) FINAL OVERRIDE
+  {
+    return ev.formatted_print ("write to %<const%> object %qE here", m_decl);
+  }
+
+private:
+  const region *m_reg;
+  tree m_decl;
+};
+
+/* A subclass of pending_diagnostic for complaining about writes to
+   string literals.  */
+
+class write_to_string_literal_diagnostic
+: public pending_diagnostic_subclass<write_to_string_literal_diagnostic>
+{
+public:
+  write_to_string_literal_diagnostic (const region *reg)
+  : m_reg (reg)
+  {}
+
+  const char *get_kind () const FINAL OVERRIDE
+  {
+    return "write_to_string_literal_diagnostic";
+  }
+
+  bool operator== (const write_to_string_literal_diagnostic &other) const
+  {
+    return m_reg == other.m_reg;
+  }
+
+  bool emit (rich_location *rich_loc) FINAL OVERRIDE
+  {
+    return warning_at (rich_loc, OPT_Wanalyzer_write_to_string_literal,
+                      "write to string literal");
+    /* Ideally we would show the location of the STRING_CST as well,
+       but it is not available at this point.  */
+  }
+
+  label_text describe_final_event (const evdesc::final_event &ev) FINAL OVERRIDE
+  {
+    return ev.formatted_print ("write to string literal here");
+  }
+
+private:
+  const region *m_reg;
+};
+
+/* Use CTXT to warn If DEST_REG is a region that shouldn't be written to.  */
+
+void
+region_model::check_for_writable_region (const region* dest_reg,
+                                        region_model_context *ctxt) const
+{
+  /* Fail gracefully if CTXT is NULL.  */
+  if (!ctxt)
+    return;
+
+  const region *base_reg = dest_reg->get_base_region ();
+  switch (base_reg->get_kind ())
+    {
+    default:
+      break;
+    case RK_DECL:
+      {
+       const decl_region *decl_reg = as_a <const decl_region *> (base_reg);
+       tree decl = decl_reg->get_decl ();
+       /* Warn about writes to const globals.
+          Don't warn for writes to const locals, and params in particular,
+          since we would warn in push_frame when setting them up (e.g the
+          "this" param is "T* const").  */
+       if (TREE_READONLY (decl)
+           && is_global_var (decl))
+         ctxt->warn (new write_to_const_diagnostic (dest_reg, decl));
+      }
+      break;
+    case RK_STRING:
+      ctxt->warn (new write_to_string_literal_diagnostic (dest_reg));
+      break;
+    }
+}
+
 /* Set the value of the region given by LHS_REG to the value given
    by RHS_SVAL.  */
 
 void
 region_model::set_value (const region *lhs_reg, const svalue *rhs_sval,
-                        region_model_context */*ctxt*/)
+                        region_model_context *ctxt)
 {
   gcc_assert (lhs_reg);
   gcc_assert (rhs_sval);
 
+  check_for_writable_region (lhs_reg, ctxt);
+
   m_store.set_value (m_mgr->get_store_manager(), lhs_reg, rhs_sval,
                     BK_direct);
 }
index cfeac8d695163b9f73d794c7adf48bc175a1322a..234ca16bcefc74f668f99fd28308c6ba1590783c 100644 (file)
@@ -2736,6 +2736,9 @@ class region_model
   bool called_from_main_p () const;
   const svalue *get_initial_value_for_global (const region *reg) const;
 
+  void check_for_writable_region (const region* dest_reg,
+                                 region_model_context *ctxt) const;
+
   /* Storing this here to avoid passing it around everywhere.  */
   region_model_manager *const m_mgr;
 
index 307f4f5426ca4a4a791c7c9874768c1bfa11903b..c8281ecf5021063f8a3a902b36d2af97755214fe 100644 (file)
@@ -429,6 +429,8 @@ Objective-C and Objective-C++ Dialects}.
 -Wno-analyzer-use-after-free @gol
 -Wno-analyzer-use-of-pointer-in-stale-stack-frame @gol
 -Wno-analyzer-use-of-uninitialized-value @gol
+-Wno-analyzer-write-to-const @gol
+-Wno-analyzer-write-to-string-literal @gol
 }
 
 @item C and Objective-C-only Warning Options
@@ -8801,6 +8803,8 @@ Enabling this option effectively enables the following warnings:
 -Wanalyzer-unsafe-call-within-signal-handler @gol
 -Wanalyzer-use-after-free @gol
 -Wanalyzer-use-of-pointer-in-stale-stack-frame @gol
+-Wanalyzer-write-to-const @gol
+-Wanalyzer-write-to-string-literal @gol
 }
 
 This option is only available if GCC was configured with analyzer
@@ -8983,6 +8987,30 @@ to disable it.
 This diagnostic warns for paths through the code in which a pointer
 is dereferenced that points to a variable in a stale stack frame.
 
+@item -Wno-analyzer-write-to-const
+@opindex Wanalyzer-write-to-const
+@opindex Wno-analyzer-write-to-const
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-write-to-const}
+to disable it.
+
+This diagnostic warns for paths through the code in which the analyzer
+detects an attempt to write through a pointer to a @code{const} object.
+However, the analyzer does not prioritize detection of such paths, so
+false negatives are more likely relative to other warnings.
+
+@item -Wno-analyzer-write-to-string-literal
+@opindex Wanalyzer-write-to-string-literal
+@opindex Wno-analyzer-write-to-string-literal
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-write-to-string-literal}
+to disable it.
+
+This diagnostic warns for paths through the code in which the analyzer
+detects an attempt to write through a pointer to a string literal.
+However, the analyzer does not prioritize detection of such paths, so
+false negatives are more likely relative to other warnings.
+
 @end table
 
 Pertinent parameters for controlling the exploration are:
diff --git a/gcc/testsuite/gcc.dg/analyzer/write-to-const-1.c b/gcc/testsuite/gcc.dg/analyzer/write-to-const-1.c
new file mode 100644 (file)
index 0000000..dc724e2
--- /dev/null
@@ -0,0 +1,29 @@
+/* PR middle-end/90404 */
+
+const int c1 = 20; /* { dg-message "declared here" } */
+int test_1 (void)
+{
+  *((int*) &c1) = 10; /* { dg-warning "write to 'const' object 'c1'" } */
+  return c1;
+}
+
+/* Example of writing to a subregion (an element within a const array).  */
+
+const int c2[10]; /* { dg-message "declared here" } */
+int test_2 (void)
+{
+  ((int*) &c2)[5] = 10; /* { dg-warning "write to 'const' object 'c2'" } */
+  return c2[5];
+}
+
+const char s3[] = "012.45"; /* { dg-message "declared here" } */
+int test_3 (void)
+{
+  char *p = __builtin_strchr (s3, '.');
+  *p = 0; /* { dg-warning "write to 'const' object 's3'" } */
+
+  if (__builtin_strlen (p) != 3)
+    __builtin_abort ();
+
+  return s3[3] == 0;
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/write-to-string-literal-1.c b/gcc/testsuite/gcc.dg/analyzer/write-to-string-literal-1.c
new file mode 100644 (file)
index 0000000..092500e
--- /dev/null
@@ -0,0 +1,58 @@
+#include <string.h>
+
+/* PR analyzer/95007.  */
+
+void test_1 (void)
+{
+  char *s = "foo";
+  s[0] = 'g'; /* { dg-warning "write to string literal" } */
+}
+
+/* PR c/83347.  */
+
+void test_2 (void)
+{
+  memcpy ("abc", "def", 3); /* { dg-warning "write to string literal" } */
+}
+
+static char * __attribute__((noinline))
+called_by_test_3 (void)
+{
+  return (char *)"foo";
+}
+
+void test_3 (void)
+{
+  char *s = called_by_test_3 ();
+  s[1] = 'a'; /* { dg-warning "write to string literal" } */
+}
+
+static char * __attribute__((noinline))
+called_by_test_4 (int flag)
+{
+  if (flag)
+    return (char *)"foo";
+  else
+    return (char *)"bar";
+}
+
+void test_4 (void)
+{
+  char *s = called_by_test_4 (0);
+  s[1] = 'z'; /* { dg-warning "write to string literal" } */
+}
+
+static char * __attribute__((noinline))
+called_by_test_5 (int flag)
+{
+  if (flag)
+    return (char *)"foo";
+  else
+    return (char *)"bar";
+}
+
+void test_5 (int flag)
+{
+  char *s = called_by_test_5 (flag);
+  s[1] = 'z'; /* We miss this one, unless we disable state merging.  */
+}