Libguestfs - Aug 2019 - [nbdkit PATCH 0/2] RFC: tighter filter versions

This is not intended for v1.14.  In fact, we may decide that the
second patch is too gross, although the first one still seems like a
useful improvement in isolation.

I will also point out that all our filters are in-tree, and set the
user-controlled field .version to the current release string.  We
could replace the second patch with a simpler one that just checks
._api_version as an int (as before), but adds the additional
requirement that .version match PACKAGE_VERSION from config.h.  Then
we don't have to hack around ABI changes of the first field member,
but are slightly more protected if we forget to bump ._api_version in
the future.

Eric Blake (2):
  include: Expose nbdkit version information to public
  filters: Stronger version match requirements

 configure.ac                | 15 ++++++++++++-
 server/filters.c            | 32 +++++++++++++++++++++-----
 include/nbdkit-common.h     |  2 ++
 include/nbdkit-filter.h     | 10 ++++-----
 .gitignore                  |  1 +
 include/Makefile.am         |  1 +
 include/nbdkit-version.h.in | 45 +++++++++++++++++++++++++++++++++++++
 7 files changed, 93 insertions(+), 13 deletions(-)
 create mode 100644 include/nbdkit-version.h.in

-- 
2.21.0

Internally, our plugins and filters can (and do!) use PACKAGE_VERSION
to populate the .version field.  But this macro is defined in
config.h, which is unsuitable for installation in /usr/include, so
external plugin authors cannot use it.  It is worth letting our public
interface include a version designation (ideally, users should NOT be
basing compile-time decisions solely on what version they are
compiling against, but instead using actual feature tests, but we
weren't even allowing for a version check).

The addition is done in such as way as to continue to allow version
bumps to touch a single place in configure.ac and have that propagate
to all affected files.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 configure.ac                | 15 ++++++++++++-
 include/nbdkit-common.h     |  2 ++
 .gitignore                  |  1 +
 include/Makefile.am         |  1 +
 include/nbdkit-version.h.in | 45 +++++++++++++++++++++++++++++++++++++
 5 files changed, 63 insertions(+), 1 deletion(-)
 create mode 100644 include/nbdkit-version.h.in

diff --git a/configure.ac b/configure.ac
index ac8b4ba7..1667cb3f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -29,7 +29,11 @@
 # OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 # SUCH DAMAGE.

-AC_INIT([nbdkit], [1.13.9])
+m4_define([NBDKIT_VERSION_MAJOR], [1])
+m4_define([NBDKIT_VERSION_MINOR], [13])
+m4_define([NBDKIT_VERSION_MICRO], [9])
+AC_INIT([nbdkit],
+        NBDKIT_VERSION_MAJOR.NBDKIT_VERSION_MINOR.NBDKIT_VERSION_MICRO)
 AC_CONFIG_MACRO_DIR([m4])
 m4_ifdef([AC_USE_SYSTEM_EXTENSIONS],[],
          [m4_define([AC_USE_SYSTEM_EXTENSIONS],[])])
@@ -45,6 +49,14 @@ AC_CANONICAL_HOST

 AC_PROG_SED

+dnl Expose version information to the public headers
+[NBDKIT_]VERSION_MAJOR=NBDKIT_VERSION_MAJOR
+[NBDKIT_]VERSION_MINOR=NBDKIT_VERSION_MINOR
+[NBDKIT_]VERSION_MICRO=NBDKIT_VERSION_MICRO
+AC_SUBST([NBDKIT_VERSION_MAJOR])
+AC_SUBST([NBDKIT_VERSION_MINOR])
+AC_SUBST([NBDKIT_VERSION_MICRO])
+
 dnl Check for basic C environment.
 AC_PROG_CC_STDC
 AC_PROG_INSTALL
@@ -891,6 +903,7 @@ AC_CONFIG_FILES([Makefile
                  common/utils/Makefile
                  docs/Makefile
                  include/Makefile
+                 include/nbdkit-version.h
                  plugins/Makefile
                  plugins/curl/Makefile
                  plugins/data/Makefile
diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
index e004aa18..3a62b47e 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -41,6 +41,8 @@
 #include <stdint.h>
 #include <errno.h>

+#include <nbdkit-version.h>
+
 #ifdef __cplusplus
 extern "C" {
 #endif
diff --git a/.gitignore b/.gitignore
index 15620626..464a860d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -51,6 +51,7 @@ Makefile.in
 /docs/plugin-links.pod
 /fuzzing/sync_dir/
 /html/*.html
+/include/nbdkit-version.h
 /INSTALL
 /install-sh
 /libtool
diff --git a/include/Makefile.am b/include/Makefile.am
index 3a293359..cf46abc6 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -35,4 +35,5 @@ include_HEADERS = \
 	nbdkit-common.h \
 	nbdkit-plugin.h \
 	nbdkit-filter.h \
+	nbdkit-version.h \
 	$(NULL)
diff --git a/include/nbdkit-version.h.in b/include/nbdkit-version.h.in
new file mode 100644
index 00000000..38bbf0d2
--- /dev/null
+++ b/include/nbdkit-version.h.in
@@ -0,0 +1,45 @@
+/* nbdkit
+ * Copyright (C) 2013-2019 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#ifndef NBDKIT_VERSION_H
+#define NBDKIT_VERSION_H
+
+#if !defined (NBDKIT_PLUGIN_H) && !defined (NBDKIT_FILTER_H)
+#error this header file should not be directly included
+#endif
+
+#define NBDKIT_VERSION_MAJOR @NBDKIT_VERSION_MAJOR@
+#define NBDKIT_VERSION_MINOR @NBDKIT_VERSION_MINOR@
+#define NBDKIT_VERSION_MICRO @NBDKIT_VERSION_MICRO@
+#define NBDKIT_VERSION_STRING
"@NBDKIT_VERSION_MAJOR@.@NBDKIT_VERSION_MINOR@.@NBDKIT_VERSION_MICRO@"
+
+#endif /* NBDKIT_VERSION_H */
-- 
2.21.0

Rather than guessing on API/ABI compatibility based on when we
remember to bump a compile-time constant (which should be done at
least once per stable release, although we forgot in the last two, per
6934d4c1), we can go even tighter and require that the version be
precisely the version string used at compile time (catching
incompatibility even in between non-stable releases).  But as this is
yet another ABI change, and we want to avoid seg-faults if an old
filter .so is accidentally loaded, we first sanity check whether it
looks like we have a pointer (which will not live in the first page of
memory - new ABI) or a small integer (old ABI); we generally get lucky
that this works for both 32- and 64-bit platforms, whether little- or
big-endian (on 64-bit platforms, there are various corner-case
pointers where it won't work; for example, an address of 0x100000004
would fail the test on either a little- or a big- endian machine; but
in general, that doesn't happen):

$ ./nbdkit --filter /usr/lib64/nbdkit/filters/nbdkit-nozero-filter.so null
nbdkit: /usr/lib64/nbdkit/filters/nbdkit-nozero-filter.so: filter is
incompatible with this version of nbdkit, and appears to stem from nbdkit 1.14
or earlier

Similarly, we are fairly likely
that any new filter will fail to load with an old nbdkit, for the same
reasons (and again with the same corner-case addresses where it
doesn't actually fail).  A nice error message in 99.99% of the cases
with segv on the corner cases is better than nothing, especially since
all such version mismatch cases are user configuration error:

$ nbdkit --filter ./filters/nozero/.libs/nbdkit-nozero-filter.so null
nbdkit: ./filters/nozero/.libs/nbdkit-nozero-filter.so: filter is incompatible
with this version of nbdkit (_api_version = 430821533)

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 server/filters.c        | 32 ++++++++++++++++++++++++++------
 include/nbdkit-filter.h | 10 ++++------
 2 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/server/filters.c b/server/filters.c
index 3bea476b..bb6394fb 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -839,6 +839,10 @@ filter_register (struct backend *next, size_t index, const
char *filename,
   struct backend_filter *f;
   const struct nbdkit_filter *filter;
   size_t i, len;
+  union overlay {
+    const char *str;
+    unsigned int api;
+  } u;

   f = calloc (1, sizeof *f);
   if (f == NULL) {
@@ -866,15 +870,31 @@ filter_register (struct backend *next, size_t index, const
char *filename,
     exit (EXIT_FAILURE);
   }

-  /* We do not provide API or ABI guarantees for filters, other than
-   * the ABI position of _api_version that will let us diagnose
-   * mismatch when the API changes.
+  /* We do not provide API or ABI guarantees for filters; instead, we
+   * use _nbdkit_version to diagnose mismatch in compilation
+   * vs. runtime setup.  However, nbdkit 1.2 through 1.14 used an int
+   * instead of const char * (containing values ranging from 1 through
+   * 5 over time), and if we blindly dereference that ABI as a pointer
+   * without at least a sanity check, we get a SEGV instead of a nice
+   * failure message.  Hence, we have to run a sanity-check: if the
+   * first four bytes of resemble a small positive integer (but not 0,
+   * as that may be a typical upper half of a big-endian 64-bit
+   * pointer), assume it is the old filter ABI; and only dereference
+   * if it looks like we have the new API.
    */
-  if (filter->_api_version != NBDKIT_FILTER_API_VERSION) {
+  u.str = filter->_nbdkit_version;
+  if (u.api && u.api < 8) {
+    fprintf (stderr,
+             "%s: %s: filter is incompatible with this version of nbdkit,
"
+             "and appears to stem from nbdkit 1.14 or earlier\n",
+             program_name, f->filename);
+    exit (EXIT_FAILURE);
+  }
+  if (strcmp (filter->_nbdkit_version, NBDKIT_VERSION_STRING) != 0) {
     fprintf (stderr,
              "%s: %s: filter is incompatible with this version of nbdkit
"
-             "(_api_version = %d)\n",
-             program_name, f->filename, filter->_api_version);
+             "(_nbdkit_version = %s)\n",
+             program_name, f->filename, filter->_nbdkit_version);
     exit (EXIT_FAILURE);
   }

diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index 4f8f19ab..29d92755 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -43,8 +43,6 @@
 extern "C" {
 #endif

-#define NBDKIT_FILTER_API_VERSION 5 /* Corresponding to v1.14 */
-
 struct nbdkit_extent {
   uint64_t offset;
   uint64_t length;
@@ -98,11 +96,11 @@ struct nbdkit_filter {
    * one version of the header with a runtime compiled against a
    * different version.
    */
-  int _api_version;
+  const char *_nbdkit_version;

   /* Because there is no ABI guarantee, new fields may be added where
-   * logically appropriate, as long as we correctly bump
-   * NBDKIT_FILTER_API_VERSION once per stable release.
+   * logically appropriate, as long as _nbdkit_version can be used to
+   * flag version mismatch.
    */
   const char *name;
   const char *longname;
@@ -178,7 +176,7 @@ struct nbdkit_filter {
   struct nbdkit_filter *                                                \
   filter_init (void)                                                    \
   {                                                                     \
-    (filter)._api_version = NBDKIT_FILTER_API_VERSION;                  \
+    (filter)._nbdkit_version = NBDKIT_VERSION_STRING;                   \
     return &(filter);                                                   \
   }

-- 
2.21.0

As we discussed on IRC, patch 1 is obvious and uncontroversial, let's
do it.

I think patch 2 is overkill really, and weird pointer tricks have a
habit of breaking on big endian / 32 bit / obscure platforms.  Why
don't we just create a brand new filter API number (6 I think), reject
everything with API != 6, and as a further test verify that
strcmp (filter->version, PACKAGE_VERSION) == 0 ?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/