Add cppcheck static analysis

Add a check-cppcheck target and fix all the issues uncovered by it
(including adding one suppression).

diff --git a/Makefile.am b/Makefile.am
index cde92aeab965f52247fc8d8285b60c4b78613ccd..28d7672ad1e75f4f55d5f458c7d14016f0c0d070 100644 (file)
--- a/Makefile.am
+++ b/Makefile.am
@@ -156,6 +156,14 @@ tests_portable_strndup_t_LDADD = tests/tap/libtap.a portable/libportable.la
 check-local: $(check_PROGRAMS)
        cd tests && ./runtests -l '$(abs_top_srcdir)/tests/TESTS'
 
+# Used by maintainers to check the source code with cppcheck.
+check-cppcheck:
+       cd $(abs_top_srcdir) &&                                         \
+           find . -name .git -prune -o -name '*.[ch]' -print           \
+           | cppcheck -q --force --error-exitcode=2 --file-list=-      \
+               --suppressions-list=tests/data/cppcheck.supp            \
+               --enable=warning,performance,portability,style
+
 # Used by maintainers to run the test suite under valgrind.
 check-valgrind: $(check_PROGRAMS)
        rm -rf $(abs_top_builddir)/tmp-valgrind

diff --git a/context.c b/context.c
index 73f973156eafb8e4930ad169d18a56197ac94eb0..71a628e5c3e68735f424495a77daab73038961ac 100644 (file)
--- a/context.c
+++ b/context.c
@@ -4,11 +4,11 @@
  * The context structure is the internal state maintained by the pam-krb5
  * module between calls to the various public interfaces.
  *
+ * Copyright 2005-2009, 2014, 2020 Russ Allbery <eagle@eyrie.org>
  * Copyright 2011
  *     The Board of Trustees of the Leland Stanford Junior University
- * Copyright 2005, 2006, 2007, 2008, 2009, 2014 Russ Allbery <eagle@eyrie.org>
  * Copyright 2005 Andres Salomon <dilinger@debian.org>
- * Copyright 1999, 2000 Frank Cusack <fcusack@fcusack.com>
+ * Copyright 1999-2000 Frank Cusack <fcusack@fcusack.com>
  *
  * See LICENSE for licensing terms.
  */
@@ -92,7 +92,7 @@ pamk5_context_fetch(struct pam_args *args)
     pamret = pam_get_data(args->pamh, "pam_krb5", (void *) &args->config->ctx);
     if (pamret != PAM_SUCCESS)
         args->config->ctx = NULL;
-    if (pamret == 0 && args->config->ctx == NULL)
+    if (pamret == PAM_SUCCESS && args->config->ctx == NULL)
         return PAM_SERVICE_ERR;
     if (args->config->ctx != NULL)
         args->user = args->config->ctx->name;

diff --git a/internal.h b/internal.h
index a2a68c2d62f63ffaa2da3a645ff5448259e4f3de..d5af879887663204372b46d4c3e41343c457476a 100644 (file)
--- a/internal.h
+++ b/internal.h
@@ -1,11 +1,11 @@
 /*
  * Internal prototypes and structures for pam-krb5.
  *
+ * Copyright 2005-2009, 2014, 2020 Russ Allbery <eagle@eyrie.org>
  * Copyright 2011, 2012
  *     The Board of Trustees of the Leland Stanford Junior University
- * Copyright 2005, 2006, 2007, 2008, 2009, 2014 Russ Allbery <eagle@eyrie.org>
  * Copyright 2005 Andres Salomon <dilinger@debian.org>
- * Copyright 1999, 2000 Frank Cusack <fcusack@fcusack.com>
+ * Copyright 1999-2000 Frank Cusack <fcusack@fcusack.com>
  *
  * See LICENSE for licensing terms.
  */

diff --git a/setcred.c b/setcred.c
index 90d3e736497a00390e70d3232b04251f6895a520..5a0852504f6955d3728abb5592ac5136d90bde78 100644 (file)
--- a/setcred.c
+++ b/setcred.c
@@ -5,12 +5,12 @@
  * to create the user's ticket cache.  The shared code is abstracted here into
  * the pamk5_setcred function.
  *
- * Copyright 2005, 2006, 2007, 2008, 2009, 2014, 2017
+ * Copyright 2005-2009, 2014, 2017, 2020
  *     Russ Allbery <eagle@eyrie.org>
  * Copyright 2011
  *     The Board of Trustees of the Leland Stanford Junior University
  * Copyright 2005 Andres Salomon <dilinger@debian.org>
- * Copyright 1999, 2000 Frank Cusack <fcusack@fcusack.com>
+ * Copyright 1999-2000 Frank Cusack <fcusack@fcusack.com>
  *
  * See LICENSE for licensing terms.
  */
@@ -20,6 +20,7 @@
 #include <portable/pam.h>
 #include <portable/system.h>
 
+#include <assert.h>
 #include <errno.h>
 #include <pwd.h>
 
@@ -267,8 +268,8 @@ pamk5_setcred(struct pam_args *args, bool refresh)
      * with its weird PAM handling, so we're going to cobble up a new context
      * for ourselves.
      */
