Bug 73689

(In reply to comment #0)
> Running dbus unit test crashes on linux and windows regardless of the
> buildsystem 

Hmm? 'make check' of autotools works fine on my machine, ubuntu 12.04. All test cases are passed, 15 + 2.

several lines of test-dbus log

test-dbus: checking for memleaks
test-dbus: completed successfully
PASS: ../dbus/test-dbus

> 
> make check
> ... 
> test-dbus: checking for memleaks
> test-dbus: running memory tests
> test-dbus: checking for memleaks
> test-dbus: running mem-pool tests
> test-dbus: checking for memleaks
> test-dbus: running list tests
> test-dbus: checking for memleaks
> test-dbus: running marshal-validate tests
> test-dbus: checking for memleaks
> test-dbus: running message tests
> /bin/sh: line 5:  2106 Speicherzugriffsfehler 
> XDG_RUNTIME_DIR=/home/xxx/src/dbus-autotools-build/test/XDG_RUNTIME_DIR
> DBUS_FATAL_WARNINGS=1
> DBUS_TEST_DAEMON=/home/xxx/src/dbus-autotools-build/bus/dbus-daemon
> DBUS_TEST_DATA=/home/xxx/src/dbus-autotools-build/test/data
> DBUS_TEST_HOMEDIR=/home/xxx/src/dbus-autotools-build/dbus ${dir}$tst
> FAIL: ../dbus/test-dbus

I'm on opensuse 12.3 x64, one crash is shown below: 

Program received signal SIGSEGV, Segmentation fault.
0x0000000000440843 in _dbus_header_get_byte_order (header=0x4460e7) at ../../dbus/dbus/dbus-marshal-header.c:178
178       return (char) _dbus_string_get_byte (&header->data, BYTE_ORDER_OFFSET);
(gdb) bt
#0  0x0000000000440843 in _dbus_header_get_byte_order (header=0x4460e7) at ../../dbus/dbus/dbus-marshal-header.c:178
#1  0x0000000000436cb9 in _dbus_message_iter_check (iter=0x7fffffffd6f0) at ../../dbus/dbus/dbus-message.c:744
#2  0x0000000000436d89 in _dbus_message_iter_append_check (iter=0x7fffffffd6f0) at ../../dbus/dbus/dbus-message.c:2588
#3  0x0000000000439351 in dbus_message_iter_abandon_container (iter=0x7fffffffd6f0, sub=0x7fffffffd740) at ../../dbus/dbus/dbus-message.c:2957
#4  0x0000000000426b94 in _dbus_message_test (test_data_dir=test_data_dir@entry=0x7fffffffeaff "/home/xxx/src/dbus-autotools-build/test/data")
    at ../../dbus/dbus/dbus-message-util.c:1621
#5  0x000000000040494d in run_data_test (test_data_dir=0x7fffffffeaff "/home/xxx/src/dbus-autotools-build/test/data", test=0x426090 <_dbus_message_test>, 
    test_name=0x4465b3 "message", specific_test=<optimized out>) at ../../dbus/dbus/dbus-test.c:80
#6  run_data_test (test_name=0x4465b3 "message", specific_test=<optimized out>, test=0x426090 <_dbus_message_test>, 
    test_data_dir=0x7fffffffeaff "/home/xxx/src/dbus-autotools-build/test/data") at ../../dbus/dbus/dbus-test.c:72
#7  0x0000000000404b26 in dbus_internal_do_not_use_run_tests (test_data_dir=0x7fffffffeaff "/home/xxx/src/dbus-autotools-build/test/data", specific_test=0x0)
    at ../../dbus/dbus/dbus-test.c:146
#8  0x0000000000404701 in main (argc=1, argv=0x7fffffffd958) at ../../dbus/dbus/dbus-test-main.c:55
(gdb)

