diff mbox series

[committed] analyzer: provide placeholder implementation of sprintf

Message ID 20230303230459.2730864-1-dmalcolm@redhat.com
State New
Headers show
Series [committed] analyzer: provide placeholder implementation of sprintf | expand

Commit Message

David Malcolm March 3, 2023, 11:04 p.m. UTC
Previously, the analyzer lacked a known_function implementation of
sprintf, and thus would handle calls to sprintf with the "anything could
happen" fallback.

Whilst working on PR analyzer/107565 I noticed that this was preventing
a lot of genuine memory leaks from being reported for Doom; fixing
thusly.

Integration testing of the effect of the patch shows a big increase in
true positives due to the case mentioned in Doom, and one new false
positive (in pcre2), which I'm tracking as PR analyzer/109014.

Comparison:
  GOOD:  67 -> 123 (+56); 10.91% -> 18.33%
   BAD: 547 -> 548 (+1)

where the affected warnings/projects are:
 
  -Wanalyzer-malloc-leak:
    GOOD:  0 -> 56 (+56);  0.00% -> 41.48%
     BAD: 79
      True positives: 0 -> 56 (+56)
        (all in Doom)

  -Wanalyzer-use-of-uninitialized-value:
    GOOD: 0;  0.00%
     BAD: 80 -> 81 (+1)
      False positives:
        pcre2-10.42: 0 -> 1 (+1)

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to trunk as r13-6466-g56572a08ec4a0f.

gcc/analyzer/ChangeLog:
	* kf.cc (class kf_sprintf): New.
	(register_known_functions): Register it.

gcc/testsuite/ChangeLog:
	* gcc.dg/analyzer/doom-d_main-IdentifyVersion.c: New test.
	* gcc.dg/analyzer/sprintf-1.c: New test.
	* gcc.dg/analyzer/sprintf-concat.c: New test.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>
---
 gcc/analyzer/kf.cc                            |  29 ++
 .../analyzer/doom-d_main-IdentifyVersion.c    | 272 ++++++++++++++++++
 gcc/testsuite/gcc.dg/analyzer/sprintf-1.c     |  64 +++++
 .../gcc.dg/analyzer/sprintf-concat.c          |  35 +++
 4 files changed, 400 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/doom-d_main-IdentifyVersion.c
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/sprintf-1.c
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/sprintf-concat.c
diff mbox series

Patch

diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc
index 3a91b6bd6eb..c4485153e1b 100644
--- a/gcc/analyzer/kf.cc
+++ b/gcc/analyzer/kf.cc
@@ -778,6 +778,34 @@  kf_strchr::impl_call_post (const call_details &cd) const
     }
 }
 
+/* Handler for "sprintf".
+     int sprintf(char *str, const char *format, ...);
+*/
+
+class kf_sprintf : public known_function
+{
+public:
+  bool matches_call_types_p (const call_details &cd) const final override
+  {
+    return (cd.num_args () >= 2
+	    && cd.arg_is_pointer_p (0)
+	    && cd.arg_is_pointer_p (1));
+  }
+
+  void impl_call_pre (const call_details &cd) const final override
+  {
+    /* For now, merely assume that the destination buffer gets set to a
+       new svalue.  */
+    region_model *model = cd.get_model ();
+    region_model_context *ctxt = cd.get_ctxt ();
+    const svalue *dst_ptr = cd.get_arg_svalue (0);
+    const region *dst_reg
+      = model->deref_rvalue (dst_ptr, cd.get_arg_tree (0), ctxt);
+    const svalue *content = cd.get_or_create_conjured_svalue (dst_reg);
+    model->set_value (dst_reg, content, ctxt);
+  }
+};
+
 /* Handler for "__builtin_stack_restore".  */
 
 class kf_stack_restore : public known_function
@@ -990,6 +1018,7 @@  register_known_functions (known_function_manager &kfm)
     kfm.add (BUILT_IN_MEMSET, make_unique<kf_memset> ());
     kfm.add (BUILT_IN_MEMSET_CHK, make_unique<kf_memset> ());
     kfm.add (BUILT_IN_REALLOC, make_unique<kf_realloc> ());
