Bug 92721 - bus-test and dbus-test failure on cross compile for windows
Summary: bus-test and dbus-test failure on cross compile for windows
Status: RESOLVED MOVED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.10
Hardware: Other All
: medium normal
Assignee: Ralf Habacker
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review-
Keywords: patch
Depends on:
Blocks: 93069
  Show dependency treegraph
 
Reported: 2015-10-29 05:12 UTC by Ralf Habacker
Modified: 2018-10-12 21:24 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Fix runtime path of DBUS_DATADIR. (1.44 KB, patch)
2015-10-30 05:03 UTC, Ralf Habacker
Details | Splinter Review
Wrap path verbose output with '' to be able to see trailing spaces. (1.40 KB, patch)
2015-10-30 05:18 UTC, Ralf Habacker
Details | Splinter Review
Fix runtime path of DBUS_DATADIR (update 1) (1.44 KB, patch)
2015-10-30 06:46 UTC, Ralf Habacker
Details | Splinter Review
Fix bus-test parser check error caused by using wrong runtime path of DBUS_DATADIR. (1.96 KB, patch)
2015-10-31 10:01 UTC, Ralf Habacker
Details | Splinter Review
Fix bus-test error 'Unknown username "root" on element <deny>' (7.57 KB, patch)
2015-10-31 12:06 UTC, Ralf Habacker
Details | Splinter Review
error.log (115.76 KB, text/plain)
2015-10-31 19:44 UTC, Ralf Habacker
Details
Fix bus-test error 'Unknown username "root" on element <deny>' (update 1) (7.50 KB, patch)
2015-10-31 20:01 UTC, Ralf Habacker
Details | Splinter Review
Fix bus-test parser check error caused by using wrong runtime path of DBUS_DATADIR (update 1) (2.01 KB, patch)
2015-11-02 12:16 UTC, Ralf Habacker
Details | Splinter Review
Test system bus config files on Unix only (9.51 KB, patch)
2015-11-02 14:36 UTC, Simon McVittie
Details | Splinter Review
test_default_session_servicedirs: simplify to a single exit code-path (3.46 KB, patch)
2015-11-02 14:37 UTC, Simon McVittie
Details | Splinter Review
test_default_session_servicedirs: use the intended data directory (2.66 KB, patch)
2015-11-02 14:37 UTC, Simon McVittie
Details | Splinter Review
Don't use _dbus_warn() for intentionally-skipped tests (1.37 KB, patch)
2015-11-02 14:47 UTC, Simon McVittie
Details | Splinter Review
Don't use _dbus_warn() for intentionally-skipped tests (1.55 KB, patch)
2015-11-02 18:21 UTC, Simon McVittie
Details | Splinter Review
Test system bus config files on Unix only (9.51 KB, patch)
2015-11-02 18:22 UTC, Simon McVittie
Details | Splinter Review
Fix missing eol on debug message. (728 bytes, patch)
2015-11-02 19:45 UTC, Ralf Habacker
Details | Splinter Review
Fix warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]. (1.09 KB, patch)
2015-11-02 19:46 UTC, Ralf Habacker
Details | Splinter Review
Fix warning: variable 'ret' set but not used [-Wunused-but-set-variable]. (929 bytes, patch)
2015-11-02 19:47 UTC, Ralf Habacker
Details | Splinter Review
Fix warning: variable 'ret' set but not used [-Wunused-but-set-variable]. (1.30 KB, patch)
2015-11-02 20:57 UTC, Ralf Habacker
Details | Splinter Review
Test system bus config files on Unix only (update 1) (14.06 KB, patch)
2015-11-02 23:18 UTC, Ralf Habacker
Details | Splinter Review
Test system bus config files on Unix only (update 2) (14.47 KB, patch)
2015-11-02 23:25 UTC, Ralf Habacker
Details | Splinter Review
Fix test cases running client and server dispatch design issue. (5.97 KB, patch)
2015-11-05 04:45 UTC, Ralf Habacker
Details | Splinter Review
Fix bug unrefing connection too early in check_hello_message(). (927 bytes, patch)
2015-11-06 13:04 UTC, Ralf Habacker
Details | Splinter Review
Fix test cases running client and server dispatch design issue. (5.80 KB, patch)
2015-11-06 13:04 UTC, Ralf Habacker
Details | Splinter Review
Fix test cases running client and server dispatch design issue. (2.18 KB, patch)
2015-11-06 13:11 UTC, Ralf Habacker
Details | Splinter Review
Fix memory leak in _dbus_win_set_error_from_win_error() with zero error parameter. (839 bytes, patch)
2015-11-08 13:48 UTC, Ralf Habacker
Details | Splinter Review
Fix memory leaks in bus_activation_service_reload_test() in case of errors. (1.41 KB, patch)
2015-11-08 13:48 UTC, Ralf Habacker
Details | Splinter Review
Not not abort test-bus on printing windows todo messages. (1.01 KB, patch)
2015-11-08 21:50 UTC, Ralf Habacker
Details | Splinter Review
Fix test-bus segfault_service_no_auto_start test on windows. (3.13 KB, patch)
2015-11-09 03:40 UTC, Ralf Habacker
Details | Splinter Review
Add tests for GetConnectionWindowsUser and GetConnectionProcessID to test-bus for windows. (15.36 KB, patch)
2015-11-09 04:18 UTC, Ralf Habacker
Details | Splinter Review
Fix memory leak in _dbus_win_set_error_from_win_error() (update 1) (1.60 KB, patch)
2015-11-09 13:18 UTC, Ralf Habacker
Details | Splinter Review
Fix memory leaks in bus_activation_service_reload_test() in case of errors. (1.63 KB, patch)
2015-11-09 13:19 UTC, Ralf Habacker
Details | Splinter Review
Fix test-bus segfault_service_no_auto_start test on windows (update 1) (3.81 KB, patch)
2015-11-09 19:39 UTC, Ralf Habacker
Details | Splinter Review
Skip deprecated test-bus tests for GetConnectionUnixUser and GetConnectionUnixProcessID on Windows. (1.17 KB, patch)
2015-11-09 19:49 UTC, Ralf Habacker
Details | Splinter Review
Skip deprecated test-bus tests for GetConnectionUnixUser and GetConnectionUnixProcessID on Windows. (1.16 KB, patch)
2015-11-10 07:40 UTC, Ralf Habacker
Details | Splinter Review
Do not use dbus_warn for skipping unit tests on windows to avoid fatal errors. (896 bytes, patch)
2015-11-10 07:55 UTC, Ralf Habacker
Details | Splinter Review
Fix memory leak in _dbus_win_set_error_from_win_error() (update 2) (1.15 KB, patch)
2015-11-11 13:31 UTC, Ralf Habacker
Details | Splinter Review
Complete support for dbus-daemon driver methods GetConnectionUnixUser and GetConnectionUnixProcessID on windows. (3.77 KB, patch)
2015-11-11 23:25 UTC, Ralf Habacker
Details | Splinter Review
Skip launch helper activation tests on windows silently. (1.05 KB, patch)
2015-11-11 23:39 UTC, Ralf Habacker
Details | Splinter Review
Fix test-bus test for GetConnectionUnixUser driver method on windows. (1.34 KB, patch)
2015-11-13 18:17 UTC, Ralf Habacker
Details | Splinter Review
Fix test-bus test for GetConnectionUnixUser driver method on windows (update 1) (1.75 KB, patch)
2015-11-13 18:21 UTC, Ralf Habacker
Details | Splinter Review
Do not fail with fatal message skipping GetConnectionUnixProcessID test-bus test on windows. (1.00 KB, patch)
2015-11-13 18:48 UTC, Ralf Habacker
Details | Splinter Review
Fix recursive loop in _dbus_abort() in case DBUS_FATAL_WARNINGS=1 on windows. (949 bytes, patch)
2015-11-13 23:39 UTC, Ralf Habacker
Details | Splinter Review
Fix recursive loop in backtrace generator on windows. (891 bytes, patch)
2015-11-15 18:55 UTC, Ralf Habacker
Details | Splinter Review
Print stack index in backtrace generator on Windows. (1.62 KB, patch)
2015-11-16 19:24 UTC, Ralf Habacker
Details | Splinter Review
Add file and line support to backtrace generator on windows. (3.97 KB, patch)
2015-11-16 19:24 UTC, Ralf Habacker
Details | Splinter Review
Add backtrace test app on windows. (3.29 KB, patch)
2015-11-16 19:24 UTC, Ralf Habacker
Details | Splinter Review
Enable debug symbols on wine. (941 bytes, patch)
2015-11-16 19:25 UTC, Ralf Habacker
Details | Splinter Review
Add backtrace test app on windows. (4.13 KB, patch)
2015-11-16 20:21 UTC, Ralf Habacker
Details | Splinter Review
Add backtrace test app. (4.12 KB, patch)
2015-11-16 20:31 UTC, Ralf Habacker
Details | Splinter Review
Refactor backtrace generator (12.99 KB, patch)
2015-11-16 23:18 UTC, Ralf Habacker
Details | Splinter Review
Add file and line support to backtrace generator on windows. (update1) (4.08 KB, patch)
2015-11-17 15:06 UTC, Ralf Habacker
Details | Splinter Review
Refactor windows backtrace generator to link directly to dbghelp shared library. (12.79 KB, patch)
2015-11-17 15:07 UTC, Ralf Habacker
Details | Splinter Review
Add x86_64 support to backtrace generator on windows. (1.45 KB, patch)
2015-11-17 15:43 UTC, Ralf Habacker
Details | Splinter Review
Workaround with infinite loop while walking along the stack frames on wine64. (1.77 KB, patch)
2015-11-17 15:43 UTC, Ralf Habacker
Details | Splinter Review
Use dwarf-2 debug symbol format on non msvc windows builds to support file and line numbers display on wine backtraces. (1.61 KB, patch)
2015-11-24 13:18 UTC, Ralf Habacker
Details | Splinter Review
Add linking to dbhelp for autotools on Windows which is required for backtrace generator. (741 bytes, patch)
2015-11-27 12:37 UTC, Ralf Habacker
Details | Splinter Review
Add linking to dbghelp for autotools on Windows which is required for backtrace generator. (744 bytes, patch)
2015-11-28 00:10 UTC, Ralf Habacker
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Habacker 2015-10-29 05:12:12 UTC
At Bug 92538 Comment 22 there has been reported that running make check on a cross compile build for windows returns the following errors:

sed -e 's![@]RUN[@]!../bus/test-bus.exe!' \
        < ../../dbus-1/test/tap-test.sh.in > test-bus.sh
FAIL: test-bus.sh 1 ../bus/test-bus.exe (exit status 1)

and

sed -e 's![@]RUN[@]!../dbus/test-dbus.exe!' \
        < ../../dbus-1/test/tap-test.sh.in > test-dbus.sh