(In reply to comment #2)
> I'm on opensuse 12.3 x64, one crash is shown below: 
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x0000000000440843 in _dbus_header_get_byte_order (header=0x4460e7) at ../../dbus/dbus/dbus-marshal-header.c:178
> 178       return (char) _dbus_string_get_byte (&header->data,
> BYTE_ORDER_OFFSET);

looks like header points to an invalid memory 

gdb) p *header
$2 = {data = {dummy1 = 0x76206120746f6e00, dummy2 = 1684630625, dummy3 = 1734964000, dummy_bit1 = 0, dummy_bit2 = 1, dummy_bit3 = 1, dummy_bits = 5}, fields = {{value_pos = 679174400}, 
    {value_pos = 1764321641}, {value_pos = 1650524285}, {value_pos = 1717907043}, {value_pos = 1650524263}, {value_pos = 1701064291}, {value_pos = 774766694}, {value_pos = 779054918}, {
      value_pos = 7496002}, {value_pos = 779054918}}, padding = 2, byte_order = 40}


In the opposite an earlier call to this function gives reasonable values
...
test-dbus: running object-tree tests

Breakpoint 1, _dbus_header_get_byte_order (header=header@entry=0x663808) at ../../dbus/dbus/dbus-marshal-header.c:178
178       return (char) _dbus_string_get_byte (&header->data, BYTE_ORDER_OFFSET);
(gdb) p *header
$3 = {data = {dummy1 = 0x6639a0, dummy2 = 88, dummy3 = 160, dummy_bit1 = 0, dummy_bit2 = 0, dummy_bit3 = 0, dummy_bits = 0}, fields = {{value_pos = -2}, {value_pos = -2}, {
      value_pos = -2}, {value_pos = -2}, {value_pos = -2}, {value_pos = -2}, {value_pos = -2}, {value_pos = -2}, {value_pos = -2}, {value_pos = -2}}, padding = 4, byte_order = 0}
(gdb)

Created attachment 92216 [details] [review]
Crash fix on _dbus_message_test

Created attachment 92217 [details] [review]
Crash fix on check_spawn

Here are test-bus related messages
... 
bin/test-bus: Running message dispatch test
Activating service name='org.freedesktop.DBus.TestSuiteEchoService'
Fd 5 did not have the close-on-exec flag set!
  bin/test-bus(_dbus_print_backtrace+0x1f) [0x494319]
  bin/test-bus(_dbus_abort+0xd) [0x48f3c5]
  bin/test-bus(_dbus_warn+0xd3) [0x47c769]
  bin/test-bus() [0x4a4282]
  bin/test-bus(_dbus_spawn_async_with_babysitter+0x46c) [0x4a4a43]
  bin/test-bus(bus_activation_activate_service+0x1070) [0x41ba2f]
  bin/test-bus() [0x4354a4]
  bin/test-bus(bus_driver_handle_message+0x399) [0x437a05]
  bin/test-bus() [0x42cbf0]
  bin/test-bus() [0x42cf7c]
  bin/test-bus(dbus_connection_dispatch+0x5af) [0x44f32f]
  bin/test-bus(_dbus_loop_dispatch+0x43) [0x498cce]
  bin/test-bus(_dbus_loop_iterate+0x6de) [0x49942b]
  bin/test-bus(bus_test_run_everything+0x26) [0x443119]
  bin/test-bus() [0x432c7f]
  bin/test-bus() [0x43356d]
  bin/test-bus(bus_dispatch_test+0x40) [0x43399b]
  bin/test-bus(main+0x25a) [0x4446b7]
  /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f3b07bf1455]
  bin/test-bus() [0x417979]
Activated service 'org.freedesktop.DBus.TestSuiteEchoService' failed: Process org.freedesktop.DBus.TestSuiteEchoService received signal 6
Did not get the expected org.freedesktop.DBus.TestSuiteEchoService from ListNames
  bin/test-bus(_dbus_print_backtrace+0x1f) [0x494319]
  bin/test-bus(_dbus_abort+0xd) [0x48f3c5]
  bin/test-bus(_dbus_warn+0xd3) [0x47c769]
  bin/test-bus() [0x432fef]
  bin/test-bus() [0x43356d]
  bin/test-bus(bus_dispatch_test+0x40) [0x43399b]
  bin/test-bus(main+0x25a) [0x4446b7]
  /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f3b07bf1455]
  bin/test-bus() [0x417979]
Abgebrochen

Comment on attachment 92216 [details] [review]
Crash fix on _dbus_message_test

Review of attachment 92216 [details] [review]:
-----------------------------------------------------------------

No, this shouldn't be necessary. dbus_message_iter_open_container (&iter, ..., &array_iter) takes an uninitialized array_iter and initializes it.