+    kfm.add (BUILT_IN_SPRINTF, make_unique<kf_sprintf> ());
     kfm.add (BUILT_IN_STACK_RESTORE, make_unique<kf_stack_restore> ());
     kfm.add (BUILT_IN_STACK_SAVE, make_unique<kf_stack_save> ());
     kfm.add (BUILT_IN_STRCHR, make_unique<kf_strchr> ());
diff --git a/gcc/testsuite/gcc.dg/analyzer/doom-d_main-IdentifyVersion.c b/gcc/testsuite/gcc.dg/analyzer/doom-d_main-IdentifyVersion.c
new file mode 100644
index 00000000000..982b9b74349
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/doom-d_main-IdentifyVersion.c
@@ -0,0 +1,272 @@ 
+/* Reduced from Doom's d_main.c, which is under the GPLv2 or later.  */
+
+/* { dg-additional-options "-Wno-analyzer-too-complex" } */
+
+typedef __SIZE_TYPE__ size_t;
+typedef struct _IO_FILE FILE;
+
+extern int
+printf(const char* __restrict, ...);
+
+extern int
+sprintf(char* __restrict __s, const char* __restrict, ...)
+  __attribute__((__nothrow__));
+
+extern void*
+malloc(size_t __size) __attribute__((__nothrow__, __leaf__))
+__attribute__((__malloc__)) __attribute__((__alloc_size__(1)));
+extern char*
+getenv(const char* __name) __attribute__((__nothrow__, __leaf__))
+__attribute__((__nonnull__(1)));
+extern int
+access(const char* __name, int __type) __attribute__((__nothrow__, __leaf__))
+__attribute__((__nonnull__(1)));
+extern char*
+strcpy(char* __restrict __dest, const char* __restrict __src)
+  __attribute__((__nothrow__, __leaf__)) __attribute__((__nonnull__(1, 2)));
+extern size_t
+strlen(const char* __s) __attribute__((__nothrow__, __leaf__))
+__attribute__((__pure__)) __attribute__((__nonnull__(1)));
+
+typedef enum
+{
+  shareware,
+  registered,
+  commercial,
+
+  retail,
+  indetermined
+
+} GameMode_t;
+
+typedef enum
+{
+  doom,
+  doom2,
+  pack_tnt,
+  pack_plut,
+  none
+
+} GameMission_t;
+
+typedef enum
+{
+  english,
+  french,
+  german,
+  unknown
+
+} Language_t;
+
+typedef enum
+{
+  false,
+  true
+} boolean;
+
+extern boolean devparm;
+extern GameMode_t gamemode;
+extern Language_t language;
+extern char basedefault[1024];
+int
+M_CheckParm(char* check);
+void
+I_Error(char* error, ...);
+
+extern char* wadfiles[20];
+
+void
+D_AddFile(char* file)
+{
+  int numwadfiles;
+  char* newfile;
+
+  for (numwadfiles = 0; wadfiles[numwadfiles]; numwadfiles++)
+    ;
+
+  newfile = malloc(strlen(file) + 1);
+  strcpy(newfile, file); /* { dg-warning "use of possibly-NULL 'newfile' where non-null expected" } */
+
+  wadfiles[numwadfiles] = newfile;
+}
+
+void
+IdentifyVersion(void)
+{
+
+  char* doom1wad;
+  char* doomwad;
+  char* doomuwad;
+  char* doom2wad;
+
+  char* doom2fwad;
+  char* plutoniawad;
+  char* tntwad;
+
+  char* home;
+  char* doomwaddir;
+  doomwaddir = getenv("DOOMWADDIR");
+  if (!doomwaddir)
+    doomwaddir = ".";
+
+  doom2wad = malloc(strlen(doomwaddir) + 1 + 9 + 1);
+  sprintf(doom2wad, "%s/doom2.wad", doomwaddir); /* { dg-warning "possibly-NULL 'doom2wad'" } */
+
+  doomuwad = malloc(strlen(doomwaddir) + 1 + 8 + 1);
+  sprintf(doomuwad, "%s/doomu.wad", doomwaddir); /* { dg-warning "possibly-NULL 'doomuwad'" } */
+
+  doomwad = malloc(strlen(doomwaddir) + 1 + 8 + 1);
+  sprintf(doomwad, "%s/doom.wad", doomwaddir); /* { dg-warning "possibly-NULL 'doomwad'" } */
+
+  doom1wad = malloc(strlen(doomwaddir) + 1 + 9 + 1);
+  sprintf(doom1wad, "%s/doom1.wad", doomwaddir); /* { dg-warning "possibly-NULL 'doom1wad'" } */
+
+  plutoniawad = malloc(strlen(doomwaddir) + 1 + 12 + 1);
+  sprintf(plutoniawad, "%s/plutonia.wad", doomwaddir); /* { dg-warning "possibly-NULL 'plutoniawad'" } */
+
+  tntwad = malloc(strlen(doomwaddir) + 1 + 9 + 1);
+  sprintf(tntwad, "%s/tnt.wad", doomwaddir); /* { dg-warning "possibly-NULL 'tntwad'" } */
+
+  doom2fwad = malloc(strlen(doomwaddir) + 1 + 10 + 1);
+  sprintf(doom2fwad, "%s/doom2f.wad", doomwaddir); /* { dg-warning "possibly-NULL 'doom2fwad'" } */
+
+  home = getenv("HOME");
+  if (!home)
+    I_Error("Please set $HOME to your home directory");
+  sprintf(basedefault, "%s/.doomrc", home);
+
+  if (M_CheckParm("-shdev")) {
+    gamemode = shareware;
+    devparm = true;
+    D_AddFile("devdata"
+              "doom1.wad");
+    D_AddFile("devmaps"
+              "data_se/texture1.lmp");
+    D_AddFile("devmaps"
+              "data_se/pnames.lmp");
+    strcpy(basedefault,
+           "devdata"
+           "default.cfg");
+    return; /* { dg-warning "leak of 'doom2wad'" } */
+    /* { dg-warning "leak of 'doomuwad'"    "leak" { target *-*-* } .-1 } */
+    /* { dg-warning "leak of 'doomwad'"     "leak" { target *-*-* } .-2 } */
+    /* { dg-warning "leak of 'doom1wad'"    "leak" { target *-*-* } .-3 } */
+    /* { dg-warning "leak of 'plutoniawad'" "leak" { target *-*-* } .-4 } */
+    /* { dg-warning "leak of 'tntwad'"      "leak" { target *-*-* } .-5 } */
+    /* { dg-warning "leak of 'doom2fwad'"   "leak" { target *-*-* } .-6 } */
+  }
+
+  if (M_CheckParm("-regdev")) {
+    gamemode = registered;
+    devparm = true;
+    D_AddFile("devdata"
+              "doom.wad");
+    D_AddFile("devmaps"
+              "data_se/texture1.lmp");
+    D_AddFile("devmaps"
+              "data_se/texture2.lmp");
+    D_AddFile("devmaps"
+              "data_se/pnames.lmp");
+    strcpy(basedefault,
+           "devdata"
+           "default.cfg");
+    return; /* { dg-warning "leak of 'doom2wad'" } */
+    /* { dg-warning "leak of 'doomuwad'"    "leak" { target *-*-* } .-1 } */
+    /* { dg-warning "leak of 'doomwad'"     "leak" { target *-*-* } .-2 } */
+    /* { dg-warning "leak of 'doom1wad'"    "leak" { target *-*-* } .-3 } */
+    /* { dg-warning "leak of 'plutoniawad'" "leak" { target *-*-* } .-4 } */
+    /* { dg-warning "leak of 'tntwad'"      "leak" { target *-*-* } .-5 } */
+    /* { dg-warning "leak of 'doom2fwad'"   "leak" { target *-*-* } .-6 } */
+  }
+
+  if (M_CheckParm("-comdev")) {
+    gamemode = commercial;
+    devparm = true;
+
+    D_AddFile("devdata"
+              "doom2.wad");
+
+    D_AddFile("devmaps"
+              "cdata/texture1.lmp");
+    D_AddFile("devmaps"
+              "cdata/pnames.lmp");
+    strcpy(basedefault,
+           "devdata"
+           "default.cfg");
+    return; /* { dg-warning "leak of 'doom2wad'" } */
+    /* { dg-warning "leak of 'doomuwad'"    "leak" { target *-*-* } .-1 } */
+    /* { dg-warning "leak of 'doomwad'"     "leak" { target *-*-* } .-2 } */
+    /* { dg-warning "leak of 'doom1wad'"    "leak" { target *-*-* } .-3 } */
+    /* { dg-warning "leak of 'plutoniawad'" "leak" { target *-*-* } .-4 } */
+    /* { dg-warning "leak of 'tntwad'"      "leak" { target *-*-* } .-5 } */
+    /* { dg-warning "leak of 'doom2fwad'"   "leak" { target *-*-* } .-6 } */
+  }
+
+  if (!access(doom2fwad, 4)) {
+    gamemode = commercial;
+
+    language = french;
+    printf("French version\n");
+    D_AddFile(doom2fwad);
+    return; /* { dg-warning "leak of 'doom2wad'" } */
+    /* { dg-warning "leak of 'doomuwad'"    "leak" { target *-*-* } .-1 } */
+    /* { dg-warning "leak of 'doomwad'"     "leak" { target *-*-* } .-2 } */
+    /* { dg-warning "leak of 'doom1wad'"    "leak" { target *-*-* } .-3 } */
+    /* { dg-warning "leak of 'plutoniawad'" "leak" { target *-*-* } .-4 } */
+    /* { dg-warning "leak of 'tntwad'"      "leak" { target *-*-* } .-5 } */
+    /* { dg-warning "leak of 'doom2fwad'"   "leak" { target *-*-* } .-6 } */
+  }
+
+  if (!access(doom2wad, 4)) {
+    gamemode = commercial;
+    D_AddFile(doom2wad);
+    return; /* { dg-warning "leak of 'doom2wad'" } */
+    /* { dg-warning "leak of 'doomuwad'"    "leak" { target *-*-* } .-1 } */
+    /* { dg-warning "leak of 'doomwad'"     "leak" { target *-*-* } .-2 } */
+    /* { dg-warning "leak of 'doom1wad'"    "leak" { target *-*-* } .-3 } */
+    /* { dg-warning "leak of 'plutoniawad'" "leak" { target *-*-* } .-4 } */
+    /* { dg-warning "leak of 'tntwad'"      "leak" { target *-*-* } .-5 } */
+    /* { dg-warning "leak of 'doom2fwad'"   "leak" { target *-*-* } .-6 } */
+  }
+
+  if (!access(plutoniawad, 4)) {
+    gamemode = commercial;
+    D_AddFile(plutoniawad);
+    return; /* { dg-warning "leak of 'doom2wad'" } */
+    /* { dg-warning "leak of 'doomuwad'"    "leak" { target *-*-* } .-1 } */
+    /* { dg-warning "leak of 'doomwad'"     "leak" { target *-*-* } .-2 } */
+    /* { dg-warning "leak of 'doom1wad'"    "leak" { target *-*-* } .-3 } */
+    /* { dg-warning "leak of 'plutoniawad'" "leak" { target *-*-* } .-4 } */
+    /* { dg-warning "leak of 'tntwad'"      "leak" { target *-*-* } .-5 } */
+    /* { dg-warning "leak of 'doom2fwad'"   "leak" { target *-*-* } .-6 } */
+  }
+
+  if (!access(tntwad, 4)) {
+    gamemode = commercial;
+    D_AddFile(tntwad);
+    return; /* { dg-warning "leak of 'doom2wad'" } */
+    /* { dg-warning "leak of 'doomuwad'"    "leak" { target *-*-* } .-1 } */
+    /* { dg-warning "leak of 'doomwad'"     "leak" { target *-*-* } .-2 } */
+    /* { dg-warning "leak of 'doom1wad'"    "leak" { target *-*-* } .-3 } */
+    /* { dg-warning "leak of 'plutoniawad'" "leak" { target *-*-* } .-4 } */
+    /* { dg-warning "leak of 'tntwad'"      "leak" { target *-*-* } .-5 } */
+    /* { dg-warning "leak of 'doom2fwad'"   "leak" { target *-*-* } .-6 } */
+  }
+
+  if (!access(doomuwad, 4)) {
+    gamemode = retail;
+    D_AddFile(doomuwad);
+    return; /* { dg-warning "leak of 'doom2wad'" } */
+    /* { dg-warning "leak of 'doomuwad'"    "leak" { target *-*-* } .-1 } */
+    /* { dg-warning "leak of 'doomwad'"     "leak" { target *-*-* } .-2 } */
+    /* { dg-warning "leak of 'doom1wad'"    "leak" { target *-*-* } .-3 } */
+    /* { dg-warning "leak of 'plutoniawad'" "leak" { target *-*-* } .-4 } */
+    /* { dg-warning "leak of 'tntwad'"      "leak" { target *-*-* } .-5 } */
+    /* { dg-warning "leak of 'doom2fwad'"   "leak" { target *-*-* } .-6 } */
+  }
+
+  /* [...snip...] */
+
+  printf("Game mode indeterminate.\n");
+  gamemode = indetermined;
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/sprintf-1.c b/gcc/testsuite/gcc.dg/analyzer/sprintf-1.c
new file mode 100644
index 00000000000..c79525d912f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/sprintf-1.c
@@ -0,0 +1,64 @@ 
+/* See e.g. https://en.cppreference.com/w/c/io/fprintf
+   and https://www.man7.org/linux/man-pages/man3/sprintf.3.html */
+
+extern int
+sprintf(char* dst, const char* fmt, ...)
+  __attribute__((__nothrow__));
+
+#define NULL ((void *)0)
+
+int
+test_passthrough (char* dst, const char* fmt)
+{
+  /* This assumes that fmt doesn't have any arguments.  */
+  return sprintf (dst, fmt);
+}
+
+void
+test_known (void)
+{
+  char buf[10];
+  int res = sprintf (buf, "foo");
+  /* TODO: ideally we would know the value of "res" is 3,
+     and known the content and strlen of "buf" after the call */
+}
+
+int
+test_null_dst (void)
+{
+  return sprintf (NULL, "hello world"); /* { dg-warning "use of NULL where non-null expected" } */
+}
+
+int
+test_null_fmt (char *dst)
+{
+  return sprintf (dst, NULL);  /* { dg-warning "use of NULL where non-null expected" } */
+}
+
+int
+test_uninit_dst (void)
+{
+  char *dst;
+  return sprintf (dst, "hello world"); /* { dg-warning "use of uninitialized value 'dst'" } */
+}
+
+int
+test_uninit_fmt_ptr (char *dst)
+{
+  const char *fmt;
+  return sprintf (dst, fmt); /* { dg-warning "use of uninitialized value 'fmt'" } */
+}
+
+int
+test_uninit_fmt_buf (char *dst)
+{
+  const char fmt[10];
+  return sprintf (dst, fmt); // TODO (PR analyzer/105899): complain about "fmt" not being initialized
+}
+
+int
+test_fmt_not_terminated (char *dst)
+{
+  const char fmt[3] = "foo";
+  return sprintf (dst, fmt); // TODO (PR analyzer/105899): complain about "fmt" not being terminated
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/sprintf-concat.c b/gcc/testsuite/gcc.dg/analyzer/sprintf-concat.c
new file mode 100644
index 00000000000..0094f3e6449
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/sprintf-concat.c
@@ -0,0 +1,35 @@ 
+typedef __SIZE_TYPE__ size_t;
+#define NULL ((void *)0)
+
+extern size_t
+strlen(const char* __s) __attribute__((__nothrow__, __leaf__))
+__attribute__((__pure__)) __attribute__((__nonnull__(1)));
+
+extern void*
+malloc(size_t __size) __attribute__((__nothrow__, __leaf__))
+__attribute__((__malloc__)) __attribute__((__alloc_size__(1)));
+
+extern int
+sprintf(char* __restrict __s, const char* __restrict, ...)
+  __attribute__((__nothrow__));
+
+char *
+test_1 (const char *a, const char *b)
+{
+  size_t sz = strlen (a) + strlen (b) + 2;
+  char *p = malloc (sz);
+  if (!p)
+    return NULL;
+  sprintf (p, "%s/%s", a, b);
+  return p;
+}
+
+void
+test_2 (const char *a, const char *b)
+{
+  size_t sz = strlen (a) + strlen (b) + 2;
+  char *p = malloc (sz); /* { dg-message "allocated here" } */
+  if (!p)
+    return;
+  sprintf (p, "%s/%s", a, b); /* { dg-warning "leak of 'p' " } */
+}