Add Coccinelle usage: one for blacklisting, one for patch collection

This is inspired by the [Coccinelle](http://coccinelle.lip6.fr/) usage
in systemd.  I also took it a bit further and added infrastructure
to have spatches which should never apply.  This acts as a blacklist.

The reason to do the latter is that coccinelle is *way* more powerful than the
regular expresssions we have in `make syntax-check`.

I started with blacklisting `g_error_free()` directly. The reason that's bad is
it leaves a dangling pointer.

Closes: #754
Approved by: jlebon

.redhat-ci.Dockerfile

@@ -8,6 +8,7 @@ RUN dnf install -y \
         fuse \
         gjs \
         parallel \
+        coccinelle \
         clang \
         libubsan \
         libasan \

.redhat-ci.yml

@@ -11,6 +11,7 @@ container:
 
 packages:
   - libasan
+  - coccinelle
 
 env:
     CFLAGS: '-fsanitize=undefined -fsanitize-undefined-trap-on-error -fsanitize=address -O2 -Wp,-D_FORTIFY_SOURCE=2'

Makefile-tests.am

@@ -28,6 +28,7 @@ EXTRA_DIST += \
 # include the builddir in $PATH so we find our just-built ostree
 # binary.
 TESTS_ENVIRONMENT += OT_TESTS_DEBUG=1 \
+  OSTREE_UNINSTALLED_SRCDIR=$(abs_top_srcdir) \
 	OSTREE_UNINSTALLED=$(abs_top_builddir) \
 	G_DEBUG=fatal-warnings \
 	GI_TYPELIB_PATH=$$(cd $(top_builddir) && pwd)$${GI_TYPELIB_PATH:+:$$GI_TYPELIB_PATH} \
@@ -99,6 +100,7 @@ dist_test_scripts = \
 	tests/test-switchroot.sh \
 	tests/test-pull-contenturl.sh \
 	tests/test-pull-mirrorlist.sh \
+	tests/coccinelle.sh \
 	$(NULL)
 
 if BUILDOPT_FUSE

coccinelle/README.md

@@ -0,0 +1,6 @@
+This is a directory of semantic patches
+to apply with coccinelle, like the collection in systemd:
+https://github.com/systemd/systemd/tree/29f32655842a0712e8db482bcefc4da8908460c8/coccinelle
+
+See also the tests/coccinelle directory which
+has spatches which detect errors.

coccinelle/newstyle.cocci

@@ -0,0 +1,22 @@
+@@
+expression p;
+@@
+- glnx_set_error_from_errno (p);
+- goto out;
++ return glnx_throw_errno (p);
+@@
+expression p;
+@@
+- if (!p)
+-   goto out;
++ if (!p)
++   return FALSE;
+@@
+expression p;
+@@
+- gboolean ret = FALSE;
+...
+- ret = TRUE;
+- out:
+- return ret;
++ return TRUE;

src/libostree/ostree-repo-pull.c

@@ -279,18 +279,21 @@ pull_termination_condition (OtPullData          *pull_data)
 
 static void
 check_outstanding_requests_handle_error (OtPullData          *pull_data,
-                                         GError              *error)
+                                         GError             **errorp)
 {
+  g_assert (errorp);
+
+  GError *error = *errorp;
   if (error)
     {
       if (!pull_data->caught_error)
         {
           pull_data->caught_error = TRUE;
-          g_propagate_error (pull_data->async_error, error);
+          g_propagate_error (pull_data->async_error, g_steal_pointer (errorp));
         }
       else
         {
-          g_error_free (error);
+          g_clear_error (errorp);
         }
     }
   else
@@ -382,7 +385,7 @@ idle_worker (gpointer user_data)
 {
   OtPullData *pull_data = user_data;
   ScanObjectQueueData *scan_data;
-  GError *error = NULL;
+  g_autoptr(GError) error = NULL;
 
   scan_data = g_queue_pop_head (&pull_data->scan_object_queue);
   if (!scan_data)
@@ -398,7 +401,7 @@ idle_worker (gpointer user_data)
                               scan_data->recursion_depth,
                               pull_data->cancellable,
                               &error);
-  check_outstanding_requests_handle_error (pull_data, error);
+  check_outstanding_requests_handle_error (pull_data, &error);
 
   g_free (scan_data->path);
   g_free (scan_data);
@@ -760,7 +763,7 @@ content_fetch_on_write_complete (GObject        *object,
 {
   FetchObjectData *fetch_data = user_data;
   OtPullData *pull_data = fetch_data->pull_data;
-  GError *local_error = NULL;
+  g_autoptr(GError) local_error = NULL;
   GError **error = &local_error;
   OstreeObjectType objtype;
   const char *expected_checksum;
@@ -794,7 +797,7 @@ content_fetch_on_write_complete (GObject        *object,
     pull_data->n_fetched_deltapart_fallbacks++;
  out:
   pull_data->n_outstanding_content_write_requests--;
-  check_outstanding_requests_handle_error (pull_data, local_error);
+  check_outstanding_requests_handle_error (pull_data, &local_error);
   fetch_object_data_free (fetch_data);
 }
 
@@ -806,7 +809,7 @@ content_fetch_on_complete (GObject        *object,
   OstreeFetcher *fetcher = (OstreeFetcher *)object;
   FetchObjectData *fetch_data = user_data;
   OtPullData *pull_data = fetch_data->pull_data;
-  GError *local_error = NULL;
+  g_autoptr(GError) local_error = NULL;
   GError **error = &local_error;
   GCancellable *cancellable = NULL;
   guint64 length;
@@ -881,7 +884,7 @@ content_fetch_on_complete (GObject        *object,
 
  out:
   pull_data->n_outstanding_content_fetches--;
-  check_outstanding_requests_handle_error (pull_data, local_error);
+  check_outstanding_requests_handle_error (pull_data, &local_error);
   if (free_fetch_data)
     fetch_object_data_free (fetch_data);
 }
@@ -893,7 +896,7 @@ on_metadata_written (GObject           *object,
 {
   FetchObjectData *fetch_data = user_data;
   OtPullData *pull_data = fetch_data->pull_data;
-  GError *local_error = NULL;
+  g_autoptr(GError) local_error = NULL;
   GError **error = &local_error;
   const char *expected_checksum;
   OstreeObjectType objtype;
@@ -927,7 +930,7 @@ on_metadata_written (GObject           *object,
   pull_data->n_outstanding_metadata_write_requests--;
   fetch_object_data_free (fetch_data);
 
-  check_outstanding_requests_handle_error (pull_data, local_error);
+  check_outstanding_requests_handle_error (pull_data, &local_error);
 }
 
 static void
@@ -943,7 +946,7 @@ meta_fetch_on_complete (GObject           *object,
   const char *checksum;
   g_autofree char *checksum_obj = NULL;
   OstreeObjectType objtype;
-  GError *local_error = NULL;
+  g_autoptr(GError) local_error = NULL;
   GError **error = &local_error;
   glnx_fd_close int fd = -1;
   gboolean free_fetch_data = TRUE;
@@ -1038,7 +1041,7 @@ meta_fetch_on_complete (GObject           *object,
   g_assert (pull_data->n_outstanding_metadata_fetches > 0);
   pull_data->n_outstanding_metadata_fetches--;
   pull_data->n_fetched_metadata++;
-  check_outstanding_requests_handle_error (pull_data, local_error);
+  check_outstanding_requests_handle_error (pull_data, &local_error);
   if (free_fetch_data)
     fetch_object_data_free (fetch_data);
 }
@@ -1061,7 +1064,7 @@ on_static_delta_written (GObject           *object,
 {
   FetchStaticDeltaData *fetch_data = user_data;
   OtPullData *pull_data = fetch_data->pull_data;
-  GError *local_error = NULL;
+  g_autoptr(GError) local_error = NULL;
   GError **error = &local_error;
 
   g_debug ("execute static delta part %s complete", fetch_data->expected_checksum);
@@ -1072,7 +1075,7 @@ on_static_delta_written (GObject           *object,
  out:
   g_assert (pull_data->n_outstanding_deltapart_write_requests > 0);
   pull_data->n_outstanding_deltapart_write_requests--;
-  check_outstanding_requests_handle_error (pull_data, local_error);
+  check_outstanding_requests_handle_error (pull_data, &local_error);
   /* Always free state */
   fetch_static_delta_data_free (fetch_data);
 }
@@ -1088,7 +1091,7 @@ static_deltapart_fetch_on_complete (GObject           *object,
   g_autofree char *temp_path = NULL;
   g_autoptr(GInputStream) in = NULL;
   g_autoptr(GVariant) part = NULL;
-  GError *local_error = NULL;
+  g_autoptr(GError) local_error = NULL;
   GError **error = &local_error;
   glnx_fd_close int fd = -1;
   gboolean free_fetch_data = TRUE;
@@ -1132,7 +1135,7 @@ static_deltapart_fetch_on_complete (GObject           *object,
   g_assert (pull_data->n_outstanding_deltapart_fetches > 0);
   pull_data->n_outstanding_deltapart_fetches--;
   pull_data->n_fetched_deltaparts++;
-  check_outstanding_requests_handle_error (pull_data, local_error);
+  check_outstanding_requests_handle_error (pull_data, &local_error);
   if (free_fetch_data)
     fetch_static_delta_data_free (fetch_data);
 }
@@ -1968,7 +1971,7 @@ on_superblock_fetched (GObject   *src,
 {
   FetchDeltaSuperData *fdata = data;
   OtPullData *pull_data = fdata->pull_data;
-  GError *local_error = NULL;
+  g_autoptr(GError) local_error = NULL;
   GError **error = &local_error;
   g_autoptr(GBytes) delta_superblock_data = NULL;
   const char *from_revision = fdata->from_revision;
@@ -2045,7 +2048,7 @@ on_superblock_fetched (GObject   *src,
   g_assert (pull_data->n_outstanding_metadata_fetches > 0);
   pull_data->n_outstanding_metadata_fetches--;
   pull_data->n_fetched_metadata++;
-  check_outstanding_requests_handle_error (pull_data, local_error);
+  check_outstanding_requests_handle_error (pull_data, &local_error);
 }
 
 static gboolean

src/ostree/main.c

@@ -68,7 +68,7 @@ int
 main (int    argc,
       char **argv)
 {
-  GError *error = NULL;
+  g_autoptr(GError) error = NULL;
   int ret;
 
   setlocale (LC_ALL, "");
@@ -88,7 +88,6 @@ main (int    argc,
           suffix = "\x1b[22m\x1b[0m"; /* bold off, color reset */
         }
       g_printerr ("%serror: %s%s\n", prefix, suffix, error->message);
-      g_error_free (error);
     }
 
   return ret;

src/ostree/ot-main.c

@@ -259,7 +259,7 @@ ostree_option_context_parse (GOptionContext *context,
 
   if (opt_repo == NULL && !(flags & OSTREE_BUILTIN_FLAG_NO_REPO))
     {
-      GError *local_error = NULL;
+      g_autoptr(GError) local_error = NULL;
 
       repo = ostree_repo_new_default ();
       if (!ostree_repo_open (repo, cancellable, &local_error))
@@ -270,14 +270,13 @@ ostree_option_context_parse (GOptionContext *context,
 
               g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED,
                                    "Command requires a --repo argument");
-              g_error_free (local_error);
 
               help = g_option_context_get_help (context, FALSE, NULL);
               g_printerr ("%s", help);
             }
           else
             {
-              g_propagate_error (error, local_error);
+              g_propagate_error (error, g_steal_pointer (&local_error));
             }
           goto out;
         }

tests/coccinelle.sh

@@ -0,0 +1,29 @@
+#!/usr/bin/env bash
+
+# Run the .cocci files in the tests directory; these act
+# as a blacklist.
+
+set -euo pipefail
+
+. $(dirname $0)/libtest.sh
+
+if ! spatch --version 2>/dev/null; then
+    skip "no spatch; get it from http://coccinelle.lip6.fr/"
+fi
+
+if test -z "${OSTREE_UNINSTALLED_SRCDIR:-}"; then
+    skip "running installed?"
+fi
+
+coccitests=$(ls $(dirname $0)/coccinelle/*.cocci)
+echo "1.."$(echo ${coccitests} | wc -l)
+
+for cocci in $(dirname $0)/coccinelle/*.cocci; do
+    echo "Running: ${cocci}"
+    spatch --very-quiet --dir ${OSTREE_UNINSTALLED_SRCDIR} ${cocci} > cocci.out
+    if test -s cocci.out; then
+        sed -e 's/^/# /' < cocci.out >&2
+        fatal "Failed semantic patch: ${cocci}"
+    fi
+    echo ok ${cocci}
+done

tests/coccinelle/README.md

@@ -0,0 +1,2 @@
+Add patches here which should never match in the code; i.e. the suggested
+replacement may be junk.

tests/coccinelle/raw-free.cocci

@@ -0,0 +1,5 @@
+@@
+expression p;
+@@
+- g_error_free (p);
++ g_clear_error (&p);

tests/test-rollsum-cli.c

@@ -22,10 +22,12 @@
 
 #include "ostree-rollsum.h"
 
+#include "libglnx.h"
+
 int
 main (int argc, char **argv)
 {
-  GError *local_error = NULL;
+  g_autoptr(GError) local_error = NULL;
   GError **error = &local_error;
   GBytes *from_bytes = NULL;
   GBytes *to_bytes = NULL;
@@ -64,7 +66,6 @@ main (int argc, char **argv)
   if (local_error)
     {
       g_printerr ("%s\n", local_error->message);
-      g_error_free (local_error);
       return 1;
     }
   return 0;