FAIL: test-dbus.sh 1 ../dbus/test-dbus.exe (exit status 3)
Comment 1 Ralf Habacker 2015-10-29 05:18:38 UTC
(In reply to Ralf Habacker from comment #0)

> sed -e 's![@]RUN[@]!../bus/test-bus.exe!' \
>         < ../../dbus-1/test/tap-test.sh.in > test-bus.sh
> FAIL: test-bus.sh 1 ../bus/test-bus.exe (exit status 1)
> 

S:/xxx/src/dbus-1-autotools-cross-build/bus/.libs/test-bus.exe: Running config file parser test
Testing retrieving the default session service directories
    default service dir: /usr/local/share/dbus-1/services
    default service dir: C:\Program Files\Common Files/dbus-1/services
    test service dir: /usr/local/share/dbus-1/services
/usr/local/share/dbus-1/services directory does not match S:\xxx\src\dbus-1-autotools-cross-build\dbus\.libs\/usr/local/share/dbus-1/services in the match set
Unit test failed: parser
Comment 2 Simon McVittie 2015-10-29 17:52:19 UTC
You're the Windows expert: which is correct? The "expected" result from the test, the actual result from the test, or neither?
Comment 3 Ralf Habacker 2015-10-29 20:15:08 UTC
(In reply to Simon McVittie from comment #2)
> You're the Windows expert: which is correct? The "expected" result from the
> test, the actual result from the test, or neither?

Because we are cross compiling local host pathes should not be used as default service dir.
Comment 4 Ralf Habacker 2015-10-29 20:34:43 UTC
(In reply to Ralf Habacker from comment #1)
> (In reply to Ralf Habacker from comment #0)
> 
> > sed -e 's![@]RUN[@]!../bus/test-bus.exe!' \
> >         < ../../dbus-1/test/tap-test.sh.in > test-bus.sh
> > FAIL: test-bus.sh 1 ../bus/test-bus.exe (exit status 1)
> > 
> 
> S:/xxx/src/dbus-1-autotools-cross-build/bus/.libs/test-bus.exe: Running
> config file parser test
> Testing retrieving the default session service directories
>     default service dir: /usr/local/share/dbus-1/services
>     default service dir: C:\Program Files\Common Files/dbus-1/services
>     test service dir: /usr/local/share/dbus-1/services
> /usr/local/share/dbus-1/services directory does not match
> S:\xxx\src\dbus-1-autotools-cross-build\dbus\.libs\/usr/local/share/dbus-1/
> services in the match set
> Unit test failed: parser

here is a variant generated from running with a cmake build:

2: Test command: /home/xxx/src/dbus-cmake-cross-x86-build/bin/test-bus.exe
2: Environment variables: 
2:  DBUS_TEST_DAEMON=z:/home/xxx/src/dbus-cmake-cross-x86-build/bin/dbus-daemon.exe
2:  DBUS_SESSION_BUS_ADDRESS=
2:  DBUS_FATAL_WARNINGS=1
2:  DBUS_TEST_DATA=z:/home/xxx/src/dbus-cmake-cross-x86-build/test/data
2:  DBUS_TEST_HOMEDIR=z:/home/xxx/src/dbus-cmake-cross-x86-build/dbus
2: Test timeout computed to be: 9.99988e+06
2: S:\xxx\src\dbus-cmake-cross-x86-build\bin\test-bus.exe: Running expire list test
2: S:\xxx\src\dbus-cmake-cross-x86-build\bin\test-bus.exe: checking for memleaks
2: S:\xxx\src\dbus-cmake-cross-x86-build\bin\test-bus.exe: Running config file parser test
2: Testing retrieving the default session service directories
2:     default service dir: S:/xxx/src/dbus-cmake-cross-x86-build/share/dbus-1/services
2:     default service dir: C:\Program Files\Common Files/dbus-1/services
2:     test service dir: S:/xxx/src/dbus-cmake-cross-x86-build/share/dbus-1/services
2: S:/xxx/src/dbus-cmake-cross-x86-build/share/dbus-1/services directory does not match S:/xxx/src/dbus-cmake-cross-x86-build/share in the match set
2: Unit test failed: parser
Comment 5 Ralf Habacker 2015-10-30 05:03:01 UTC
Created attachment 119290 [details] [review]
Fix runtime path of DBUS_DATADIR.
Comment 6 Ralf Habacker 2015-10-30 05:15:06 UTC
(In reply to Ralf Habacker from comment #5)
> Created attachment 119290 [details] [review] [review]
> Fix runtime path of DBUS_DATADIR.

With that patch I get with cmake build: 

2: Testing retrieving the default session service directories
2:     default service dir: S:/xxx/src/dbus-1-cmake-cross-x86-build/share/dbus-1/services
2:     default service dir: C:\Program Files\Common Files/dbus-1/services
2:     test service dir: 'S:/xxx/src/dbus-1-cmake-cross-x86-build/share/dbus-1/services'
2: 'S:/xxx/src/dbus-1-cmake-cross-x86-build/share/dbus-1/services' directory does not match 'S:/xxx/src/dbus-1-cmake-cross-x86-build/share/dbus-1/services' in the match set
Comment 7 Ralf Habacker 2015-10-30 05:18:12 UTC
Created attachment 119291 [details] [review]
Wrap path verbose output with '' to be able to see trailing spaces.

With this patch applied there are no trailing spaces in the path to see and the default service dir and test service looks equal in term of character case, but the match still fails. 

Then I printed out the length of both strings and got: 

2:     length test service dir: 62
2:     length standard service dir: 63
Comment 8 Ralf Habacker 2015-10-30 06:46:39 UTC
Created attachment 119292 [details] [review]
Fix runtime path of DBUS_DATADIR (update 1)

fixed initialisation of buffer
Comment 9 Ralf Habacker 2015-10-30 06:52:26 UTC
(In reply to Ralf Habacker from comment #7)
 length of both strings and got: 
> 
> 2:     length test service dir: 62
> 2:     length standard service dir: 63

fixed by updated patch. 

The remaining issue on running bus-test is

autotools: 
Unknown username "root" in message bus configuration file

cmake: 
2: Unknown username "root" on element <deny>
Comment 10 Ralf Habacker 2015-10-30 09:41:20 UTC
(In reply to Ralf Habacker from comment #9)
> (In reply to Ralf Habacker from comment #7)
>  length of both strings and got: 
> > 
> > 2:     length test service dir: 62
> > 2:     length standard service dir: 63
> 
> fixed by updated patch. 
> 
> The remaining issue on running bus-test is
> 
> autotools: 
> Unknown username "root" in message bus configuration file
> 
> cmake: 
> 2: Unknown username "root" on element <deny>

These are the relevant files: 

test/data/valid-config-files/many-rules.conf:20:    <deny user="root"/>
test/data/valid-config-files/many-rules.conf:39:    <deny user="root"/>

which need to be setup on configure time. 

If noone objects I would add build system variables DBUS_TEST_ROOT_USER and DBUS_TEST_ROOT_USER for that purpose.

Additional the related config parser need to be extended for windows.
Comment 11 Ralf Habacker 2015-10-31 10:01:28 UTC
Created attachment 119312 [details] [review]
Fix bus-test parser check error caused by using wrong runtime path of DBUS_DATADIR.

Add missing prototype.
Comment 12 Ralf Habacker 2015-10-31 12:06:52 UTC
Created attachment 119313 [details] [review]
Fix bus-test error 'Unknown username "root" on element <deny>'
Comment 13 Ralf Habacker 2015-10-31 19:43:29 UTC
(In reply to Ralf Habacker from comment #12)
> Created attachment 119313 [details] [review] [review]
> Fix bus-test error 'Unknown username "root" on element <deny>'

After applying this patch the next displayed error message is "Expecting NameAcquired, got nothing". See appended file error.log for details
Comment 14 Ralf Habacker 2015-10-31 19:44:24 UTC
Created attachment 119316 [details]
error.log
Comment 15 Ralf Habacker 2015-10-31 20:01:18 UTC
Created attachment 119317 [details] [review]
Fix bus-test error 'Unknown username "root" on element <deny>' (update 1)

remove obsolate debug message
Comment 16 Simon McVittie 2015-11-02 11:55:16 UTC
Comment on attachment 119291 [details] [review]
Wrap path verbose output with '' to be able to see trailing spaces.

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

OK for master, too much noise for 1.10 IMO
Comment 17 Simon McVittie 2015-11-02 11:58:32 UTC
Comment on attachment 119312 [details] [review]
Fix bus-test parser check error caused by using wrong runtime path of DBUS_DATADIR.

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

::: bus/config-parser.c
@@ +3407,5 @@
>    const char *common_progs;
>    char buffer[1024];
>  
> +  strcpy(buffer, _dbus_windows_get_datadir  ());
> +  strcat(buffer, "/dbus-1/services");

Can we please not have buffer overflows in new code? I won't insist on replacing this buffer handling with DBusString (although that would be the fully-correct answer), but strncpy() and strncat() would be an easy improvement.
Comment 18 Ralf Habacker 2015-11-02 12:16:05 UTC
Created attachment 119341 [details] [review]
Fix bus-test parser check error caused by using wrong runtime path of DBUS_DATADIR (update 1)
Comment 19 Simon McVittie 2015-11-02 12:17:20 UTC
Comment on attachment 119317 [details] [review]
Fix bus-test error 'Unknown username "root" on element <deny>' (update 1)

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

This is a lot of code for something we'll probably never use. The well-known system bus is not supported on Windows, and the well-known session bus doesn't need multi-user support.

I'd prefer to copy many-rules.conf into valid-config-files-system/, test that directory on Unix only, and remove the <user> and <group> rules from valid-config-files/many-rules.conf for use on Windows. I'll attach a patch for that.

(I haven't reviewed in detail.)
Comment 20 Ralf Habacker 2015-11-02 12:20:23 UTC
Comment on attachment 119291 [details] [review]
Wrap path verbose output with '' to be able to see trailing spaces.

committed to master
Comment 21 Simon McVittie 2015-11-02 12:24:35 UTC
Comment on attachment 119341 [details] [review]
Fix bus-test parser check error caused by using wrong runtime path of DBUS_DATADIR (update 1)

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

::: bus/config-parser.c
@@ +3407,4 @@
>    const char *common_progs;
>    char buffer[1024];
>  
> +  strncpy(buffer, _dbus_windows_get_datadir (), sizeof (buffer));

Also needs buffer[1023] = '\0' (strncpy does not guarantee to terminate if truncation occurs)

@@ +3407,5 @@
>    const char *common_progs;
>    char buffer[1024];
>  
> +  strncpy(buffer, _dbus_windows_get_datadir (), sizeof (buffer));
> +  strncat(buffer, "/dbus-1/services", sizeof (buffer));

Needs to be sizeof (buffer) - strlen (buffer) - 1, if I've got my pointer arithmetic right.

Using a DBusString might be safer:

DBusString buffer;

if (!_dbus_string_init (&buffer) ||
    !_dbus_string_append (&buffer, _dbus_windows_get_datadir ()) ||
    !_dbus_string_append (&buffer, "/dbus-1/services")))
  /* error: out of memory */

On master, I think we'll need a DBusString anyway, because _dbus_replace_install_prefix() changed its signature.
Comment 22 Ralf Habacker 2015-11-02 12:35:28 UTC
(In reply to Simon McVittie from comment #21)

> Using a DBusString might be safer:
> 
> DBusString buffer;
> 
> if (!_dbus_string_init (&buffer) ||
>     !_dbus_string_append (&buffer, _dbus_windows_get_datadir ()) ||
>     !_dbus_string_append (&buffer, "/dbus-1/services")))
>   /* error: out of memory */
> 
> On master, I think we'll need a DBusString anyway, because
> _dbus_replace_install_prefix() changed its signature.

Unfortunally introducing DBusString at this place requires to add many calls to _dbus_string_free() on any exit below this location.
Comment 23 Simon McVittie 2015-11-02 14:36:51 UTC
Created attachment 119344 [details] [review]
Test system bus config files on Unix only

Previously, we didn't consistently test parsing of every file in
valid-config-files-system/ everywhere that we tested valid-config-files/.
We now test it on Unix.

The system bus is not supported on Windows, so we do not test
valid-config-files-system/ there.

valid-config-files/many-rules.conf contains <user> and <group> rules
which are not applicable to Windows. Copy the original many-rules.conf
to valid-config-files-system/ so that it will be tested on Unix, and
remove the non-portable rules from valid-config-files/many-rules.conf.

---

I'd prefer this, rather than Attachment #119317 [details].
Comment 24 Simon McVittie 2015-11-02 14:37:07 UTC
Created attachment 119345 [details] [review]
test_default_session_servicedirs: simplify to a single  exit code-path

A similar simplification was already done on master as part of commit
f830e14, Bug #83539.
Comment 25 Simon McVittie 2015-11-02 14:37:36 UTC
Created attachment 119346 [details] [review]
test_default_session_servicedirs: use the intended data  directory

If D-Bus was configured for /usr/local and built in Z:/build,
the previous code would use

    Z:/build/dbus/.libs/usr/local/share/dbus-1/services

whereas the intention was to replace the configured prefix /usr/local
with the detected location, more like

    Z:/build/dbus/.libs/share/dbus-1/services
Comment 26 Simon McVittie 2015-11-02 14:39:24 UTC
(In reply to Ralf Habacker from comment #22)
> Unfortunally introducing DBusString at this place requires to add many calls
> to _dbus_string_free() on any exit below this location.

You're right that this would be really annoying without Attachment #119345 [details], but that's why the "goto out" idiom is useful. It's essentially try/finally, but in C; I've even used "goto finally" in other C projects, but "goto out" is the conventional name for this in dbus.
Comment 27 Simon McVittie 2015-11-02 14:47:25 UTC
Created attachment 119347 [details] [review]
Don't use _dbus_warn() for intentionally-skipped tests

The tests are run with _dbus_warn() fatal, so if a particular test is
not applicable on the current platform, we shouldn't call it.

---

With all these patches applied to dbus-1.10, test-dbus can pass in a cross-build "make check" (testing with Debian, Autotools, mingw-w64, Wine 1.6.2).

test-bus still fails, with the error you previously described:

Z:/srv/tmp/smcv/build/dbus/1.10/mingw/bus/.libs/test-bus.exe: Running SHA1 connection test^M
Expecting NameAcquired, got nothing^M

I don't know what's going on there.
Comment 28 Ralf Habacker 2015-11-02 15:11:37 UTC
Comment on attachment 119344 [details] [review]
Test system bus config files on Unix only

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

looks good
Comment 29 Ralf Habacker 2015-11-02 15:16:39 UTC
Comment on attachment 119345 [details] [review]
test_default_session_servicedirs: simplify to a single  exit code-path

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

looks good
Comment 30 Ralf Habacker 2015-11-02 15:23:05 UTC
(In reply to Simon McVittie from comment #27)
> Created attachment 119347 [details] [review] [review]
> Don't use _dbus_warn() for intentionally-skipped tests
> 
> The tests are run with _dbus_warn() fatal, so if a particular test is
> not applicable on the current platform, we shouldn't call it.
> 
> ---
> 
> With all these patches applied to dbus-1.10, test-dbus can pass in a
> cross-build "make check" (testing with Debian, Autotools, mingw-w64, Wine
> 1.6.2).

with cmake test-dbus passed also :-)
1/1 Test #1: test-dbus ........................   Passed   74.83 sec

> 
> test-bus still fails, with the error you previously described:

still have the unknown username "root" failure with cmake

2: 987: [dbus/dbus-file-win.c(134):_dbus_file_get_contents] file z:/home/ralf.habacker/src/dbus-1-cmake-cross-x86-build/test/data\valid-config-files\system.conf hnd 0000008C opened
2: Unknown username "root" in message bus configuration file

> 
> Z:/srv/tmp/smcv/build/dbus/1.10/mingw/bus/.libs/test-bus.exe: Running SHA1
> connection test^M
> Expecting NameAcquired, got nothing^M
> 
> I don't know what's going on there.

gave the appended error.log no hints ?
Comment 31 Ralf Habacker 2015-11-02 17:50:13 UTC
(In reply to Ralf Habacker from comment #30)
> (In reply to Simon McVittie from comment #27)
> > Created attachment 119347 [details] [review] [review] [review]
> > Don't use _dbus_warn() for intentionally-skipped tests
> > 
> > The tests are run with _dbus_warn() fatal, so if a particular test is
> > not applicable on the current platform, we shouldn't call it.
> > 
> > ---
> > 
> > With all these patches applied to dbus-1.10, test-dbus can pass in a
> > cross-build "make check" (testing with Debian, Autotools, mingw-w64, Wine
> > 1.6.2).
> 
> with cmake test-dbus passed also :-)
> 1/1 Test #1: test-dbus ........................   Passed   74.83 sec
> 
> > 
> > test-bus still fails, with the error you previously described:
> 
> still have the unknown username "root" failure with cmake
> 
> 2: 987: [dbus/dbus-file-win.c(134):_dbus_file_get_contents] file
> z:/home/ralf.habacker/src/dbus-1-cmake-cross-x86-build/test/data\valid-
> config-files\system.conf hnd 0000008C opened
> 2: Unknown username "root" in message bus configuration file
> 
> > 
> > Z:/srv/tmp/smcv/build/dbus/1.10/mingw/bus/.libs/test-bus.exe: Running SHA1
> > connection test^M
> > Expecting NameAcquired, got nothing^M
> > 
> > I don't know what's going on there.
> 
> gave the appended error.log no hints ?

From the log it looks that the client expects a signal named NameAcquired to be received, which does not happens in check_hello_message()

... 
      while (!dbus_bus_set_unique_name (connection, name))
        _dbus_wait_for_memory ();

      socd.expected_kind = SERVICE_CREATED;
      socd.expected_service_name = name;
      socd.failed = FALSE;
      socd.skip_connection = connection; /* we haven't done AddMatch so won't get it ourselves */
      bus_test_clients_foreach (check_service_owner_changed_foreach,
                                &socd);

      if (socd.failed)
        goto out;

      name_message = message;
      /* Client should also have gotten ServiceAcquired */

      message = pop_message_waiting_for_memory (connection);
      if (message == NULL)
        {
->>>>     _dbus_warn ("Expecting %s, got nothing\n",
                      "NameAcquired");
          goto out;
        }
Comment 32 Simon McVittie 2015-11-02 18:20:13 UTC
(In reply to Ralf Habacker from comment #30)
> still have the unknown username "root" failure with cmake

I got the CMake part of the patch backwards... will attach a new one.

You'll also need to delete any leftover files named system.conf from your build tree.
Comment 33 Simon McVittie 2015-11-02 18:21:16 UTC
Created attachment 119352 [details] [review]
Don't use _dbus_warn() for intentionally-skipped tests

---

add missing #include
Comment 34 Simon McVittie 2015-11-02 18:22:27 UTC
Created attachment 119353 [details] [review]
Test system bus config files on Unix only

---

Under CMake, put system.conf in v-c-f-system/ and the rest in v-c-f/ (matching Autotools), not the other way round
Comment 35 Simon McVittie 2015-11-02 18:26:16 UTC
(In reply to Ralf Habacker from comment #30)
> gave the appended error.log no hints ?

Sorry, no. I've already spent longer on this today than I should, and DBUS_VERBOSE is very good at producing a lot of irrelevant noise, but not particularly good at pointing to the actual problem :-(
Comment 36 Ralf Habacker 2015-11-02 19:45:36 UTC
Created attachment 119355 [details] [review]
Fix missing eol on debug message.
Comment 37 Ralf Habacker 2015-11-02 19:46:14 UTC
Created attachment 119356 [details] [review]
Fix warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement].
Comment 38 Ralf Habacker 2015-11-02 19:47:34 UTC
Created attachment 119357 [details] [review]
Fix warning: variable 'ret' set but not used [-Wunused-but-set-variable].
Comment 39 Simon McVittie 2015-11-02 19:50:15 UTC
Comment on attachment 119355 [details] [review]
Fix missing eol on debug message.

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

Sure

(also, it should be "peer's pid" if we're being pedantic)
Comment 40 Simon McVittie 2015-11-02 19:51:42 UTC
Comment on attachment 119356 [details] [review]
Fix warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement].

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

Yes please!

The spaces around the brackets should ideally be

= strlen (address) + 1
 ^      ^         ^

if you're touching that line anyway, but no need to fix that at the same time if you don't want to.
Comment 41 Simon McVittie 2015-11-02 19:53:53 UTC
Comment on attachment 119357 [details] [review]
Fix warning: variable 'ret' set but not used [-Wunused-but-set-variable].

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

Yes please, but the unused return might also be a bug in its own right. Should there be some sort of

if (!GetExitCodeProcess (...))
  {
    /* error handling here */
  }

or is GetExitCodeProcess not something that can actually fail in practice?
Comment 42 Ralf Habacker 2015-11-02 20:28:15 UTC
Comment on attachment 119355 [details] [review]
Fix missing eol on debug message.

committed to dbus-1.10
Comment 43 Ralf Habacker 2015-11-02 20:28:46 UTC
Comment on attachment 119356 [details] [review]
Fix warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement].

committed to dbus-1.10 with hints applied
Comment 44 Ralf Habacker 2015-11-02 20:57:28 UTC
Created attachment 119358 [details] [review]
Fix warning: variable 'ret' set but not used [-Wunused-but-set-variable].
Comment 45 Ralf Habacker 2015-11-02 20:59:22 UTC
Comment on attachment 119352 [details] [review]
Don't use _dbus_warn() for intentionally-skipped tests

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

looks good
Comment 46 Ralf Habacker 2015-11-02 22:28:43 UTC
Comment on attachment 119346 [details] [review]
test_default_session_servicedirs: use the intended data  directory

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

look good
Comment 47 Ralf Habacker 2015-11-02 22:30:19 UTC
Comment on attachment 119345 [details] [review]
test_default_session_servicedirs: simplify to a single  exit code-path

committed to dbus-1.10
Comment 48 Ralf Habacker 2015-11-02 22:30:48 UTC
Comment on attachment 119346 [details] [review]
test_default_session_servicedirs: use the intended data  directory

committed to dbus-1.10
Comment 49 Ralf Habacker 2015-11-02 22:32:36 UTC
Comment on attachment 119352 [details] [review]
Don't use _dbus_warn() for intentionally-skipped tests

committed to dbus-1.10
Comment 50 Ralf Habacker 2015-11-02 23:18:11 UTC
Created attachment 119361 [details] [review]
Test system bus config files on Unix only (update 1)

- cmake fixes
- move valid-config-files/system.d to valid-config-files-system
Comment 51 Ralf Habacker 2015-11-02 23:25:34 UTC
Created attachment 119362 [details] [review]
Test system bus config files on Unix only (update 2)

- autotools fixes (location of system.d/test.conf)
Comment 52 Simon McVittie 2015-11-03 11:53:48 UTC
Comment on attachment 119362 [details] [review]
Test system bus config files on Unix only (update 2)

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

Makes sense.

::: cmake/test/CMakeLists.txt
@@ +180,5 @@
>  ENDFOREACH(FILE_TYPE)
>  
>  MESSAGE(STATUS "Copying generated bus config files to test directory")
> +configure_file("${CMAKE_SOURCE_DIR}/../bus/session.conf.in" ${CMAKE_BINARY_DIR}/test/data/valid-config-files/session.conf @ONLY)
> +configure_file("${CMAKE_SOURCE_DIR}/../bus/system.conf.in" ${CMAKE_BINARY_DIR}/test/data/valid-config-files-system/system.conf @ONLY)

Ah, this is a lot simpler than what I had :-)

If the result is equivalent, great, do this.
Comment 53 Simon McVittie 2015-11-03 11:54:30 UTC
Comment on attachment 119358 [details] [review]
Fix warning: variable 'ret' set but not used [-Wunused-but-set-variable].

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

OK
Comment 54 Ralf Habacker 2015-11-03 13:23:50 UTC
Comment on attachment 119358 [details] [review]
Fix warning: variable 'ret' set but not used [-Wunused-but-set-variable].

committed to dbus-1.10
Comment 55 Ralf Habacker 2015-11-03 13:24:05 UTC
Comment on attachment 119362 [details] [review]
Test system bus config files on Unix only (update 2)

committed to dbus-1.10
Comment 56 Ralf Habacker 2015-11-03 17:18:56 UTC
(In reply to Simon McVittie from comment #35)
> (In reply to Ralf Habacker from comment #30)
> > gave the appended error.log no hints ?
> 
> Sorry, no. I've already spent longer on this today than I should, and
> DBUS_VERBOSE is very good at producing a lot of irrelevant noise, but not
> particularly good at pointing to the actual problem :-(

I compared the verbose log file from a test-bus running on unix with the generated from the windows build and got: 

on unix:

19031: [bus/test.c(251):bus_test_run_bus_loop] ---> Done dispatching on "server side"

19031: [dbus/dbus-transport-socket.c(707):do_writing]  wrote 89 bytes of 89
19031: [dbus/dbus-connection.c(660):_dbus_connection_message_sent_unlocked] Message 0x115d1b0 (method_return no path no interface no member 's') removed from outgoing queue 0x
1152510, 0 left to send
...
19031: [dbus/dbus-transport-socket.c(707):do_writing]  wrote 169 bytes of 169
19031: [dbus/dbus-connection.c(660):_dbus_connection_message_sent_unlocked] Message 0x115d540 (signal /org/freedesktop/DBus org.freedesktop.DBus NameAcquired 's') removed from outgoing queue 0x1152510, 0 left to send
...
9031: [bus/test.c(208):bus_test_run_clients_loop] ---> Dispatching on "client side"
...
19031: [dbus/dbus-transport-socket.c(887):do_reading]  read 258 bytes
19031: [dbus/dbus-marshal-header.c(747):_dbus_header_have_message_untrusted] have 258 bytes, need body 9 + header 80 = 89
...
19031: [dbus/dbus-marshal-header.c(747):_dbus_header_have_message_untrusted] have 169 bytes, need body 9 + header 160 = 169

windows:

2: 134: [bus/test.c(233):bus_test_run_bus_loop] ---> Dispatching on "server side"
...
2: 134: [dbus/dbus-transport-socket.c(707):do_writing]  wrote 89 bytes of 89
2: 134: [dbus/dbus-connection.c(660):_dbus_connection_message_sent_unlocked] Message 00245010 (method_return no path no interface no member 's') removed from outgoing queue 00
246368, 0 left to send
...
2: 134: [dbus/dbus-transport-socket.c(707):do_writing]  wrote 169 bytes of 169
2: 134: [dbus/dbus-connection.c(660):_dbus_connection_message_sent_unlocked] Message 0024C980 (signal /org/freedesktop/DBus org.freedesktop.DBus NameAcquired 's') removed from outgoing queue 00246368, 0 left to send
...
2: 134: [bus/test.c(208):bus_test_run_clients_loop] ---> Dispatching on "client side"
...
2: 134: [dbus/dbus-transport-socket.c(887):do_reading]  read 89 bytes
2: 134: [dbus/dbus-marshal-header.c(747):_dbus_header_have_message_untrusted] have 89 bytes, need body 9 + header 80 = 89

that are all bytes gotten from the server side. Looks that the data from the NameAcquired signal (169 Bytes) are send from the server side, but not been received by the client - a tcp buffer issue ?
Comment 57 Ralf Habacker 2015-11-04 06:22:01 UTC
(In reply to Ralf Habacker from comment #56)

> 2: 134: [dbus/dbus-transport-socket.c(887):do_reading]  read 89 bytes
> 2: 134:
> [dbus/dbus-marshal-header.c(747):_dbus_header_have_message_untrusted] have
> 89 bytes, need body 9 + header 80 = 89
> 
> that are all bytes gotten from the server side. Looks that the data from the
> NameAcquired signal (169 Bytes) are send from the server side, but not been
> received by the client - a tcp buffer issue ?

here is a related tcpdump: port 55172 is server and port 57349 is client:

07:02:29.974814 IP localhost.55172 > localhost.57349: Flags [.], ack 1, win 342, options [nop,nop,TS val 22231288 ecr 22231288], length 0
07:02:29.976720 IP localhost.55172 > localhost.57349: Flags [P.], seq 1:2, ack 1, win 342, options [nop,nop,TS val 22231290 ecr 22231288], length 1
07:02:29.976730 IP localhost.57349 > localhost.55172: Flags [.], ack 2, win 342, options [nop,nop,TS val 22231290 ecr 22231290], length 0
07:02:29.976759 IP localhost.55172 > localhost.57349: Flags [P.], seq 2:56, ack 1, win 342, options [nop,nop,TS val 22231290 ecr 22231290], length 54
07:02:29.976766 IP localhost.57349 > localhost.55172: Flags [.], ack 56, win 342, options [nop,nop,TS val 22231290 ecr 22231290], length 0
07:02:30.008782 IP localhost.57349 > localhost.55172: Flags [P.], seq 1:47, ack 56, win 342, options [nop,nop,TS val 22231322 ecr 22231290], length 46
07:02:30.008799 IP localhost.55172 > localhost.57349: Flags [.], ack 47, win 342, options [nop,nop,TS val 22231322 ecr 22231322], length 0
07:02:30.009317 IP localhost.55172 > localhost.57349: Flags [P.], seq 56:118, ack 47, win 342, options [nop,nop,TS val 22231323 ecr 22231322], length 62
07:02:30.020530 IP localhost.57349 > localhost.55172: Flags [P.], seq 47:188, ack 118, win 342, options [nop,nop,TS val 22231334 ecr 22231323], length 141
07:02:30.024247 IP localhost.55172 > localhost.57349: Flags [P.], seq 118:271, ack 188, win 350, options [nop,nop,TS val 22231338 ecr 22231334], length 153
07:02:30.024896 IP localhost.57349 > localhost.55172: Flags [P.], seq 188:225, ack 271, win 350, options [nop,nop,TS val 22231338 ecr 22231338], length 37
07:02:30.025574 IP localhost.55172 > localhost.57349: Flags [P.], seq 271:278, ack 225, win 350, options [nop,nop,TS val 22231339 ecr 22231338], length 7
07:02:30.064969 IP localhost.57349 > localhost.55172: Flags [.], ack 278, win 350, options [nop,nop,TS val 22231379 ecr 22231339], length 0
07:02:30.065009 IP localhost.55172 > localhost.57349: Flags [P.], seq 278:406, ack 225, win 350, options [nop,nop,TS val 22231379 ecr 22231379], length 128
07:02:30.065032 IP localhost.57349 > localhost.55172: Flags [.], ack 406, win 359, options [nop,nop,TS val 22231379 ecr 22231379], length 0

07:02:30.071630 IP localhost.57349 > localhost.55172: Flags [P.], seq 225:314, ack 406, win 359, options [nop,nop,TS val 22231385 ecr 22231379], length 89
-> server send package [1]

07:02:30.075732 IP localhost.55172 > localhost.57349: Flags [F.], seq 406, ack 314, win 350, options [nop,nop,TS val 22231389 ecr 22231385], length 0
-> client reads tcp data and closes the connection (sends FIN flag)

07:02:30.075753 IP localhost.57349 > localhost.55172: Flags [P.], seq 314:483, ack 407, win 359, options [nop,nop,TS val 22231389 ecr 22231389], length 169
-> server sends another packet

07:02:30.075796 IP localhost.55172 > localhost.57349: Flags [R], seq 298084616, win 0, length 0
-> client sends RESET flag

conclusion: test-bus excepts to gets all messages in *one* tcp package, which is not the case on windows/wine. Therefore the NameAcquired signal get's lost.

[1] flags description has been taken https://ask.wireshark.org/questions/31080/tcpdump-output-fpu-and-r-flags
Comment 58 Ralf Habacker 2015-11-04 09:54:08 UTC
(In reply to Ralf Habacker from comment #57)
> conclusion: test-bus excepts to gets all messages in *one* tcp package,
> which is not the case on windows/wine. Therefore the NameAcquired signal
> get's lost.
s/excepts/expects/g, sorry for confusion.
Comment 59 Ralf Habacker 2015-11-04 12:51:49 UTC
(In reply to Ralf Habacker from comment #58)
> (In reply to Ralf Habacker from comment #57)
> > conclusion: test-bus excepts to gets all messages in *one* tcp package,
> > which is not the case on windows/wine. Therefore the NameAcquired signal
> > get's lost.
> s/excepts/expects/g, sorry for confusion.

Tried to run the test on a native windows 7 and did not get the error. Looks like a bug in wine or a emulate-windows-tcp-on-unix-tcp issue.
Comment 60 Ralf Habacker 2015-11-04 19:08:23 UTC
(In reply to Ralf Habacker from comment #59)
> Tried to run the test on a native windows 7 and did not get the error. Looks
> like a bug in wine or a emulate-windows-tcp-on-unix-tcp issue.

which is caused by an issue with wine WSASend called in dbus_write_socket_two().

With WINEDEBUG=trace+winsock I get 
trace:winsock:WS2_sendto socket 0094, wsabuf 0x6cf544, nbufs 2, flags 0, to (nil), tolen 0, ovl (nil), func (nil)
trace:winsock:WS2_sendto fd=15, options=0
trace:winsock:WS2_sendto  -> 89 bytes
trace:winsock:WS2_sendto socket 0094, wsabuf 0x6cf544, nbufs 2, flags 0, to (nil), tolen 0, ovl (nil), func (nil)
trace:winsock:WS2_sendto fd=15, options=0
trace:winsock:WS2_sendto  -> 169 bytes
-> 89+169 = 258 bytes send
trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120
trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6c7130, count 1) <- 1
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6bf12c, count 0) <- 0
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6b7128, count 0) <- 0
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6c7130, count 1) <- 1
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6bf12c, count 0) <- 0
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6b7128, count 0) <- 0
trace:winsock:WS2_recv_base socket 0090, wsabuf 0x6cf808, nbufs 1, flags 0, from (nil), fromlen -1, ovl (nil), func (nil)
trace:winsock:WS2_recv_base fd=15, options=0
trace:winsock:WS2_recv_base  -> 89 bytes
trace:winsock:WS2_recv_base socket 0090, wsabuf 0x6cf808, nbufs 1, flags 0, from (nil), fromlen -1, ovl (nil), func (nil)
trace:winsock:WS2_recv_base fd=15, options=0
trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120
--> this is the last winsock calls
-> 89 bytes received -> incomplete

Replacing WSASend() by two dbus_write_socket calls as already done on the unix variant when WRITEV() is not available, solves the issue. Wine tracing reports then

trace:winsock:WS2_sendto  -> 80 bytes 
trace:winsock:WS2_sendto socket 0094, wsabuf 0x6cf4e8, nbufs 1, flags 0, to (nil), tolen 0, ovl (nil), func (nil)
trace:winsock:WS2_sendto fd=15, options=0
trace:winsock:WS2_sendto  -> 9 bytes
trace:winsock:WS2_sendto socket 0094, wsabuf 0x6cf4e8, nbufs 1, flags 0, to (nil), tolen 0, ovl (nil), func (nil)
trace:winsock:WS2_sendto fd=15, options=0
trace:winsock:WS2_sendto  -> 160 bytes
trace:winsock:WS2_sendto socket 0094, wsabuf 0x6cf4e8, nbufs 1, flags 0, to (nil), tolen 0, ovl (nil), func (nil)
trace:winsock:WS2_sendto fd=15, options=0
trace:winsock:WS2_sendto  -> 9 bytes
-> 89+169 = 258 bytes send
trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120
trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6c7130, count 1) <- 1
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6bf12c, count 0) <- 0
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6b7128, count 0) <- 0
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6c7130, count 1) <- 1
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6bf12c, count 0) <- 0
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6b7128, count 0) <- 0
trace:winsock:WS2_recv_base socket 0090, wsabuf 0x6cf808, nbufs 1, flags 0, from (nil), fromlen -1, ovl (nil), func (nil)
trace:winsock:WS2_recv_base fd=15, options=0
trace:winsock:WS2_recv_base  -> 80 bytes
trace:winsock:WS2_recv_base socket 0090, wsabuf 0x6cf808, nbufs 1, flags 0, from (nil), fromlen -1, ovl (nil), func (nil)
trace:winsock:WS2_recv_base fd=15, options=0
trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120
trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120
trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120
trace:winsock:WS_select read 0x6c7130, write 0x6bf12c, excp 0x6b7128 timeout 0x6b7120
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6c7130, count 1) <- 1
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6bf12c, count 0) <- 0
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6b7128, count 0) <- 0
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6c7130, count 1) <- 1
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6bf12c, count 0) <- 0
trace:winsock:__WSAFDIsSet (socket 0090, fd_set 0x6b7128, count 0) <- 0
trace:winsock:WS2_recv_base socket 0090, wsabuf 0x6cf808, nbufs 1, flags 0, from (nil), fromlen -1, ovl (nil), func (nil)
trace:winsock:WS2_recv_base fd=15, options=0
trace:winsock:WS2_recv_base  -> 178 bytes
--> 80 +178 = 258 -> all bytes received
Comment 61 Ralf Habacker 2015-11-05 04:15:27 UTC
(In reply to Ralf Habacker from comment #60)
> (In reply to Ralf Habacker from comment #59)
> > Tried to run the test on a native windows 7 and did not get the error. Looks
> > like a bug in wine or a emulate-windows-tcp-on-unix-tcp issue.
> 
> which is caused by an issue with wine WSASend called in
> dbus_write_socket_two().
WSASend() is not problem. Adding a simple delay to dbus_read_socket() in the WSAEWOULDBLOCK case fixes the issue.

  bytes_read = recv (fd.sock, data, count, 0);
  
  if (bytes_read == SOCKET_ERROR)
	{
	  DBUS_SOCKET_SET_ERRNO();
	  _dbus_verbose ("recv: failed: %s (%d)\n", _dbus_strerror (errno), errno);
+          if (errno == WSAEWOULDBLOCK)
+             Sleep(50);
	  bytes_read = -1;
	}

(In reply to Ralf Habacker from comment #57)
> (In reply to Ralf Habacker from comment #56)
> 
> conclusion: test-bus excepts to gets all messages in *one* tcp package,
> which is not the case on windows/wine. Therefore the NameAcquired signal get's lost.

The main root cause in dbus test case(s) running the server *and* client loop in the same application is the assumption in hello_check_message() (and probably other locations), that all messages send from the server has to be received in *one* client dispatch, which is not the case in all environments.

In fact running the test cases on wine brings a design issue in dbus test cases to light.
Comment 62 Ralf Habacker 2015-11-05 04:45:07 UTC
Created attachment 119419 [details] [review]
Fix test cases running client and server dispatch design issue.
Comment 63 Simon McVittie 2015-11-06 11:30:39 UTC
Comment on attachment 119419 [details] [review]
Fix test cases running client and server dispatch design issue.

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

Thanks for analyzing this. The fix makes sense in general.

::: bus/dispatch.c
@@ +676,5 @@
>    if (message == NULL)
>      {
> +      dbus_connection_ref (connection);
> +      block_connection_until_message_from_bus (d->context, connection, "NameOwnerChanged");
> +      dbus_connection_unref (connection);

Why this ref/unref? Aren't we holding a ref at this point anyway?

If they are needed, shouldn't the unref be *after* we call pop_message_waiting_for_memory (connection)?
Comment 64 Ralf Habacker 2015-11-06 12:49:30 UTC
(In reply to Simon McVittie from comment #63)
> Comment on attachment 119419 [details] [review] [review]
> Fix test cases running client and server dispatch design issue.
> 
> Review of attachment 119419 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Thanks for analyzing this. The fix makes sense in general.
> 
> ::: bus/dispatch.c
> @@ +676,5 @@
> >    if (message == NULL)
> >      {
> > +      dbus_connection_ref (connection);
> > +      block_connection_until_message_from_bus (d->context, connection, "NameOwnerChanged");
> > +      dbus_connection_unref (connection);
> 
> Why this ref/unref? Aren't we holding a ref at this point anyway?

no, it has been unref'd at http://cgit.freedesktop.org/dbus/dbus/tree/bus/dispatch.c?h=dbus-1.10#n944 . A bug ?
 
> If they are needed, shouldn't the unref be *after* we call
> pop_message_waiting_for_memory (connection)?

copied from http://cgit.freedesktop.org/dbus/dbus/tree/bus/dispatch.c?h=dbus-1.10#n933
Comment 65 Ralf Habacker 2015-11-06 13:04:02 UTC
Created attachment 119437 [details] [review]
Fix bug unrefing connection too early in check_hello_message().
Comment 66 Ralf Habacker 2015-11-06 13:04:59 UTC
Created attachment 119438 [details] [review]
Fix test cases running client and server dispatch design issue.

requires attachment 119437 [details] [review]
Comment 67 Ralf Habacker 2015-11-06 13:11:16 UTC
Created attachment 119439 [details] [review]
Fix test cases running client and server dispatch design issue.

removed obsolate BusContext struct member
Comment 68 Ralf Habacker 2015-11-06 13:14:24 UTC
Comment on attachment 119439 [details] [review]
Fix test cases running client and server dispatch design issue.

revert this patch, context struct member is required
Comment 69 Simon McVittie 2015-11-06 16:50:07 UTC
Comment on attachment 119437 [details] [review]
Fix bug unrefing connection too early in check_hello_message().

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

Go ahead. OK for 1.10, too.
Comment 70 Simon McVittie 2015-11-06 16:51:12 UTC
Comment on attachment 119438 [details] [review]
Fix test cases running client and server dispatch design issue.

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

Looks good now. Also fine for 1.10 I think.
Comment 71 Ralf Habacker 2015-11-06 16:58:58 UTC
Comment on attachment 119437 [details] [review]
Fix bug unrefing connection too early in check_hello_message().

committed to dbus-1.10 and merged into master
Comment 72 Ralf Habacker 2015-11-06 16:59:12 UTC
Comment on attachment 119438 [details] [review]
Fix test cases running client and server dispatch design issue.

committed to dbus-1.10 and merged into master
Comment 73 Ralf Habacker 2015-11-07 11:14:09 UTC
(In reply to Ralf Habacker from comment #72)
> Comment on attachment 119438 [details] [review] [review]
> Fix test cases running client and server dispatch design issue.
> 
> committed to dbus-1.10 and merged into master

Remaining issue is a memory leak: 

S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: checking for memleaks
S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: Running service files reloading test
S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: checking for memleaks
1 dbus_malloc blocks were not freed
Unit test failed: memleaks
Comment 74 Simon McVittie 2015-11-07 16:15:23 UTC
(In reply to Ralf Habacker from comment #73)
> Remaining issue is a memory leak: 
> 
> S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: checking for
> memleaks
> S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: Running service
> files reloading test
> S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: checking for
> memleaks
> 1 dbus_malloc blocks were not freed
> Unit test failed: memleaks

Yeah. This is weird: I wasn't seeing memory leaks reported when I ran this stuff last week, but I ran the tests again yesterday and the Windows cross-builds were leaking. I'm not sure why; nothing we've done recently jumped out at me as a likely source of Windows-specific leaks.

The Unix builds aren't leaking, so my usual leak-debugging techniques (valgrind etc.) are useless here. Do you have better Windows debug tools available?
Comment 75 Ralf Habacker 2015-11-08 12:51:41 UTC
(In reply to Simon McVittie from comment #74)
> (In reply to Ralf Habacker from comment #73)
> > Remaining issue is a memory leak: 
> > 
> > S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: checking for
> > memleaks
> > S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: Running service
> > files reloading test
> > S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: checking for
> > memleaks
> > 1 dbus_malloc blocks were not freed
> > Unit test failed: memleaks
> 
> Yeah. This is weird: I wasn't seeing memory leaks reported when I ran this
> stuff last week, but I ran the tests again yesterday and the Windows
> cross-builds were leaking. I'm not sure why; nothing we've done recently
> jumped out at me as a likely source of Windows-specific leaks.
<> 
> The Unix builds aren't leaking, so my usual leak-debugging techniques
> (valgrind etc.) are useless here. Do you have better Windows debug tools
> available?

I added some test code using a map to track malloc/free's and got: 

S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: checking for memleaks
S:\ralf\src\dbus-1-cmake-cross-x86-build\bin\test-bus.exe: Running service files reloading test
00246098 malloc (File not found.)

There is an error case where a dbus string is not free'd.
Comment 76 Ralf Habacker 2015-11-08 13:48:02 UTC
Created attachment 119478 [details] [review]
Fix memory leak in _dbus_win_set_error_from_win_error() with zero error parameter.
Comment 77 Ralf Habacker 2015-11-08 13:48:33 UTC
Created attachment 119479 [details] [review]
Fix memory leaks in bus_activation_service_reload_test() in case of errors.
Comment 78 Ralf Habacker 2015-11-08 13:49:53 UTC
Just for record: To be able to see debug symbols in winedbg I need to cross compile dbus with '-gdwarf-2 -gstrict-dwarf'.
Comment 79 Ralf Habacker 2015-11-08 20:42:51 UTC
(In reply to Ralf Habacker from comment #76)
> Created attachment 119478 [details] [review] [review]
> Fix memory leak in _dbus_win_set_error_from_win_error() with zero error
> parameter.

This patch is the required one, the other are fixes I found too while analyzing.
Comment 80 Ralf Habacker 2015-11-08 21:50:47 UTC
Created attachment 119488 [details] [review]
Not not abort test-bus on printing windows todo messages.
Comment 81 Ralf Habacker 2015-11-09 03:40:29 UTC
Created attachment 119490 [details] [review]
Fix test-bus segfault_service_no_auto_start test on windows.
Comment 82 Ralf Habacker 2015-11-09 04:18:01 UTC
Created attachment 119491 [details] [review]
Add tests for GetConnectionWindowsUser and GetConnectionProcessID to test-bus for windows.
Comment 83 Ralf Habacker 2015-11-09 04:18:52 UTC
Comment on attachment 119488 [details] [review]
Not not abort test-bus on printing windows todo messages.

superseeded by attachment 119491 [details] [review]
Comment 84 Simon McVittie 2015-11-09 12:00:40 UTC
Comment on attachment 119478 [details] [review]
Fix memory leak in _dbus_win_set_error_from_win_error() with zero error parameter.

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

Well caught, but I don't think this is actually the right fix: I think the current implementation is just wrong, and would leak memory whether error is NULL or not.

>  if (msg)
>    {
>      char *msg_copy;
>
>      msg_copy = dbus_malloc (strlen (msg));

This doesn't allow space for the '\0' at the end, so we're probably overrunning msg_copy by 1 byte.

>      strcpy (msg_copy, msg);

strcpy() is usually wrong. We prefer DBusString, or strncpy() if DBusString really can't be used for some reason.

>      LocalFree (msg);
>
>      dbus_set_error (error, "win32.error", "%s", msg_copy);

msg_copy is never freed here, as far as I can see, so it's leaked. Also, I don't think we even needed to copy it, because dbus_set_error() copies its arguments!

I think the correct fix would be something like this:

FormatMessageA (..., &msg, ...);

if (msg)
  {
    dbus_set_error (error, "win32.error", "%s", msg);
  }
else
  ... what we have now is fine ...

("win32.error" should really also be replaced by one of the documented error names like DBUS_ERROR_FILE_EXISTS, based on a switch() over the error code, but that's a separate bug.)
Comment 85 Simon McVittie 2015-11-09 12:03:25 UTC
Comment on attachment 119479 [details] [review]
Fix memory leaks in bus_activation_service_reload_test() in case of errors.

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

Looks good, although I slightly prefer the pattern of

  dbus_bool_t ret = FALSE;

  if (something failed)
    goto out;

  if (something else failed)
    goto out;

  ret = TRUE;

out:
  free (stuff);
  free (other stuff);
  return ret;

(effectively a C implementation of the C++ try/finally construct)
Comment 86 Simon McVittie 2015-11-09 12:08:25 UTC
Comment on attachment 119490 [details] [review]
Fix test-bus segfault_service_no_auto_start test on windows.

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

::: bus/dispatch.c
@@ +3022,5 @@
> +#ifdef DBUS_WIN
> +      else if (dbus_message_is_error (message,
> +                                      DBUS_ERROR_SPAWN_CHILD_EXITED))
> +        {
> +          ; /* good, this is expected also */

Am I right in thinking that this is expected because on Unix we can reliably tell the difference between a child that was killed by a signal and a child that gracefully exited nonzero, but on Windows both of those are just treated as a nonzero exit?

If that's what is going on here, then the change is fine, but I'd prefer to have a comment explaining why the expected result is different on Unix vs Windows.

I know you're just copying what the current code does, but the semicolon is unnecessary here: it's fine to have a block containing only a comment, like

else if (...)
  {
    /* Windows exit codes don't distinguish between segfault and
     * graceful unsuccessful exit in the same way Unix does */
  }

and some compilers will warn about the empty statement.
Comment 87 Simon McVittie 2015-11-09 12:21:08 UTC
Comment on attachment 119491 [details] [review]
Add tests for GetConnectionWindowsUser and GetConnectionProcessID to test-bus for windows.

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

The commit message seems rather backwards: the primary thing you're doing here is adding new functionality (the new methods); secondarily, you're testing it.

I don't want these two new methods, because GetConnectionCredentials() covers both of them, and only needs one round-trip.

::: bus/driver.c
@@ +2227,5 @@
>      DBUS_TYPE_STRING_AS_STRING,
>      DBUS_TYPE_ARRAY_AS_STRING DBUS_TYPE_STRING_AS_STRING,
>      bus_driver_handle_list_queued_owners },
> +#ifdef DBUS_WIN
> +    { "GetConnectionWindowsUser",

Please don't add new methods to get individual credentials. They lead to unnecessary round-trips in the common case that a caller wants to know multiple credentials (user ID, process ID and LSM context on Unix; SID and process ID on Windows). New code should use GetConnectionCredentials() on all platforms. We're only keeping the Unix-specific ones for backwards compatibility with things that already rely on them.

If new methods are added to the o.fd.DBus interface, they should not be platform-specific: the contents of an interface should not change on different platforms. If a method cannot work on a particular platform, it should exist but return an error, just like GetConnectionUnixUser() returns an error on Windows.

@@ +2231,5 @@
> +    { "GetConnectionWindowsUser",
> +      DBUS_TYPE_STRING_AS_STRING,
> +      DBUS_TYPE_UINT32_AS_STRING,
> +      bus_driver_handle_get_connection_windows_user },
> +    { "GetConnectionProcessID",

Definitely don't add this, because GetConnectionUnixProcessID() (despite its name, which we have to keep for historical reasons) would be fine to make cross-platform.
Comment 88 Ralf Habacker 2015-11-09 13:18:41 UTC
Created attachment 119509 [details] [review]
Fix memory leak in _dbus_win_set_error_from_win_error() (update 1)
Comment 89 Ralf Habacker 2015-11-09 13:19:06 UTC
Created attachment 119510 [details] [review]
Fix memory leaks in bus_activation_service_reload_test() in case of errors.
Comment 90 Ralf Habacker 2015-11-09 15:13:08 UTC
(In reply to Simon McVittie from comment #86)
Y
> Am I right in thinking that this is expected because on Unix we can reliably
> tell the difference between a child that was killed by a signal and a child
> that gracefully exited nonzero, but on Windows both of those are just
> treated as a nonzero exit?
> 
yes from https://msdn.microsoft.com/de-de/library/windows/desktop/ms683189%28v=vs.85%29.aspx
"... 
If the process has terminated and the function succeeds, the status returned is one of the following values:

    The exit value specified in the ExitProcess or TerminateProcess function.
    The return value from the main or WinMain function of the process.
    The exception value for an unhandled exception that caused the process to terminate."

> If that's what is going on here, then the change is fine, but I'd prefer to
> have a comment explaining why the expected result is different on Unix vs
> Windows.

... because on unix DBUS_ERROR_SPAWN_CHILD_SIGNALED indicates a signaled child, which could also be used on windows if we find a way to distinguish normal exit codes from signal codes at http://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-spawn-win.c?h=dbus-1.10#n343. One indication are the high negative values for example 0xc00000005 -> segfault 

> 
> I know you're just copying what the current code does, but the semicolon is
> unnecessary here: 
will fix this
Comment 91 Ralf Habacker 2015-11-09 15:51:42 UTC
(In reply to Ralf Habacker from comment #90)
>
> ... because on unix DBUS_ERROR_SPAWN_CHILD_SIGNALED indicates a signaled
> child, which could also be used on windows if we find a way to distinguish
> normal exit codes from signal codes at
> http://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-spawn-win.c?h=dbus-1.
> 10#n343. One indication are the high negative values for example 0xc00000005
> -> segfault 

cygwin for example detects an unhandled exception by comparing the exit code with >= 0xc00000000  https://cygwin.com/ml/cygwin-patches/2008-q1/msg00063.html.
Comment 92 Ralf Habacker 2015-11-09 19:39:26 UTC
Created attachment 119522 [details] [review]
Fix test-bus segfault_service_no_auto_start test on windows (update 1)

Note: Using DBUS_ERROR_SPAWN_CHILD_EXITED looks more natural on windows as adding an extra emulation layer.
Comment 93 Ralf Habacker 2015-11-09 19:42:06 UTC
(In reply to Simon McVittie from comment #87)
> 
> I don't want these two new methods, because GetConnectionCredentials()
> covers both of them, and only needs one round-trip.

Then those tests should be skipped on windows ?
Comment 94 Ralf Habacker 2015-11-09 19:49:50 UTC
Created attachment 119523 [details] [review]
Skip deprecated test-bus tests for GetConnectionUnixUser and GetConnectionUnixProcessID on Windows.
Comment 95 Ralf Habacker 2015-11-10 07:40:24 UTC
Created attachment 119529 [details] [review]
Skip deprecated test-bus tests for GetConnectionUnixUser and GetConnectionUnixProcessID on Windows.

fixed wrong base
Comment 96 Ralf Habacker 2015-11-10 07:55:06 UTC
Created attachment 119530 [details] [review]
Do not use dbus_warn for skipping unit tests on windows to avoid fatal errors.
Comment 97 Simon McVittie 2015-11-11 11:02:59 UTC
Comment on attachment 119522 [details] [review]
Fix test-bus segfault_service_no_auto_start test on windows (update 1)

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

Looks good, thanks
Comment 98 Ralf Habacker 2015-11-11 11:16:33 UTC
Comment on attachment 119522 [details] [review]
Fix test-bus segfault_service_no_auto_start test on windows (update 1)

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

found little spelling issue  s/and by/and not by/. Will commit with this fix
Comment 99 Ralf Habacker 2015-11-11 11:20:34 UTC
Comment on attachment 119522 [details] [review]
Fix test-bus segfault_service_no_auto_start test on windows (update 1)

committed to dbus-1.10
Comment 100 Simon McVittie 2015-11-11 12:29:31 UTC
Comment on attachment 119530 [details] [review]
Do not use dbus_warn for skipping unit tests on windows to avoid fatal errors.

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

OK. It would also be OK to skip testing the launch helper without any comment at all: it makes no sense on Windows (the system bus is unsupported there, and I don't think Windows has any concept of setuid binaries, so it can't ever actually work).
Comment 101 Simon McVittie 2015-11-11 12:35:42 UTC
Comment on attachment 119529 [details] [review]
Skip deprecated test-bus tests for GetConnectionUnixUser and GetConnectionUnixProcessID on Windows.

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

The functional change is fine, but these tests are not actually deprecated as such. The methods that they're testing are weakly deprecated in favour of GetConnectionCredentials(), but must still be supported on Unix. On Windows, they're unimportant, and I don't mind continuing to not test them on Windows: presumably they did not traditionally work anyway, and they're mainly useful for the system bus, which really only makes sense on Unix.

GetConnectionUnixUser() is meaningless on Windows and should just raise an error. Ideally, its test would verify that it raises an error #ifdef DBUS_WIN, or succeeds otherwise. "Out of memory" is a valid result either way, because of how these tests are structured.

I think GetConnectionUnixProcessID() might actually be able to succeed on Windows now (despite its name!) since you added that functionality a few months ago? But if it fails, that's OK too. Again, "out of memory" is also a valid result on either platform.
Comment 102 Simon McVittie 2015-11-11 12:39:38 UTC
Comment on attachment 119509 [details] [review]
Fix memory leak in _dbus_win_set_error_from_win_error() (update 1)

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

::: dbus/dbus-sysdeps-win.c
@@ +2377,4 @@
>  # define BACKTRACES
>  #endif
>  
> +#undef BACKTRACES

Why?

If the backtrace code is broken or otherwise not useful (I've never had it work on Wine, but perhaps I'm using wrong toolchain settings), we should just delete it. If it's useful, we can keep it.

@@ +3604,5 @@
>  {
>    char *msg;
>  
> +  if (!error)
> +      return;

OK: minor optimization (not doing unnecessary work), harmless. Ideally this would be a separate commit though.

@@ +3615,4 @@
>                    (LPSTR) &msg, 0, NULL);
>    if (msg)
>      {
> +      dbus_set_error (error, "win32.error", "%s", msg);

OK: this is the actual bug fix.
Comment 103 Simon McVittie 2015-11-11 12:40:23 UTC
Comment on attachment 119510 [details] [review]
Fix memory leaks in bus_activation_service_reload_test() in case of errors.

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

Yes please!
Comment 104 Ralf Habacker 2015-11-11 13:31:44 UTC
Created attachment 119562 [details] [review]
Fix memory leak in _dbus_win_set_error_from_win_error() (update 2)
Comment 105 Ralf Habacker 2015-11-11 13:32:21 UTC
Comment on attachment 119510 [details] [review]
Fix memory leaks in bus_activation_service_reload_test() in case of errors.

committed to dbus-1.10
Comment 106 Simon McVittie 2015-11-11 13:37:23 UTC
Comment on attachment 119562 [details] [review]
Fix memory leak in _dbus_win_set_error_from_win_error() (update 2)

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

Perfect
Comment 107 Ralf Habacker 2015-11-11 14:06:40 UTC
Comment on attachment 119562 [details] [review]
Fix memory leak in _dbus_win_set_error_from_win_error() (update 2)

committed to dbus-1.10
Comment 108 Ralf Habacker 2015-11-11 23:25:19 UTC
Created attachment 119575 [details] [review]
Complete support for dbus-daemon driver methods GetConnectionUnixUser and GetConnectionUnixProcessID on windows.
Comment 109 Ralf Habacker 2015-11-11 23:32:54 UTC
(In reply to Simon McVittie from comment #101)

> GetConnectionUnixUser() is meaningless on Windows and should just raise an
> error. Ideally, its test would verify that it raises an error #ifdef
> DBUS_WIN, or succeeds otherwise. "Out of memory" is a valid result either
> way, because of how these tests are structured.
I choosed the already available error code DBUS_ERROR_NOT_SUPPORTED.
 
> I think GetConnectionUnixProcessID() might actually been able to succeed on
> Windows now (despite its name!) since you added that functionality a few
> months ago? 
Recent patch enable it for windows, but after implementing I'm coming to the conclusion, that it should better fail and return a hint to use GetConnectionCredentials instead. The hint would also be useful for GetConnectionUnixUser.
Comment 110 Ralf Habacker 2015-11-11 23:35:07 UTC
(In reply to Simon McVittie from comment #102)
> > +#undef BACKTRACES
> 
removed from patch 
> 
> If the backtrace code is broken or otherwise not useful (I've never had it
> work on Wine, but perhaps I'm using wrong toolchain settings), we should
> just delete it. If it's useful, we can keep it.
Need to take a deeper look.
Comment 111 Ralf Habacker 2015-11-11 23:39:51 UTC
Created attachment 119576 [details] [review]
Skip launch helper activation tests on windows silently.
Comment 112 Simon McVittie 2015-11-12 10:15:15 UTC
Comment on attachment 119576 [details] [review]
Skip launch helper activation tests on windows silently.

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

OK
Comment 113 Simon McVittie 2015-11-12 10:24:29 UTC
Comment on attachment 119575 [details] [review]
Complete support for dbus-daemon driver methods GetConnectionUnixUser and GetConnectionUnixProcessID on windows.

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

::: bus/dispatch.c
@@ -1421,5 @@
> -        {
> -          /* We are expecting this error, since we know in the test suite we aren't
> -           * talking to a client running on UNIX
> -           */
> -          _dbus_verbose ("Windows correctly does not support GetConnectionUnixProcessID\n");

If we now have a fully working GetConnectionUnixProcessID() on Windows (i.e. it returns the pid of the other end of the connection), then this part is fine.

@@ -1446,5 @@
>    else
>      {
> -#ifdef DBUS_WIN
> -      warn_unexpected (connection, message, "GetConnectionUnixProcessID to fail on Windows");
> -      goto out;

This too.

@@ -4838,5 @@
>  #endif
>  
> -#ifdef DBUS_WIN
> -  _dbus_verbose("Skipping deprecated test for GetConnectionUnixUser\n");
> -  _dbus_verbose("Skipping deprecated test for GetConnectionUnixProcessID\n");

This part is fine, assuming you can make those two tests pass (either by changing the implementation or by changing the expectation)

::: bus/driver.c
@@ +1463,5 @@
> +#ifdef DBUS_WIN
> +  dbus_set_error (error,
> +                  DBUS_ERROR_NOT_SUPPORTED,
> +                  "Not supported on windows platform");
> +  goto failed;

You shouldn't need an explicit #ifdef DBUS_WIN here, because dbus_connection_get_unix_user() already returns FALSE whenever we don't know the uid of the other end of the connection.

On Windows, the other end of the connection doesn't *have* a uid (there is no such thing). It looks as though we currently raise DBUS_ERROR_FAILED for that, the same as we would on Unix for a connection that has used ANONYMOUS authentication. I think that error is fine: there's no need for a special case for Windows here.
Comment 114 Ralf Habacker 2015-11-12 11:57:50 UTC
Comment on attachment 119576 [details] [review]
Skip launch helper activation tests on windows silently.

committed to dbus-1.10
Comment 115 Ralf Habacker 2015-11-12 11:58:47 UTC
Comment on attachment 119529 [details] [review]
Skip deprecated test-bus tests for GetConnectionUnixUser and GetConnectionUnixProcessID on Windows.

required for attachment 119575 [details] [review]
Comment 116 Ralf Habacker 2015-11-12 14:58:09 UTC
(In reply to Simon McVittie from comment #113)

> ::: bus/driver.c
> @@ +1463,5 @@
> > +#ifdef DBUS_WIN
> > +  dbus_set_error (error,
> > +                  DBUS_ERROR_NOT_SUPPORTED,
> > +                  "Not supported on windows platform");
> > +  goto failed;
> 
> You shouldn't need an explicit #ifdef DBUS_WIN here, because
> dbus_connection_get_unix_user() already returns FALSE whenever we don't know
> the uid of the other end of the connection.
> 
> On Windows, the other end of the connection doesn't *have* a uid (there is
> no such thing). It looks as though we currently raise DBUS_ERROR_FAILED for
> that, the same as we would on Unix for a connection that has used ANONYMOUS
> authentication. I think that error is fine: there's no need for a special case for Windows here.

recent implementation of check_get_connection_unix_user() do not check the DBUS_ERROR_FAILED error: 

8: [bus/dispatch.c(640):verbose_message_received] Received message interface "(unset)" member "(unset)" error name "org.freedesktop.DBus.Error.Failed" on 0024F4A8
check_get_connection_unix_user:1290 received message interface "(unset)" member "(unset)" error name "org.freedesktop.DBus.Error.Failed" on 0024F4A8, expecting not this error

It is required to add a dedicated catch. 


Also the  message returned from the driver: 

  "Could not determine UID for '%s'", service);

is very unspecific and forces the user to take additional time to find out the real reason. Instead returning 

 "Not supported, use GetConnectionCredentials instead");

would give a helpful explanation of the problem.
Comment 117 Simon McVittie 2015-11-12 15:37:48 UTC
(In reply to Ralf Habacker from comment #116)
> recent implementation of check_get_connection_unix_user() do not check the
> DBUS_ERROR_FAILED error: 
> 
> 8: [bus/dispatch.c(640):verbose_message_received] Received message interface
> "(unset)" member "(unset)" error name "org.freedesktop.DBus.Error.Failed" on
> 0024F4A8
> check_get_connection_unix_user:1290 received message interface "(unset)"
> member "(unset)" error name "org.freedesktop.DBus.Error.Failed" on 0024F4A8,
> expecting not this error
> 
> It is required to add a dedicated catch. 

OK, then something like

#ifdef DBUS_WIN
  else if (it is FAILED)
    {
      /* this is OK, Unix uids aren't meaningful on Windows */
    }
#endif

in the test-case would be appropriate.

> Also the  message returned from the driver: 
> 
>   "Could not determine UID for '%s'", service);
> 
> is very unspecific and forces the user to take additional time to find out
> the real reason.

What would it mean for a peer to have a Unix uid on a Windows system? What Unix uid could it possibly have?

What would a caller do with that uid? They can't compare it with the result of geteuid(), because Windows doesn't have that function. They can't usefully compare it with (struct stat)->st_uid, because that struct member is always 0 on Windows. And so on.

> Instead returning 
> 
>  "Not supported, use GetConnectionCredentials instead");
> 
> would give a helpful explanation of the problem.

Not particularly helpful. If GetConnectionUnixUser() fails, then UnixUserID would not be in the result of GetConnectionCredentials() either: they give the same information, in a different format.

If you are working with identity information on Windows, then you need to know that the operating system represents that as a string SID instead of a numeric uid, and use APIs that give you that information (like GetConnectionCredentials)... but presumably you're doing something with that information, like comparing it with your own SID, in which case you already need to know that SIDs are strings.

GetConnectionCredentials and its semi-deprecated ancestors are mainly useful on the system bus, which Windows doesn't have. The session bus is not designed to be a security boundary, so session services can assume that every peer has the same user ID (uid or SID) that the service does, because the session bus does not have <allow user="*"/>; that means connections that do not have the same uid or SID are rejected (auth_via_default_rules() in dbus/dbus-transport.c calls _dbus_credentials_same_user() which knows about both uids and SIDs).
Comment 118 Ralf Habacker 2015-11-13 18:17:39 UTC
Created attachment 119649 [details] [review]
Fix test-bus test for GetConnectionUnixUser driver method on windows.
Comment 119 Ralf Habacker 2015-11-13 18:21:55 UTC
Created attachment 119650 [details] [review]
Fix test-bus test for GetConnectionUnixUser driver method on windows (update 1)
Comment 120 Ralf Habacker 2015-11-13 18:48:46 UTC
Created attachment 119651 [details] [review]
Do not fail with fatal message skipping GetConnectionUnixProcessID test-bus test on windows.
Comment 121 Simon McVittie 2015-11-13 19:41:17 UTC
Comment on attachment 119650 [details] [review]
Fix test-bus test for GetConnectionUnixUser driver method on windows (update 1)

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

Nice. OK for 1.10 too
Comment 122 Simon McVittie 2015-11-13 19:42:07 UTC
Comment on attachment 119651 [details] [review]
Do not fail with fatal message skipping GetConnectionUnixProcessID test-bus test on windows.

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

Fine

::: bus/dispatch.c
@@ +4847,3 @@
>      _dbus_assert_not_reached ("GetConnectionUnixUser message failed");
>  
>  #ifdef DBUS_WIN_FIXME

Out of interest, where do we #define this, and can we make it just be DBUS_WIN?
Comment 123 Ralf Habacker 2015-11-13 22:06:35 UTC
Comment on attachment 119650 [details] [review]
Fix test-bus test for GetConnectionUnixUser driver method on windows (update 1)

committed to dbus-1.10
Comment 124 Ralf Habacker 2015-11-13 22:06:49 UTC
Comment on attachment 119651 [details] [review]
Do not fail with fatal message skipping GetConnectionUnixProcessID test-bus test on windows.

committed to dbus-1.10
Comment 125 Ralf Habacker 2015-11-13 22:21:21 UTC
(In reply to Simon McVittie from comment #122)
>  #ifdef DBUS_WIN_FIXME
> 
> Out of interest, where do we #define this, 

cmake/config.h.cmake

> and can we make it just be DBUS_WIN?

The recent situation looks somehow unfinished: 
1. By implementing GetConnectionCredentials on windows deprecated method GetConnectionUnixProcessID has also been enabled, is available on windows and may be used by applications.

2. By reverting this implicit enable and let GetConnectionUnixProcessID now fail on windows may break applications, so it has to stay :-(

3. Persistently skipping the testcase for the working GetConnectionUnixProcessID on windows by renaming the define to DBUS_WIN may hide regressions introduced in the future.

BTW: DBUS_WIN_FIXME is also defined in _dbus_decrement_fail_alloc_counter.
Comment 126 Ralf Habacker 2015-11-13 23:39:42 UTC
Created attachment 119655 [details] [review]
Fix recursive loop in _dbus_abort() in case DBUS_FATAL_WARNINGS=1 on windows.

running bus-test with environment variable DBUS_FATAL_WARNINGS=1 and calls to dbus_warn() (for example without attachment 119651 [details] [review]) prints out

Backtrace:
Backtrace:
Backtrace:
Backtrace:
Backtrace:
Backtrace:
...
Comment 127 Ralf Habacker 2015-11-13 23:41:43 UTC
(In reply to Ralf Habacker from comment #110)

> > If the backtrace code is broken or otherwise not useful (I've never had it
> > work on Wine, but perhaps I'm using wrong toolchain settings), we should
> > just delete it. If it's useful, we can keep it.
> Need to take a deeper look.

backtrace support needs attachment 119655 [details] [review]
Comment 128 Simon McVittie 2015-11-15 13:01:37 UTC
Comment on attachment 119655 [details] [review]
Fix recursive loop in _dbus_abort() in case DBUS_FATAL_WARNINGS=1 on windows.

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

::: dbus/dbus-sysdeps.c
@@ +79,5 @@
>    const char *s;
> +  static dbus_bool_t active = FALSE;
> +
> +  if (active)
> +      return;

This makes me uncomfortable, because _dbus_abort() should never return.

@@ +86,1 @@
>    _dbus_print_backtrace ();

Instead of doing this, I'd prefer to make the Windows implementation of _dbus_print_backtrace() use fprintf (stderr, ...) instead of _dbus_warn (...).

The author of the Unix implementation was aware of this issue: it has this comment:

     /* don't use dbus_warn since it can _dbus_abort() */
      fprintf (stderr, "  %s\n", syms[i]);

but unfortunately the same is not true of the Windows implementation.

As far as I can see, the necessary change to dbus-sysdeps-win.c would be to change

#define DPRINTF _dbus_warn

to something like

#define DPRINTF(fmt, ...) fprintf (stderr, fmt, ##__VA_ARGS__)

or alternatively, replace every call to DPRINTF with the corresponding fprintf to stderr. DPRINTF is only used in the backtracing code, as far as I can tell.
Comment 129 Simon McVittie 2015-11-15 13:07:21 UTC
(In reply to Ralf Habacker from comment #125)
> 1. By implementing GetConnectionCredentials on windows deprecated method
> GetConnectionUnixProcessID has also been enabled, is available on windows
> and may be used by applications.

That's fine. The method is deprecated, but we have to continue to support it (at least on Unix) anyway, because applications use it; and 99% of the implementation is shared with GetConnectionCredentials().

If dbus-daemon on Windows returns a process ID in GetConnectionCredentials(), then there is no real harm in having GetConnectionUnixProcessID() also work (despite its name) - again, 99% of the implementation is needed for GetConnectionCredentials() anyway.

The method name was presumably chosen because the author of the relevant bit of the specification wasn't sure whether there was an equivalent of pid_t on non-Unix, but when we discussed it during implementation of GetConnectionCredentials(), we came to the conclusion that Unix pid_t and Windows process IDs are essentially equivalent - they're a positive integer that identifies a local process - so there's no real point in distinguishing.
Comment 130 Ralf Habacker 2015-11-15 18:55:12 UTC
Created attachment 119689 [details] [review]
Fix recursive loop in backtrace generator on windows.
Comment 131 Simon McVittie 2015-11-16 10:50:02 UTC
Comment on attachment 119689 [details] [review]
Fix recursive loop in backtrace generator on windows.

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

Looks good. I think this is sufficiently narrow for 1.10.
Comment 132 Ralf Habacker 2015-11-16 18:32:59 UTC
Comment on attachment 119689 [details] [review]
Fix recursive loop in backtrace generator on windows.

committed to dbus-1.10
Comment 133 Ralf Habacker 2015-11-16 19:24:06 UTC
Created attachment 119710 [details] [review]
Print stack index in backtrace generator on Windows.
Comment 134 Ralf Habacker 2015-11-16 19:24:25 UTC
Created attachment 119711 [details] [review]
Add file and line support to backtrace generator on windows.
Comment 135 Ralf Habacker 2015-11-16 19:24:41 UTC
Created attachment 119712 [details] [review]
Add backtrace test app on windows.
Comment 136 Ralf Habacker 2015-11-16 19:25:01 UTC
Created attachment 119713 [details] [review]
Enable debug symbols on wine.
Comment 137 Simon McVittie 2015-11-16 19:31:41 UTC
Comment on attachment 119710 [details] [review]
Print stack index in backtrace generator on Windows.

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

OK, but only for master.
Comment 138 Simon McVittie 2015-11-16 19:46:22 UTC
Comment on attachment 119711 [details] [review]
Add file and line support to backtrace generator on windows.

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

::: dbus/dbus-sysdeps-win.c
@@ +2550,5 @@
> +  HANDLE hProcess,
> +  DWORD dwAddr,
> +  PDWORD pdwDisplacement,
> +  PIMAGEHLP_LINE Line
> +))GetProcAddress (hmodDbgHelp, FUNC(SymGetLineFromAddr));

This indentation is really quite weird.

It's functionally OK, but I'd prefer something like this:

/* globally */
typedef BOOL (WINAPI *SymGetLineFromAddrImpl) (HANDLE hProcess,
    DWORD dwAddr,
    PDWORD pdwDisplacement,
    PIMAGEHLP_LINE Line);
static SymGetLineFromAddrImpl pSymGetLineFromAddr;


  /* in the function */
  pSymGetLineFromAddr = (SymGetLineFromAddrImpl) GetProcAddress (hmodDbgHelp,
      FUNC (SymGetLineFromAddr));

@@ +2638,5 @@
>  
>          pSymbol->SizeOfStruct = sizeof(IMAGEHLP_SYMBOL);
>          pSymbol->MaxNameLength = sizeof(buffer) - sizeof(IMAGEHLP_SYMBOL) + 1;
>  
> +        if (pSymGetSymFromAddr(GetCurrentProcess(), sf.AddrPC.Offset,

Whitespace nitpicking: pSymGetEtcEtc (GetCurrentProcess (), sf.AddrPC.Offset,
                                    ^                  ^
with the spaces before the opening parentheses

@@ +2650,2 @@
>          else
> +          DPRINTF ("%3d 0x%lx", i++, sf.AddrPC.Offset);

Is Offset guaranteed to be the same size as long, even on 64-bit?

@@ +2650,5 @@
>          else
> +          DPRINTF ("%3d 0x%lx", i++, sf.AddrPC.Offset);
> +
> +        line.SizeOfStruct = sizeof(IMAGEHLP_LINE);
> +        if (pSymGetLineFromAddr(GetCurrentProcess(), sf.AddrPC.Offset, &dwDisplacement, &line) == TRUE)

We normally avoid "== TRUE". If the function returns a boolean result, it's unnecessary; if the function returns a multi-valued integer so the distinction between "nonzero" and "exactly 1" matters, use "== 1" or "== SOME_ENUM_MEMBER".

@@ +2652,5 @@
> +
> +        line.SizeOfStruct = sizeof(IMAGEHLP_LINE);
> +        if (pSymGetLineFromAddr(GetCurrentProcess(), sf.AddrPC.Offset, &dwDisplacement, &line) == TRUE)
> +          {
> +            DPRINTF (" [%s:%ld]", line.FileName, line.LineNumber);

Is LineNumber guaranteed to be the same size as long, even on 64-bit?
Comment 139 Simon McVittie 2015-11-16 19:50:27 UTC
Comment on attachment 119712 [details] [review]
Add backtrace test app on windows.

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

::: cmake/test/CMakeLists.txt
@@ +74,4 @@
>  add_helper_executable(test-sleep-forever ${test-sleep-forever_SOURCES} ${DBUS_INTERNAL_LIBRARIES})
>  add_test_executable(manual-tcp ${manual-tcp_SOURCES} ${DBUS_INTERNAL_LIBRARIES})
>  if(WIN32)
> +    add_helper_executable(manual-backtrace ${CMAKE_SOURCE_DIR}/../test/manual-backtrace.c ${DBUS_INTERNAL_LIBRARIES} dbus-1)

I don't think this needs ${DBUS_INTERNAL_LIBRARIES} - if you need to put DBUS_PRIVATE_EXPORT on the symbols, then they must be part of libdbus-1.

Is there any reason why this can't be built on Unix too, like manual-tcp?

Please build it in Autotools too (search for for manual-tcp in test/Makefile.am and do the same things).
Comment 140 Simon McVittie 2015-11-16 19:55:34 UTC
Comment on attachment 119713 [details] [review]
Enable debug symbols on wine.

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

::: cmake/CMakeLists.txt
@@ +225,5 @@
>      SET(CMAKE_C_FLAGS "${CMAKE_C_FLAGS}  /w14018")
>      SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /w14018")
>  else()
> +    SET(CMAKE_C_FLAGS "${CMAKE_C_FLAGS}  -Wsign-compare -gdwarf-2")
> +    SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}  -Wsign-compare -gdwarf-2")

Is it sufficient to use plain "-g", which is the general-purpose flag for adding debug symbols? 

Shouldn't this be either part of configuring CMake for the DEBUG build-type (CMAKE_C_FLAGS_DEBUG, which I think uses -g anyway), or conditional on NOT DBUS_DISABLE_ASSERT?
Comment 141 Ralf Habacker 2015-11-16 19:56:43 UTC
(In reply to Simon McVittie from comment #139)

> Is there any reason why this can't be built on Unix too, like manual-tcp?

No, did not saw that there is also a backtrace generator on linux.

> Please build it in Autotools too (search for for manual-tcp in
> test/Makefile.am and do the same things).
sure
Comment 142 Ralf Habacker 2015-11-16 20:21:11 UTC
Created attachment 119715 [details] [review]
Add backtrace test app on windows.

- autotools support
- fixes
Comment 143 Ralf Habacker 2015-11-16 20:31:56 UTC
Created attachment 119716 [details] [review]
Add backtrace test app.

- commit message fix
Comment 144 Ralf Habacker 2015-11-16 23:18:36 UTC
Created attachment 119725 [details] [review]
Refactor backtrace generator

followup of attachment 119711 [details] [review]
Comment 145 Simon McVittie 2015-11-17 13:27:34 UTC
Comment on attachment 119716 [details] [review]
Add backtrace test app.

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

Looks good for master.
Comment 146 Ralf Habacker 2015-11-17 13:31:37 UTC
Comment on attachment 119710 [details] [review]
Print stack index in backtrace generator on Windows.

committed to master
Comment 147 Ralf Habacker 2015-11-17 13:31:51 UTC
Comment on attachment 119716 [details] [review]
Add backtrace test app.

committed to master
Comment 148 Ralf Habacker 2015-11-17 13:34:24 UTC
Comment on attachment 119725 [details] [review]
Refactor backtrace generator

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

I need to resent an updated patch. I tried to git am'd it on another machine, which fails.
Comment 149 Simon McVittie 2015-11-17 13:42:26 UTC
Comment on attachment 119725 [details] [review]
Refactor backtrace generator

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

::: cmake/dbus/CMakeLists.txt
@@ +275,4 @@
>      if(WINCE)
>          target_link_libraries(dbus-1 ws2)
>      else(WINCE)
> +        target_link_libraries(dbus-1 ws2_32 advapi32 netapi32 iphlpapi dbghelp)

100% less dlopen()-equivalent. I like it!

::: dbus/dbus-sysdeps-win.c
@@ +2474,5 @@
> +  while (StackWalk (dwImageType, GetCurrentProcess (),
> +                    hThread, &sf, &context, NULL, SymFunctionTableAccess,
> +                    SymGetModuleBase, NULL))
> +    {
> +      BYTE buffer[256];

I'd prefer this to be of length sizeof (IMAGEHLP_SYMBOL) + some arbitrary constant, to guarantee that it is strictly larger than IMAGEHLP_SYMBOL.

@@ +2488,5 @@
> +      IMAGEHLP_MODULE moduleInfo;
> +      DWORD dwDisplacement;
> +
> +#ifdef _IMAGEHLP64
> +      /* wine hack */

Please include a link to a relevant bug report or something, to explain what it is that you're hacking around? Or if there is none, either briefly describe it in the comment itself, or explain it on a comment on this bug and link to that.

I infer from the code that it's something like "on Wine64, we would sometimes get an infinite number of stack entries pointing to the same stack frame"?

@@ +2493,5 @@
> +      if (old_address == sf.AddrPC.Offset)
> +        break;
> +#endif
> +      pSymbol->SizeOfStruct = sizeof (IMAGEHLP_SYMBOL);
> +      pSymbol->MaxNameLength = sizeof (buffer) - sizeof (IMAGEHLP_SYMBOL) + 1;

Should this be sizeof (buffer) - sizeof (IMAGEHLP_SYMBOL) - 1, or equivalently sizeof (buffer) - (sizeof (IMAGEHLP_SYMBOL) + 1), to allow space for an IMAGEHLP_SYMBOL structure followed by MaxNameLength bytes followed by '\0'?

@@ +2508,2 @@
>  
> +      line.SizeOfStruct = sizeof (IMAGEHLP_LINE);

I'd slightly prefer sizeof (line) so that it very obviously matches, but this isn't important.

@@ +2514,5 @@
>  
> +      moduleInfo.SizeOfStruct = sizeof (moduleInfo);
> +      if (SymGetModuleInfo (GetCurrentProcess (), sf.AddrPC.Offset, &moduleInfo))
> +        {
> +          DPRINTF (" in %s\n", moduleInfo.ModuleName);

This is actually a mistake in the earlier patches on this bug, which I'm only noticing now: if SymGetModuleInfo() fails, you won't print a newline.

I'd prefer:

if (SymGetModuleInfo (...)
  {
    DPRINTF ("in %s", ...);
  }

old_address = sf.AddrPC.Offset;
DPRINTF ("\n");

so that we are obviously printing one newline per logical line.

@@ +2526,2 @@
>  {
> +    dump_backtrace_for_thread ((HANDLE)lpParameter);

Not very important, but libdbus style for casts is (HANDLE) lpParameter, with a space.
Comment 150 Ralf Habacker 2015-11-17 15:06:46 UTC
Created attachment 119737 [details] [review]
Add file and line support to backtrace generator on windows. (update1)

Fix eol output in case module could not be retrieved.
Comment 151 Ralf Habacker 2015-11-17 15:07:07 UTC
Created attachment 119738 [details] [review]
Refactor windows backtrace generator to link directly to dbghelp shared library.
Comment 152 Ralf Habacker 2015-11-17 15:43:23 UTC
Created attachment 119739 [details] [review]
Add x86_64 support to backtrace generator on windows.
Comment 153 Ralf Habacker 2015-11-17 15:43:42 UTC
Created attachment 119740 [details] [review]
Workaround with infinite loop while walking along the stack frames on wine64.
Comment 154 Ralf Habacker 2015-11-23 19:46:27 UTC
I splitted patch from attachment 119725 [details] [review] into tree parts. Any problem with them ?
Comment 155 Simon McVittie 2015-11-24 11:02:57 UTC
Comment on attachment 119737 [details] [review]
Add file and line support to backtrace generator on windows. (update1)

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

Looks OK for master. There's a bit of foo() instead of foo () but not really a problem.

::: dbus/dbus-sysdeps-win.c
@@ +2550,5 @@
> +  HANDLE hProcess,
> +  DWORD dwAddr,
> +  PDWORD pdwDisplacement,
> +  PIMAGEHLP_LINE Line
> +))GetProcAddress (hmodDbgHelp, FUNC(SymGetLineFromAddr));

The indentation is still weird here, but I think you delete it in a subsequent patch anyway, so, whatever.
Comment 156 Simon McVittie 2015-11-24 11:07:30 UTC
Comment on attachment 119739 [details] [review]
Add x86_64 support to backtrace generator on windows.

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

::: dbus/dbus-sysdeps-win.c
@@ +2490,5 @@
>  
>        pSymbol->SizeOfStruct = sizeof(SYMBOL_INFO);
>        pSymbol->MaxNameLen = MAX_SYM_NAME;
>  
> +      if (SymFromAddr (GetCurrentProcess (), sf.AddrPC.Offset, &displacement, pSymbol))

Does this break 32-bit by passing the wrong type for the 3rd argument to SymFromAddr()?

Or was it previously overrunning the 4-byte dwDisplacement into IMAGEHLP_LINE, but working anyway because x86 is little-endian and IMAGEHLP_LINE wasn't initialized yet anyway? ...

@@ +2495,3 @@
>          {
> +          if (displacement)
> +            DPRINTF ("%3d %s+0x%llx", i++, pSymbol->Name, displacement);

Should be %I64x instead of %llx, I think?
Comment 157 Simon McVittie 2015-11-24 11:07:57 UTC
Comment on attachment 119740 [details] [review]
Workaround with infinite loop while walking along the stack frames on wine64.

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

OK
Comment 158 Simon McVittie 2015-11-24 11:14:14 UTC
(In reply to Simon McVittie from comment #149)
> Review of attachment 119725 [details] [review]

> > +      BYTE buffer[256];
> 
> I'd prefer this to be of length sizeof (IMAGEHLP_SYMBOL) + some arbitrary
> constant, to guarantee that it is strictly larger than IMAGEHLP_SYMBOL.

Doesn't seem to have been fixed, although I might have missed it (I'm not 100% sure what the complete patch series is)

> > +      pSymbol->MaxNameLength = sizeof (buffer) - sizeof (IMAGEHLP_SYMBOL) + 1;
> 
> Should this be sizeof (buffer) - sizeof (IMAGEHLP_SYMBOL) - 1, or
> equivalently sizeof (buffer) - (sizeof (IMAGEHLP_SYMBOL) + 1), to allow
> space for an IMAGEHLP_SYMBOL structure followed by MaxNameLength bytes
> followed by '\0'?

Doesn't seem to have been fixed or explained

> if SymGetModuleInfo() fails, you won't print a newline

You did fix this though.
Comment 159 Ralf Habacker 2015-11-24 11:20:02 UTC
(In reply to Simon McVittie from comment #158)
> 
> Doesn't seem to have been fixed, although I might have missed it (I'm not
> 100% sure what the complete patch series is)
... 
> Doesn't seem to have been fixed or explained
...
> > if SymGetModuleInfo() fails, you won't print a newline
> 
> You did fix this though.
Sorry, I did not mention this in the follup patch. All these issues has been fixed with attachment 119739 [details] [review].
Comment 160 Ralf Habacker 2015-11-24 11:21:05 UTC
(In reply to Ralf Habacker from comment #159)
> (In reply to Simon McVittie from comment #158)
> > 
> > Doesn't seem to have been fixed, although I might have missed it (I'm not
> > 100% sure what the complete patch series is)
> ... 
> > Doesn't seem to have been fixed or explained
> ...
> > > if SymGetModuleInfo() fails, you won't print a newline
> > 
> > You did fix this though.
> Sorry, I did not mention this in the follup patch. All these issues has been
> fixed with attachment 119739 [details] [review] [review].
 attachment 119738 [details] [review]
Comment 161 Ralf Habacker 2015-11-24 11:42:54 UTC
(In reply to Ralf Habacker from comment #160)
> (In reply to Ralf Habacker from comment #159)
> > (In reply to Simon McVittie from comment #158)
> > > 
> > > Doesn't seem to have been fixed, although I might have missed it (I'm not
> > > 100% sure what the complete patch series is)
> > ... 
> > > Doesn't seem to have been fixed or explained
> > ...
> > > > if SymGetModuleInfo() fails, you won't print a newline
> > > 
> > > You did fix this though.
> > Sorry, I did not mention this in the follup patch. All these issues has been
> > fixed with attachment 119739 [details] [review] [review] [review].
>  attachment 119738 [details] [review] [review]
this patch includes also a replace of the deprecated function SymGetSymFromAddr by SymGetAddr https://msdn.microsoft.com/de-de/library/windows/desktop/ms681344%28v=vs.85%29.aspx
Comment 162 Ralf Habacker 2015-11-24 12:41:41 UTC
Comment on attachment 119737 [details] [review]
Add file and line support to backtrace generator on windows. (update1)

committed to master; remaining issues are fixed in next commit.
Comment 163 Ralf Habacker 2015-11-24 12:42:42 UTC
Comment on attachment 119738 [details] [review]
Refactor windows backtrace generator to link directly to dbghelp shared library.

committed to master with sligthly modified commit message (add term deprecated)
Comment 164 Ralf Habacker 2015-11-24 12:45:58 UTC
Comment on attachment 119739 [details] [review]
Add x86_64 support to backtrace generator on windows.

committed with merged in attachment 120075 [details] [review] reviewed by https://bugs.freedesktop.org/show_bug.cgi?id=93069#c55
Comment 165 Ralf Habacker 2015-11-24 12:49:42 UTC
Comment on attachment 119738 [details] [review]
Refactor windows backtrace generator to link directly to dbghelp shared library.

committed to master.
Comment 166 Ralf Habacker 2015-11-24 12:50:07 UTC
Comment on attachment 119740 [details] [review]
Workaround with infinite loop while walking along the stack frames on wine64.

committed to master
Comment 167 Ralf Habacker 2015-11-24 13:18:37 UTC
Created attachment 120084 [details] [review]
Use dwarf-2 debug symbol format on non msvc windows builds to support file and line numbers display on wine backtraces.
Comment 168 Ralf Habacker 2015-11-24 13:19:04 UTC
Comment on attachment 119713 [details] [review]
Enable debug symbols on wine.

superseeded
Comment 169 Simon McVittie 2015-11-24 17:32:31 UTC
Comment on attachment 120084 [details] [review]
Use dwarf-2 debug symbol format on non msvc windows builds to support file and line numbers display on wine backtraces.

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

::: cmake/CMakeLists.txt
@@ +216,5 @@
> +# set compiler debug flags
> +#
> +set(DEBUG_CFLAGS "-D_DEBUG")
> +if(WIN32 AND NOT MSVC)
> +    set(DEBUG_CFLAGS "${DEBUG_CFLAGS} -gdwarf-2")

If I understand correctly, DEBUG (and RELWITHDEBINFO) should already have something like -g from the normal CMake behaviour anyway. What debug symbols does -g produce, if not DWARF 2?
Comment 170 Ralf Habacker 2015-11-24 19:09:50 UTC
(In reply to Simon McVittie from comment #169)
> Comment on attachment 120084 [details] [review] [review]
> Use dwarf-2 debug symbol format on non msvc windows builds to support file
> and line numbers display on wine backtraces.
> 
> Review of attachment 120084 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: cmake/CMakeLists.txt
> @@ +216,5 @@
> > +# set compiler debug flags
> > +#
> > +set(DEBUG_CFLAGS "-D_DEBUG")
> > +if(WIN32 AND NOT MSVC)
> > +    set(DEBUG_CFLAGS "${DEBUG_CFLAGS} -gdwarf-2")
> 
> If I understand correctly, DEBUG (and RELWITHDEBINFO) should already have
> something like -g from the normal CMake behaviour anyway. What debug symbols
> does -g produce, if not DWARF 2?

Not sure yet, I did not found a direct reference in the mingw32 doc. 

From the wine source https://github.com/wine-mirror/wine/blob/master/dlls/dbghelp/pe_module.c#L466 and https://github.com/wine-mirror/wine/blob/master/dlls/dbghelp/pe_module.c#L502 it looks that stabs and dwarf are supported, while stabs is searched first (see https://github.com/wine-mirror/wine/blob/master/dlls/dbghelp/pe_module.c#L706)

Compiling manual-backtrace -with and without -gdwarf-2 and then tracing with  

 WINEDEBUG=trace+dbghelp bin/manual-backtrace.exe 2>&1 | less

returns in both cases: 

trace:dbghelp:module_new => PE 400000-41e000 L"S:\\ralf\\src\\dbus-1-cmake-cross-x86-build\\bin\\manual-backtrace.exe"
trace:dbghelp:pe_load_stabs failed to load the STABS debug info
trace:dbghelp:pe_load_dwarf successfully loaded the DWARF debug info

so I would say -g creates dwarf symbols, but with another, unsupported, variant.
Comment 171 Ralf Habacker 2015-11-24 19:18:23 UTC
(In reply to Ralf Habacker from comment #170)
> so I would say -g creates dwarf symbols, but with another, unsupported,
> variant.

For the record: I compiled with -gstabs and got

trace:dbghelp:pe_load_stabs successfully loaded the STABS debug info
trace:dbghelp:pe_load_dwarf successfully loaded the DWARF debug info

The resulting backtrace is identical to the one compiled with -gdwarf-2.
Comment 172 Ralf Habacker 2015-11-27 12:37:19 UTC
Created attachment 120171 [details] [review]
Add linking to dbhelp for autotools on Windows which is required for backtrace generator.

Bug:
Comment 173 Ralf Habacker 2015-11-27 16:47:54 UTC
Comment on attachment 120171 [details] [review]
Add linking to dbhelp for autotools on Windows which is required for backtrace generator.

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

::: configure.ac
@@ +1226,4 @@
>    if test x$dbus_wince = xyes ; then
>      NETWORK_libs="-lws2"
>    else
> +    NETWORK_libs="-lws2_32 -liphlpapi dbghelp"

Beside the fact, that it should be -ldbghelp, it does not work. Any hint where to add this dependency welcome.
Comment 174 Ralf Habacker 2015-11-28 00:10:30 UTC
Created attachment 120186 [details] [review]
Add linking to dbghelp for autotools on Windows which is required for backtrace generator.

Bug:
Comment 175 Ralf Habacker 2015-11-28 10:39:29 UTC
(In reply to Ralf Habacker from comment #173)
> Comment on attachment 120171 [details] [review] [review]

> it does not work. Any hint
> where to add this dependency welcome.

Got it - patching configure.ac requires to regenerate configure.
Comment 176 Simon McVittie 2015-12-01 22:12:47 UTC
Comment on attachment 120186 [details] [review]
Add linking to dbghelp for autotools on Windows which is required for backtrace generator.

I've applied this one to master for 1.11.0, since it was breaking the build. I'll look at your various other patches when I can.
Comment 177 Simon McVittie 2017-11-15 11:22:00 UTC
(In reply to Simon McVittie from comment #169)
> Use dwarf-2 debug symbol format on non msvc windows builds to support file
> and line numbers display on wine backtraces.

Is this still necessary/desired?

(In reply to Ralf Habacker from comment #170)
> I would say -g creates dwarf symbols, but with another, unsupported,
> variant.

Which version of gcc are you using, from which vendor? (mingw.org? mingw-w64?)

My gcc-7 manual says that on "most targets", -gdwarf is equivalent to -gdwarf-4. -g can be equivalent to anything (target-dependent) but I would hope that if it's DWARF, the default for -g and -gdwarf should be the same?

Please try with explicit -gdwarf-3 or -gdwarf-4? If you get the same results that you get with plain -g, then that would maybe imply that Wine supports DWARF 2 but not those newer versions?

I'd consider this patch with a more explanatory commit message, if it's necessary. However, it would perhaps be better to talk to the supplier of your compiler and see whether they'd consider changing the default for -g to a format that Wine can decode?
Comment 178 GitLab Migration User 2018-10-12 21:24:52 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/133.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.