-    pamk5_context_fetch(args);
-    if (args->config->ctx == NULL) {
+    pamret = pamk5_context_fetch(args);
+    if (pamret != PAM_SUCCESS) {
         putil_debug(args, "no context found, creating one");
         pamret = create_session_context(args);
         if (pamret != PAM_SUCCESS || args->config->ctx == NULL)

diff --git a/tests/data/cppcheck.supp b/tests/data/cppcheck.supp
new file mode 100644 (file)
index 0000000..fa4329e
--- /dev/null
+++ b/tests/data/cppcheck.supp
@@ -0,0 +1,55 @@
+// Suppressions file for cppcheck.  -*- conf -*-
+//
+// This includes suppressions for all of my projects, including files that
+// aren't in rra-c-util, for ease of sharing between projects.  The ones that
+// don't apply to a particular project should hopefully be harmless.
+//
+// To determine the correct suppression to add for a new error, run cppcheck
+// with the --xml flag and then add a suppression for the error id, file
+// location, and line.
+//
+// Copyright 2018-2020 Russ Allbery <eagle@eyrie.org>
+//
+// Copying and distribution of this file, with or without modification, are
+// permitted in any medium without royalty provided the copyright notice and
+// this notice are preserved.  This file is offered as-is, without any
+// warranty.
+//
+// SPDX-License-Identifier: FSFAP
+
+// I like declaring variables at the top of a function rather than cluttering
+// every if and loop body with declarations.
+variableScope
+
+// strlen of a constant string is more maintainable code than hard-coding the
+// string length.
+constArgument:tests/runtests.c:804
+
+// False positive due to recursive function.
+knownConditionTrueFalse:portable/getopt.c:146
+
+// False positive since the string comes from a command-line define.
+knownConditionTrueFalse:tests/tap/remctl.c:79
+
+// Stored in the returned ai struct, but cppcheck can't see the assignment
+// because of the struct sockaddr * cast.
+memleak:portable/getaddrinfo.c:236
+
+// Bug in cppcheck 1.89.  The address of this variable is passed to a Windows
+// function (albeit through a cast).
+nullPointer:portable/winsock.c:61
+
+// Setting the variable to NULL explicitly after deallocation.
+redundantAssignment:tests/pam-util/options-t.c
+
+// (remctl) Bug in cppcheck 1.89.  The address of these variables are passed
+// to a PHP function.
+uninitvar:php/php_remctl.c:119
+uninitvar:php/php_remctl.c:123
+uninitvar:php/php_remctl.c:315
+uninitvar:php/php5_remctl.c:125
+uninitvar:php/php5_remctl.c:129
+uninitvar:php/php5_remctl.c:321
+
+// (pam-krb5) cppcheck doesn't recognize the unused attribute on labels.
+unusedLabel:auth.c:872

diff --git a/tests/module/alt-auth-t.c b/tests/module/alt-auth-t.c
index 1cc729e4d6bb4112f2e64a6a1fb9a232bf6699f0..2da4412f59b294c1fbe414db654caeac6eb42ecf 100644 (file)
--- a/tests/module/alt-auth-t.c
+++ b/tests/module/alt-auth-t.c
@@ -7,6 +7,7 @@
  * avoid requiring user configuration).
  *
  * Written by Russ Allbery <eagle@eyrie.org>
+ * Copyright 2020 Russ Allbery <eagle@eyrie.org>
  * Copyright 2012
  *     The Board of Trustees of the Leland Stanford Junior University
  *
@@ -97,7 +98,6 @@ main(void)
     diag("re-running username-map with fully-qualified PAM user");
     run_script("data/scripts/alt-auth/username-map", &config);
     free(user);
-    config.user = krbconf->username;
 
     /*
      * Add the password and make the user match our authentication principal,

diff --git a/tests/module/cache-t.c b/tests/module/cache-t.c
index a380aef2442a4d51573169628672bd7968bafd47..423c27f06cc76165ec3b86619fb336e68a0022fb 100644 (file)
--- a/tests/module/cache-t.c
+++ b/tests/module/cache-t.c
@@ -7,7 +7,7 @@
  * created (so without setuid and with chown doing nothing).
  *
  * Written by Russ Allbery <eagle@eyrie.org>
- * Copyright 2017 Russ Allbery <eagle@eyrie.org>
+ * Copyright 2017, 2020 Russ Allbery <eagle@eyrie.org>
  * Copyright 2011, 2012
  *     The Board of Trustees of the Leland Stanford Junior University
  *
@@ -149,6 +149,7 @@ main(void)
 
     /* Change the authenticating user and test search_k5login. */
     pwd.pw_name = (char *) "testuser";
+    pam_set_pwd(&pwd);
     config.user = "testuser";
     basprintf(&k5login, "%s/.k5login", pwd.pw_dir);
     file = fopen(k5login, "w");
@@ -166,6 +167,7 @@ main(void)
 
     /* Test search_k5login when no .k5login file exists. */
     pwd.pw_name = krbconf->username;
+    pam_set_pwd(&pwd);
     config.user = krbconf->username;
     diag("testing search_k5login with no .k5login file");
     run_script("data/scripts/cache/search-k5login", &config);