The error here is that the dbus_message_iter_open_container() is a side-effect inside a _dbus_assert(). If you compile with embedded tests but without assertions (which makes no sense, because the tests check for bad things via assertions) then it won't be called.

In principle, the right thing to do here is to introduce a _dbus_confirm() or _dbus_assert_se() (for "side-effect") or something, which always evaluates its argument even if assertions are disabled - but I'm not going to do that for 1.8 because it's too much code churn for a stable branch.

I think declaring the (DBUS_DISABLE_ASSERT && DBUS_ENABLE_EMBEDDED_TESTS) configuration to be unsupported, or even having configure/cmake fail, would be a reasonable approach.

Comment on attachment 92217 [details] [review]
Crash fix on check_spawn

Review of attachment 92217 [details] [review]:
-----------------------------------------------------------------

> The child returns a non expected signal 6

On what platform, in what configuration?

(In reply to comment #8)
> > The child returns a non expected signal 6
> 
> On what platform, in what configuration?

opensuse 12.3 x64 kernel 3.4.47-2

(In reply to comment #9)
> > On what platform, in what configuration?
> 
> opensuse 12.3 x64 kernel 3.4.47-2

Sorry, I was unclear. I meant:

Autotools or CMake?

If Autotools, what arguments/variables did you pass to configure (run "config.status --version" to confirm)?

If CMake, what compile-time configuration analogous to Autotools configure arguments did you use?

(In reply to comment #6)
> Activating service name='org.freedesktop.DBus.TestSuiteEchoService'
> Fd 5 did not have the close-on-exec flag set!
...
> Activated service 'org.freedesktop.DBus.TestSuiteEchoService' failed:
> Process org.freedesktop.DBus.TestSuiteEchoService received signal 6

As far as I can see, what's happening here is that the child process of test-dbus that's responsible for exec'ing the actual service doesn't set all of its fds O_CLOEXEC. This happens under CMake for me (although it's fd 3 for me), but doesn't under Autotools.

This suggests that there's some fallback path that isn't quite right.

(In reply to comment #11)
> This suggests that there's some fallback path that isn't quite right.

clarifying that a bit:

The Autotools build system has a lot of 

#ifdef HAVE_SOME_THING
   do something the modern way
#else
   do something the legacy way
#endif

In principle, it shouldn't matter if the Autotools build forgets to test for HAVE_SOME_THING and ends up going "the legacy way", because that should work equally well. In practice, "the legacy way" is going to be less tested, and not tested usually means doesn't work.

Looking for CLOEXEC might well be informative. It might be something like one of these (from "git grep -C3 CLOEXEC"):

bus/dir-watch-inotify.c-#ifdef HAVE_INOTIFY_INIT1
dbus/dbus-spawn.c-#ifdef HAVE_PIPE2
dbus/dbus-sysdeps-unix.c-#ifdef HAVE_ACCEPT4

(In reply to comment #12)
> In principle, it shouldn't matter if the Autotools build forgets to test for
> HAVE_SOME_THING

er, "if the CMake build forgets to test for HAVE_SOME_THING" :-)

Created attachment 92295 [details] [review]
inotify: make sure we set the close-on-exec flag

---

This is necessary, but not sufficient: there seems to be another similar fd leak somewhere, which I can't find.

Comment on attachment 92217 [details] [review]
Crash fix on check_spawn

Review of attachment 92217 [details] [review]:
-----------------------------------------------------------------

This patch is not the correct way to handle this. The child process is crashing; it's correct that that's a test failure. The way to fix this is to find and fix the reason for the crash.

(In reply to comment #14)
> inotify: make sure we set the close-on-exec flag

Reviews, anyone?

Comment on attachment 92295 [details] [review]
inotify: make sure we set the close-on-exec flag

Review of attachment 92295 [details] [review]:
-----------------------------------------------------------------

::: bus/dir-watch-inotify.c
@@ +231,4 @@
>  #else
>        inotify_fd = inotify_init ();
>  #endif
> +

unrelated empty line change ?

@@ +237,5 @@
>            _dbus_warn ("Cannot initialize inotify\n");
>            goto out;
>          }
> +
> +      /* In the inotify_init1 case this is unnecessary but harmless,

looks good

Comment on attachment 92295 [details] [review]
inotify: make sure we set the close-on-exec flag

Review of attachment 92295 [details] [review]:
-----------------------------------------------------------------

::: bus/dir-watch-inotify.c
@@ +231,4 @@
>  #else
>        inotify_fd = inotify_init ();
>  #endif
> +

Yeah, not strictly needed, you're right.

Comment on attachment 92295 [details] [review]
inotify: make sure we set the close-on-exec flag

(In reply to comment #17)
> inotify: make sure we set the close-on-exec flag

Applied (without the extra whitespace, and with the extra header that is needed now that we've fixed Bug #39610). Thanks!

Leaving the bug open since there were other similar issues, although perhaps they have since been fixed.

(In reply to comment #7)
> Comment on attachment 92216 [details] [review]
> Crash fix on _dbus_message_test
...
> No, this shouldn't be necessary.
...
> The error here is that the dbus_message_iter_open_container() is a
> side-effect inside a _dbus_assert().

That's Bug #83968.

(In reply to comment #14)
> This is necessary, but not sufficient: there seems to be another similar fd
> leak somewhere, which I can't find.

I think finding that other fd leak is the only thing left to do here, other than Bug #83968.

(In reply to comment #12)
> Looking for CLOEXEC might well be informative. It might be something like
> one of these (from "git grep -C3 CLOEXEC"):
> 
> bus/dir-watch-inotify.c-#ifdef HAVE_INOTIFY_INIT1
> dbus/dbus-spawn.c-#ifdef HAVE_PIPE2
> dbus/dbus-sysdeps-unix.c-#ifdef HAVE_ACCEPT4

I fixed the first of those, and the implementation of the second and third looks OK already, so it's probably somewhere else.

Comment on attachment 92216 [details] [review]
Crash fix on _dbus_message_test

fixed by bug #83968

Created attachment 107237 [details]
config.h autotools cmake diff

(In reply to Simon McVittie from comment #11)
> This suggests that there's some fallback path that isn't quite right.s.

Which need to be verified by comparing the cpp macros generated from config.h.in rsp. config.h.cmake and the compiler command line. Appended is a related diff on linux generated from git master using the following commands:

gcc -E -dM ...dbus-autotools-build/config.h | sort > dbus-autotools-config-vars
gcc -E -dM ../dbus-cmake-build/config.h | sort > dbus-cmake-config-vars
diff -ubB dbus-cmake-config-vars dbus-autotools-config-vars

Created attachment 107446 [details]
Command line autotools/cmake macro diff

(In reply to Ralf Habacker from comment #23)
> Which need to be verified by comparing the cpp macros generated from
> config.h.in rsp. config.h.cmake and the compiler command line.

While this is a good approach if you are trying to solve the problem "which features are not checked, and just assumed not to be present, under cmake?", that's not actually what I meant.

What I meant when I said "fallback path" is this:

For each optional feature - let's say foo2, the improved replacement for foo, as our example - there are at least two code paths, often looking like this:

#ifdef HAVE_FOO2
  /* begin preferred path */
  ok = foo2 (bar, baz);
  /* end preferred path */

  if (!ok && errno == ENOTSUPP)
#endif
    {
      /* begin fallback path */
      ok = foo (bar);

      if (ok)
        ok = compensate_for_not_having_foo2 (baz);
      /* end fallback path */
    }

  if (!ok)
    ... handle error ...

So if we didn't have foo2 at compile-time, we emulate it using older features; and if we did have it at compile-time, but it doesn't work (the kernel says no) at runtime, we emulate it in the same way.

Several of the bugs you've been hitting with cmake are of this form:

* the fallback path has bugs;
* we didn't normally notice this under Autotools because Autotools
  detects whether we can do the preferred path, and a modern Linux system
  will usually use the preferred path at runtime;
* but cmake doesn't check for the preferred path, so we end up on the
  fallback path even on modern Linux

So that's two issues, really: (1) the fallback path is wrong, and (2) cmake uses it. (1) is more severe than (2), but fixing (2) would hide it.

I do not consider cmake to be a recommended way to build D-Bus binaries for Unix - all the distributions use Autotools - so I don't think (2) is actually very serious. I just want the cmake build system to work well enough on Unix that I can be reasonably confident I haven't completely broken its use on Windows when adding some new feature.

In the specific case of the inotify bug that I fixed, (1) was that the fallback path did not set the fd close-on-exec, and the solution was to fix that. That fixed (that part of) the cmake build without actually needing to check for inotify_init1.

Having said that, if you add some more feature-checks to cmake (e.g. a check for pipe2()) and you can identify which one makes the fd leak go away, then we can use that to identify which fallback path is wrong.

Created attachment 107828 [details] [review]
Add configure check for pipe2 to cmake build system

I added this check, recompiled and did run ctest -V test-dbus

1: test-dbus: running spawn tests
1:   /home/ralf.habacker/src/dbus-cmake-build/bin/test-dbus(_dbus_print_backtrace+0x1f) [0x44f7dd]
1:   /home/ralf.habacker/src/dbus-cmake-build/bin/test-dbus(_dbus_abort+0xd) [0x44ab45]
1:   /home/ralf.habacker/src/dbus-cmake-build/bin/test-dbus(_dbus_warn+0xd3) [0x437fe9]
1:   /home/ralf.habacker/src/dbus-cmake-build/bin/test-dbus() [0x415987]
1:   /home/ralf.habacker/src/dbus-cmake-build/bin/test-dbus(_dbus_test_oom_handling+0x49) [0x439093]
1:   /home/ralf.habacker/src/dbus-cmake-build/bin/test-dbus(_dbus_spawn_test+0x20) [0x415e71]
1:   /home/ralf.habacker/src/dbus-cmake-build/bin/test-dbus() [0x413533]
1:   /home/ralf.habacker/src/dbus-cmake-build/bin/test-dbus(dbus_internal_do_not_use_run_tests+0x1fb) [0x413745]
1:   /home/ralf.habacker/src/dbus-cmake-build/bin/test-dbus(main+0x69) [0x4133e9]
1:   /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f5305a8abe5]
1:   /home/ralf.habacker/src/dbus-cmake-build/bin/test-dbus() [0x4132b9]
 1/15 Test  #1: test-dbus ........................***Exception: Other 14.66 sec

OK, so it's not the pipe2() fallback path that is at fault. pipe2() was just an example: Attachment #107237 [details] lists some other possibilities.

This:

-#define DBUS_ENABLE_VERBOSE_MODE 1
+#define DBUS_ENABLE_EMBEDDED_TESTS 1
+#define DBUS_ENABLE_MODULAR_TESTS 1

indicates that this isn't really an "apples to apples" comparison: you've enabled different features in CMake and Autotools.

I would suggest enabling the embedded and modular tests macros in CMake and seeing whether that has any effect, or alternatively, seeing whether a configuration without those features in Autotools has the same crash.

Some other things that might be worth a look, since they interact with fds:

HAVE_ACCEPT4 (but I looked at the non-accept4() fallback and it looked OK)
HAVE_DIRFD
HAVE_INOTIFY_INIT1 (but I think I already fixed that fallback)
HAVE_UNIX_FD_PASSING

It would also be worthwhile to inspect the core dump from the failure.

Comment on attachment 107828 [details] [review]
Add configure check for pipe2 to cmake build system

Review of attachment 107828 [details] [review]:
-----------------------------------------------------------------

Makes sense, even if it doesn't actually fix anything other than efficiency. Feel free to apply.

(In reply to Simon McVittie from comment #28)
> OK, so it's not the pipe2() fallback path that is at fault. pipe2() was just
> an example: Attachment #107237 [details] lists some other possibilities.
> 
> This:
> 
> -#define DBUS_ENABLE_VERBOSE_MODE 1
> +#define DBUS_ENABLE_EMBEDDED_TESTS 1
> +#define DBUS_ENABLE_MODULAR_TESTS 1
> 
> indicates that this isn't really an "apples to apples" comparison: you've
> enabled different features in CMake and Autotools.
> 

It is, because those variables are defined on the command line: 

>> Created attachment 107446 [details]
>> Command line autotools/cmake macro diff

add_definitions(-DDBUS_ENABLE_EMBEDDED_TESTS -DDBUS_ENABLE_MODULAR_TESTS)

Created attachment 108323 [details] [review]
Add configure checks for accept4 dirfd inotify_init1 and unix_fd_passing

(In reply to Ralf Habacker from comment #31)
> Created attachment 108323 [details] [review] [review]
> Add configure checks for accept4 dirfd inotify_init1 and unix_fd_passing

Crash happens also with this patch. 

If I comment out the dbus_warn line mentioned below the crash did not happen on running ctest -R test-dbus which is in fact called with make check. 

dbus_spawn.c...
do_exec (int                       child_err_report_fd,
...

#ifdef DBUS_ENABLE_EMBEDDED_TESTS
  max_open = sysconf (_SC_OPEN_MAX);
  
  for (i = 3; i < max_open; i++)
    {
      int retval;

      if (i == child_err_report_fd)
        continue;
      
      retval = fcntl (i, F_GETFD);

      if (retval != -1 && !(retval & FD_CLOEXEC))
	;//_dbus_warn ("Fd %d did not have the close-on-exec flag set!\n", i);
    }
#endif

It may be for interest that running test-dbus with the required environment variable directly from a shell or a simple c programm using execve also does not crash.

(In reply to Ralf Habacker from comment #32)
> (In reply to Ralf Habacker from comment #31)
> > Created attachment 108323 [details] [review] [review] [review]
> > Add configure checks for accept4 dirfd inotify_init1 and unix_fd_passing
> 
> Crash happens also with this patch. 
> 
> If I comment out the dbus_warn line mentioned below the crash did not happen
> on running ctest -R test-dbus which is in fact called with make check. 
> 
> dbus_spawn.c...
> do_exec (int                       child_err_report_fd,
> ...
> 
> #ifdef DBUS_ENABLE_EMBEDDED_TESTS
>   max_open = sysconf (_SC_OPEN_MAX);
>   
>   for (i = 3; i < max_open; i++)
>     {
>       int retval;
> 
>       if (i == child_err_report_fd)
>         continue;
>       
>       retval = fcntl (i, F_GETFD);
> 
>       if (retval != -1 && !(retval & FD_CLOEXEC))
> 	;//_dbus_warn ("Fd %d did not have the close-on-exec flag set!\n", i);
>     }
> #endif
> 

fd is 3 in the crash case.

Comment on attachment 107828 [details] [review]
Add configure check for pipe2 to cmake build system

committed

Created attachment 108348 [details] [review]
Include test-dbus and test-bus in cmake 'make check' target

(In reply to Ralf Habacker from comment #32)
> If I comment out the dbus_warn line mentioned below the crash did not happen
> on running ctest -R test-dbus which is in fact called with make check.

That is uninteresting: we already know (Comment #11) that the error being detected is that one fd remains open for some reason. (See Bug #83899 for a bit more background on that check.)

> It may be for interest that running test-dbus with the required environment
> variable directly from a shell or a simple c programm using execve also does
> not crash.

However, this is interesting. Is cmake perhaps running test-dbus with fds beyond 0, 1 and 2 open on startup?

If it is, then either cmake shouldn't do that, or we should call _dbus_close_all() at the beginning of main(), so that the initial state has only 0, 1 and 2 open (and then the check for leaks is "we didn't open anything that we didn't close or mark close-on-exec", as intended). That would also fix Bug #83899, which is the same thing caused by a leaked fd in NetBSD's GUI environment.

(In reply to Simon McVittie from comment #36)
> either cmake shouldn't do that

To be clear, I consider it to be a bug in cmake that it executes our test with extraneous fds open and not marked close-on-exec... but we can work around it with an early _dbus_close_all().

Comment on attachment 108348 [details] [review]
Include test-dbus and test-bus in cmake 'make check' target

Review of attachment 108348 [details] [review]:
-----------------------------------------------------------------

OK to apply when they all pass... which they don't yet.

Comment on attachment 108323 [details] [review]
Add configure checks for accept4 dirfd inotify_init1 and unix_fd_passing

Review of attachment 108323 [details] [review]:
-----------------------------------------------------------------

Fine

Created attachment 108352 [details] [review]
test-bus, test-dbus: close any inherited fds from caller

It is probably a bug for them to pass us any fds without close-on-exec;
but apparently CMake has this bug, and so does at least some NetBSD GUI
environment. Cope.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=73689
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=83899

Created attachment 108353 [details] [review]
cmake: only copy session.conf and system.conf into test  data dir

Historically, CMake used the glob *.conf.in whereas Autotools listed
the files explicitly. This used to be equivalent, but broke down
when we added example-*.conf.in which are just snippets rather than
complete configuration files (they're intended to go in session.d
or system.d, or otherwise get included by the main config file).

---

You will need to delete ${builddir}/test/data/valid-config-files/example-* from your cmake build directory (or delete the whole build directory of course) to clean up after a build that failed in this way.

With Attachment #108323 [details], Attachment #108352 [details], this, and Attachment #108348 [details] (which should be applied last), I get passing tests under cmake on Debian \o/

I don't know whether 108323 is strictly necessary, but at worst it's harmless.

(In reply to Simon McVittie from comment #40)
> Created attachment 108352 [details] [review] [review]
> test-bus, test-dbus: close any inherited fds from caller
> 
> It is probably a bug for them to pass us any fds without close-on-exec;
> but apparently CMake has this bug, and so does at least some NetBSD GUI
> environment. Cope.
> 
> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=73689
> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=83899

It looks like mandatory POSIX behavior to let fd's open http://stackoverflow.com/questions/9583845/why-isnt-close-on-exec-the-default-configuration

Comment on attachment 108323 [details] [review]
Add configure checks for accept4 dirfd inotify_init1 and unix_fd_passing

committed

Comment on attachment 108352 [details] [review]
test-bus, test-dbus: close any inherited fds from caller

Review of attachment 108352 [details] [review]:
-----------------------------------------------------------------

Looks good.

Comment on attachment 108353 [details] [review]
cmake: only copy session.conf and system.conf into test  data dir

Review of attachment 108353 [details] [review]:
-----------------------------------------------------------------

Looks good

Comment on attachment 108353 [details] [review]
cmake: only copy session.conf and system.conf into test  data dir

committed

(In reply to Ralf Habacker from comment #42)
> It looks like mandatory POSIX behavior to let fd's open

POSIX requires that new fds are non-close-on-exec by default, but at the same time, it's generally a bug for an application to "leak" fds beyond the standard three (in/out/err) to its child processes, unless it is doing it deliberately. This is one of the things where the default is now considered to be wrong, but it must still remain wrong in the same way forever, otherwise it would break compatibility.

That's why dbus' regression tests assert that we won't: a well-implemented library shouldn't cause its host process to "leak" fds like that.

Shell redirection like "foo >logs/stdout 2>logs/stderr 3>tmp/fdthree 4>logs/fdfour", and the systemd LISTEN_FDS protocol, are two examples of passing extra fds deliberately.

(In reply to Ralf Habacker from comment #44)
> Comment on attachment 108352 [details] [review] [review]
> test-bus, test-dbus: close any inherited fds from caller
> 
> Review of attachment 108352 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks good.

Just a question: Would it not be better to simply print a related message, but avoid the crash ?

Comment on attachment 108352 [details] [review]
test-bus, test-dbus: close any inherited fds from caller

committed

Comment on attachment 108348 [details] [review]
Include test-dbus and test-bus in cmake 'make check' target

commtted

Comment on attachment 92217 [details] [review]
Crash fix on check_spawn

fixed in the correct way

Comment on attachment 107237 [details]
config.h autotools cmake diff

moved to bug #85418

Comment on attachment 107446 [details]
Command line autotools/cmake macro diff

moved to bug #85418

(In reply to Ralf Habacker from comment #48)
> Just a question: Would it not be better to simply print a related message,
> but avoid the crash ?

No, the tests are meant to fail when a bug is detected. Here, we detected a bug (a fd was leaked to the child process) and failed the test, thus demonstrating a bug, as intended.

Production versions of libdbus ("release" in CMake/Visual Studio terminology) are meant to be compiled without "embedded tests", which omits this check, so they would not crash out in this way.

In this particular case, there were at least three bugs with the same symptom. One was a libdbus bug, which I fixed with Attachment #92295 [details]; one appears to be a CMake or CTest bug, which I worked around with Attachment #108352 [details]; and one was a bug in some GUI component in NetBSD, for which the same workaround is hopefully sufficient.

Now that we have fixed our bug, and compensated for other people's bugs, the tests no longer fail, as intended.