Merge branch 'error-matches-bool' into 'master'

Add _DBUS_ASSERT_ERROR_XOR_BOOL, and a test for assertions

See merge request dbus/dbus!49

Reviewed-by: pwithnall

bus/activation.c

@@ -821,10 +821,7 @@ update_directory (BusActivation       *activation,
   retval = TRUE;
 
  out:
-  if (!retval)
-    _DBUS_ASSERT_ERROR_IS_SET (error);
-  else
-    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
+  _DBUS_ASSERT_ERROR_XOR_BOOL (error, retval);
 
   if (iter != NULL)
     _dbus_directory_close (iter);

bus/desktop-file.c

@@ -688,11 +688,7 @@ bus_desktop_file_load (DBusString *filename,
   parser.desktop_file = NULL;
 
 out:
-  if (result != NULL)
-    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
-  else
-    _DBUS_ASSERT_ERROR_IS_SET (error);
-
+  _DBUS_ASSERT_ERROR_XOR_BOOL (error, result != NULL);
   parser_clear (&parser);
   return result;
 }

bus/driver.c

@@ -2310,10 +2310,7 @@ bus_driver_handle_become_monitor (DBusConnection *connection,
   ret = TRUE;
 
 out:
-  if (ret)
-    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
-  else
-    _DBUS_ASSERT_ERROR_IS_SET (error);
+  _DBUS_ASSERT_ERROR_XOR_BOOL (error, ret);
 
   for (iter = _dbus_list_get_first_link (&rules);
       iter != NULL;

cmake/test/CMakeLists.txt

@@ -96,6 +96,7 @@ if(DBUS_WITH_GLIB)
 
     set(TEST_LIBRARIES ${DBUS_INTERNAL_LIBRARIES} dbus-testutils dbus-testutils-glib ${GLIB2_LIBRARIES})
 
+    add_test_executable(test-assertions ${TEST_DIR}/internals/assertions.c ${TEST_LIBRARIES})
     add_test_executable(test-corrupt ${TEST_DIR}/corrupt.c ${TEST_LIBRARIES})
     add_test_executable(test-dbus-daemon ${TEST_DIR}/dbus-daemon.c ${TEST_LIBRARIES})
     add_test_executable(test-dbus-daemon-eavesdrop ${TEST_DIR}/dbus-daemon-eavesdrop.c ${TEST_LIBRARIES})

dbus/dbus-file-unix.c

@@ -310,9 +310,7 @@ _dbus_string_save_to_file (const DBusString *str,
 
   _dbus_string_free (&tmp_filename);
 
-  if (!retval)
-    _DBUS_ASSERT_ERROR_IS_SET (error);
-  
+  _DBUS_ASSERT_ERROR_XOR_BOOL (error, retval);
   return retval;
 }
 

dbus/dbus-internals.h

@@ -192,27 +192,64 @@ void _dbus_real_assert_not_reached (const char *explanation,
 #define _DBUS_ALIGNOF(type) \
     (_DBUS_STRUCT_OFFSET (struct { char _1; type _2; }, _2))
 
-#ifdef DBUS_DISABLE_CHECKS
+#if defined(DBUS_DISABLE_CHECKS) || defined(DBUS_DISABLE_ASSERT)
 /* this is an assert and not an error, but in the typical --disable-checks case (you're trying
  * to really minimize code size), disabling these assertions makes sense.
  */
 #define _DBUS_ASSERT_ERROR_IS_SET(error) do { } while (0)
 #define _DBUS_ASSERT_ERROR_IS_CLEAR(error) do { } while (0)
+#define _DBUS_ASSERT_ERROR_XOR_BOOL(error, retval) do { } while (0)
 #else
 static inline void
-_dbus_assert_error_is_set (const DBusError *error)
+_dbus_assert_error_is_set (const DBusError *error,
+                           const char      *file,
+                           int              line,
+                           const char      *func)
 {
-    _dbus_assert (error == NULL || dbus_error_is_set (error));
+  _dbus_real_assert (error == NULL || dbus_error_is_set (error),
+                     "error is set", file, line, func);
 }
 
 static inline void
-_dbus_assert_error_is_clear (const DBusError *error)
+_dbus_assert_error_is_clear (const DBusError *error,
+                           const char      *file,
+                           int              line,
+                           const char      *func)
 {
-    _dbus_assert (error == NULL || !dbus_error_is_set (error));
+  _dbus_real_assert (error == NULL || !dbus_error_is_set (error),
+                     "error is clear", file, line, func);
 }
 
-#define _DBUS_ASSERT_ERROR_IS_SET(error) _dbus_assert_error_is_set(error)
-#define _DBUS_ASSERT_ERROR_IS_CLEAR(error) _dbus_assert_error_is_clear(error)
+static inline void
+_dbus_assert_error_xor_bool (const DBusError *error,
+                             dbus_bool_t      retval,
+                             const char      *file,
+                             int              line,
+                             const char      *func)
+{
+  _dbus_real_assert (error == NULL || dbus_error_is_set (error) == !retval,
+                     "error is consistent with boolean result", file, line, func);
+}
+
+/**
+ * Assert that error is set, unless it is NULL in which case we cannot
+ * tell whether it would have been set.
+ */
+#define _DBUS_ASSERT_ERROR_IS_SET(error) _dbus_assert_error_is_set (error, __FILE__, __LINE__, _DBUS_FUNCTION_NAME)
+
+/**
+ * Assert that error is not set, unless it is NULL in which case we cannot
+ * tell whether it would have been set.
+ */
+#define _DBUS_ASSERT_ERROR_IS_CLEAR(error) _dbus_assert_error_is_clear (error, __FILE__, __LINE__, _DBUS_FUNCTION_NAME)
+
+/**
+ * Assert that error is consistent with retval: if error is not NULL,
+ * it must be set if and only if retval is false.
+ *
+ * retval can be a boolean expression like "result != NULL".
+ */
+#define _DBUS_ASSERT_ERROR_XOR_BOOL(error, retval) _dbus_assert_error_xor_bool (error, retval, __FILE__, __LINE__, _DBUS_FUNCTION_NAME)
 #endif
 
 #define _dbus_return_if_error_is_set(error) _dbus_return_if_fail ((error) == NULL || !dbus_error_is_set ((error)))

dbus/dbus-sysdeps-unix.c

@@ -4052,10 +4052,7 @@ _read_subprocess_line_argv (const char *progpath,
  out:
   sigprocmask (SIG_SETMASK, &old_set, NULL);
 
-  if (retval)
-    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
-  else
-    _DBUS_ASSERT_ERROR_IS_SET (error);
+  _DBUS_ASSERT_ERROR_XOR_BOOL (error, retval);
 
   if (result_pipe[0] != -1)
     close (result_pipe[0]);

dbus/dbus-sysdeps-util-unix.c

@@ -52,6 +52,10 @@
 #include <dirent.h>
 #include <sys/un.h>
 
+#ifdef HAVE_SYS_PRCTL_H
+#include <sys/prctl.h>
+#endif
+
 #ifdef HAVE_SYS_SYSLIMITS_H
 #include <sys/syslimits.h>
 #endif
@@ -1583,3 +1587,28 @@ _dbus_daemon_report_stopping (void)
   sd_notify (0, "STOPPING=1");
 #endif
 }
+
+/**
+ * Try to disable core dumps and similar special crash handling.
+ */
+void
+_dbus_disable_crash_handling (void)
+{
+#ifdef HAVE_SETRLIMIT
+  /* No core dumps please, we know we crashed. */
+  struct rlimit r = { 0, };
+
+  getrlimit (RLIMIT_CORE, &r);
+  r.rlim_cur = 0;
+  setrlimit (RLIMIT_CORE, &r);
+#endif
+
+#if defined(HAVE_PRCTL) && defined(PR_SET_DUMPABLE)
+  /* Really, no core dumps please. On Linux, if core_pattern is
+   * set to a pipe (for abrt/apport/corekeeper/etc.), RLIMIT_CORE of 0
+   * is ignored (deliberately, so people can debug init(8) and other
+   * early stuff); but Linux has PR_SET_DUMPABLE, so we can avoid core
+   * dumps anyway. */
+  prctl (PR_SET_DUMPABLE, 0, 0, 0, 0);
+#endif
+}

dbus/dbus-sysdeps-util-win.c

@@ -1688,3 +1688,27 @@ _dbus_win_stderr_win_error (const char    *app,
   fprintf (stderr, "%s: %s: %s\n", app, message, error.message);
   dbus_error_free (&error);
 }
+
+static int exception_handler (LPEXCEPTION_POINTERS p) _DBUS_GNUC_NORETURN;
+
+static int
+exception_handler (LPEXCEPTION_POINTERS p)
+{
+  ExitProcess (0xc0000005);
+}
+
+/**
+ * Try to disable core dumps and similar special crash handling.
+ */
+void
+_dbus_disable_crash_handling (void)
+{
+  /* Disable Windows popup dialog when an app crashes so that app quits
+   * immediately with error code instead of waiting for user to dismiss
+   * the dialog.  */
+  DWORD dwMode = SetErrorMode (SEM_NOGPFAULTERRORBOX);
+
+  SetErrorMode (dwMode | SEM_NOGPFAULTERRORBOX);
+  /* Disable "just in time" debugger */
+  SetUnhandledExceptionFilter ((LPTOP_LEVEL_EXCEPTION_FILTER) &exception_handler);
+}

dbus/dbus-sysdeps-win.c

@@ -3217,11 +3217,7 @@ _dbus_get_autolaunch_address (const char *scope, DBusString *address,
     }
 
 out:
-  if (retval)
-    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
-  else
-    _DBUS_ASSERT_ERROR_IS_SET (error);
-  
+  _DBUS_ASSERT_ERROR_XOR_BOOL (error, retval);
   _dbus_global_unlock (mutex);
   _dbus_string_free (&shm_name);
 

dbus/dbus-sysdeps.h

@@ -732,6 +732,8 @@ void _dbus_combine_tcp_errors (DBusList **sources,
                                const char *port,
                                DBusError *dest);
 
+void _dbus_disable_crash_handling (void);
+
 /** @} */
 
 DBUS_END_DECLS

dbus/dbus-transport.c

@@ -301,10 +301,7 @@ _dbus_transport_new_for_autolaunch (const char *scope, DBusError *error)
     }
 
   result = check_address (_dbus_string_get_const_data (&address), error);
-  if (result == NULL)
-    _DBUS_ASSERT_ERROR_IS_SET (error);
-  else
-    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
+  _DBUS_ASSERT_ERROR_XOR_BOOL (error, result != NULL);
 
  out:
   _dbus_string_free (&address);

test/Makefile.am

@@ -99,12 +99,18 @@ test_shell_LDADD = libdbus-testutils.la
 test_spawn_SOURCES = spawn-test.c
 test_spawn_LDADD = $(top_builddir)/dbus/libdbus-internal.la
 
+test_assertions_SOURCES = internals/assertions.c
+test_assertions_LDADD = libdbus-testutils.la $(GLIB_LIBS)
+
 test_printf_SOURCES = internals/printf.c
 test_printf_LDADD = $(top_builddir)/dbus/libdbus-internal.la
 
 test_refs_SOURCES = internals/refs.c
 test_refs_LDADD = libdbus-testutils.la $(GLIB_LIBS)
 
+test_segfault_SOURCES = test-segfault.c
+test_segfault_LDADD = $(top_builddir)/dbus/libdbus-internal.la
+
 test_server_oom_SOURCES = internals/server-oom.c
 test_server_oom_LDADD = libdbus-testutils.la $(GLIB_LIBS)
 
@@ -161,6 +167,7 @@ endif
 
 if DBUS_WITH_GLIB
 installable_tests += \
+	test-assertions \
 	test-corrupt \
 	test-dbus-daemon \
 	test-dbus-daemon-eavesdrop \

test/internals/assertions.c

@@ -0,0 +1,235 @@
+/*
+ * Copyright © 2018 Collabora Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation files
+ * (the "Software"), to deal in the Software without restriction,
+ * including without limitation the rights to use, copy, modify, merge,
+ * publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <config.h>
+
+#include <dbus/dbus.h>
+#include "dbus/dbus-internals.h"
+#include "test-utils-glib.h"
+
+typedef struct
+{
+  int dummy;
+} Fixture;
+
+static void
+setup (Fixture *f G_GNUC_UNUSED,
+       gconstpointer context G_GNUC_UNUSED)
+{
+}
+
+#if defined(DBUS_ENABLE_ASSERT) == defined(DBUS_DISABLE_ASSERT)
+# error Macros contradict
+#endif
+
+#if defined(DBUS_ENABLE_CHECKS) == defined(DBUS_DISABLE_CHECKS)
+# error Macros contradict
+#endif
+
+static void
+test_assert (Fixture *f,
+             gconstpointer context G_GNUC_UNUSED)
+{
+#ifdef DBUS_ENABLE_ASSERT
+  if (!g_test_undefined ())
+    {
+      g_test_skip ("Not testing programming errors");
+    }
+  else if (g_test_subprocess ())
+    {
+      dbus_setenv ("DBUS_BLOCK_ON_ABORT", NULL);
+      _dbus_disable_crash_handling ();
+      _dbus_assert (42 == 23);
+    }
+  else
+    {
+      g_test_trap_subprocess (NULL, 0, 0);
+      g_test_trap_assert_failed ();
+      g_test_trap_assert_stderr ("*42 == 23*");
+    }
+#else
+  g_test_skip ("Assertions disabled");
+#endif
+}
+
+static void
+test_assert_error_is_set (Fixture *f,
+                          gconstpointer context G_GNUC_UNUSED)
+{
+  DBusError e = DBUS_ERROR_INIT;
+  DBusError *ep = NULL;
+
+#if defined(DBUS_ENABLE_ASSERT) && defined(DBUS_ENABLE_CHECKS)
+  if (!g_test_undefined ())
+    {
+      g_test_skip ("Not testing programming errors");
+    }
+  else if (g_test_subprocess ())
+    {
+      dbus_setenv ("DBUS_BLOCK_ON_ABORT", NULL);
+      _dbus_disable_crash_handling ();
+      _DBUS_ASSERT_ERROR_IS_SET (&e);
+    }
+  else
+    {
+      g_test_trap_subprocess (NULL, 0, 0);
+      g_test_trap_assert_failed ();
+    }
+
+  _DBUS_SET_OOM (&e);
+  _DBUS_ASSERT_ERROR_IS_SET (&e);
+  _DBUS_ASSERT_ERROR_IS_SET (ep);
+  dbus_error_free (&e);
+#else
+  g_test_skip ("Assertions or checks disabled");
+#endif
+}
+
+static void
+test_assert_error_is_clear (Fixture *f,
+                            gconstpointer context G_GNUC_UNUSED)
+{
+#if defined(DBUS_ENABLE_ASSERT) && defined(DBUS_ENABLE_CHECKS)
+  DBusError e = DBUS_ERROR_INIT;
+  DBusError *ep = NULL;
+
+  if (!g_test_undefined ())
+    {
+      g_test_skip ("Not testing programming errors");
+    }
+  else if (g_test_subprocess ())
+    {
+      dbus_setenv ("DBUS_BLOCK_ON_ABORT", NULL);
+      _dbus_disable_crash_handling ();
+      _DBUS_SET_OOM (&e);
+      _DBUS_ASSERT_ERROR_IS_CLEAR (&e);
+    }
+  else
+    {
+      g_test_trap_subprocess (NULL, 0, 0);
+      g_test_trap_assert_failed ();
+    }
+
+  _DBUS_ASSERT_ERROR_IS_CLEAR (&e);
+  _DBUS_ASSERT_ERROR_IS_CLEAR (ep);
+#else
+  g_test_skip ("Assertions or checks disabled");
+#endif
+}
+
+static void
+test_assert_error_xor_true (Fixture *f,
+                            gconstpointer context G_GNUC_UNUSED)
+{
+#if defined(DBUS_ENABLE_ASSERT) && defined(DBUS_ENABLE_CHECKS)
+  DBusError e = DBUS_ERROR_INIT;
+  DBusError *ep = NULL;
+  dbus_bool_t retval = TRUE;
+
+  if (!g_test_undefined ())
+    {
+      g_test_skip ("Not testing programming errors");
+    }
+  else if (g_test_subprocess ())
+    {
+      dbus_setenv ("DBUS_BLOCK_ON_ABORT", NULL);
+      _dbus_disable_crash_handling ();
+      _DBUS_SET_OOM (&e);
+      _DBUS_ASSERT_ERROR_XOR_BOOL (&e, retval);
+    }
+  else
+    {
+      g_test_trap_subprocess (NULL, 0, 0);
+      g_test_trap_assert_failed ();
+    }
+
+  _DBUS_ASSERT_ERROR_XOR_BOOL (&e, retval);
+  _DBUS_ASSERT_ERROR_XOR_BOOL (ep, retval);
+#else
+  g_test_skip ("Assertions or checks disabled");
+#endif
+}
+
+static void
+test_assert_error_xor_false (Fixture *f,
+                             gconstpointer context G_GNUC_UNUSED)
+{
+#if defined(DBUS_ENABLE_ASSERT) && defined(DBUS_ENABLE_CHECKS)
+  DBusError e = DBUS_ERROR_INIT;
+  DBusError *ep = NULL;
+  void *retval = NULL;
+
+  if (!g_test_undefined ())
+    {
+      g_test_skip ("Not testing programming errors");
+    }
+  else if (g_test_subprocess ())
+    {
+      dbus_setenv ("DBUS_BLOCK_ON_ABORT", NULL);
+      _dbus_disable_crash_handling ();
+      _DBUS_ASSERT_ERROR_XOR_BOOL (&e, retval != NULL);
+    }
+  else
+    {
+      g_test_trap_subprocess (NULL, 0, 0);
+      g_test_trap_assert_failed ();
+    }
+
+  _DBUS_SET_OOM (&e);
+  _DBUS_ASSERT_ERROR_XOR_BOOL (&e, retval != NULL);
+  _DBUS_ASSERT_ERROR_XOR_BOOL (ep, retval != NULL);
+#else
+  g_test_skip ("Assertions or checks disabled");
+#endif
+}
+
+static void
+teardown (Fixture *f G_GNUC_UNUSED,
+          gconstpointer context G_GNUC_UNUSED)
+{
+}
+
+int
+main (int argc,
+      char **argv)
+{
+  int ret;
+
+  test_init (&argc, &argv);
+
+  g_test_add ("/assertions/assert",
+              Fixture, NULL, setup, test_assert, teardown);
+  g_test_add ("/assertions/assert_error_is_set",
+              Fixture, NULL, setup, test_assert_error_is_set, teardown);
+  g_test_add ("/assertions/assert_error_is_clear",
+              Fixture, NULL, setup, test_assert_error_is_clear, teardown);
+  g_test_add ("/assertions/assert_error_xor_true",
+              Fixture, NULL, setup, test_assert_error_xor_true, teardown);
+  g_test_add ("/assertions/assert_error_xor_false",
+              Fixture, NULL, setup, test_assert_error_xor_false, teardown);
+
+  ret = g_test_run ();
+  dbus_shutdown ();
+  return ret;
+}

test/test-segfault.c

@@ -5,63 +5,14 @@
 #include <signal.h>
 #endif
 
-#ifdef HAVE_SETRLIMIT
-#include <sys/resource.h>
-#endif
-
-#ifdef HAVE_SYS_PRCTL_H
-#include <sys/prctl.h>
-#endif
-
-#ifdef DBUS_WIN
-#include <stdio.h>
-#include <windows.h>
-
-#include <dbus/dbus-macros.h>
-
-int exception_handler (LPEXCEPTION_POINTERS p) _DBUS_GNUC_NORETURN;
-
-/* Explicit Windows exception handlers needed to supress OS popups */
-int
-exception_handler(LPEXCEPTION_POINTERS p)
-{
-  fprintf(stderr, "test-segfault: raised fatal exception as intended\n");
-  ExitProcess(0xc0000005);
-}
-#endif
+#include "dbus/dbus-sysdeps.h"
 
 int
 main (int argc, char **argv)
 {
   char *p;  
 
-#ifdef DBUS_WIN
-  /* Disable Windows popup dialog when an app crashes so that app quits
-   * immediately with error code instead of waiting for user to dismiss
-   * the dialog.  */
-  DWORD dwMode = SetErrorMode(SEM_NOGPFAULTERRORBOX);
-  SetErrorMode(dwMode | SEM_NOGPFAULTERRORBOX);
-  /* Disable "just in time" debugger */
-  SetUnhandledExceptionFilter((LPTOP_LEVEL_EXCEPTION_FILTER)&exception_handler);
-#endif
-
-#ifdef HAVE_SETRLIMIT
-  /* No core dumps please, we know we crashed. */
-  struct rlimit r = { 0, };
-  
-  getrlimit (RLIMIT_CORE, &r);
-  r.rlim_cur = 0;
-  setrlimit (RLIMIT_CORE, &r);
-#endif
-
-#if defined(HAVE_PRCTL) && defined(PR_SET_DUMPABLE)
-  /* Really, no core dumps please. On Linux, if core_pattern is
-   * set to a pipe (for abrt/apport/corekeeper/etc.), RLIMIT_CORE of 0
-   * is ignored (deliberately, so people can debug init(8) and other
-   * early stuff); but Linux has PR_SET_DUMPABLE, so we can avoid core
-   * dumps anyway. */
-  prctl (PR_SET_DUMPABLE, 0, 0, 0, 0);
-#endif
+  _dbus_disable_crash_handling ();
 
 #ifdef HAVE_RAISE
   raise (SIGSEGV);