Bug 57272 - dbus-daemon on Windows is effectively GPL-2 (only) due to LGPL code in dbus-sysdeps-util-win.c
Summary: dbus-daemon on Windows is effectively GPL-2 (only) due to LGPL code in dbus-s...
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Havoc Pennington
QA Contact:
URL:
Whiteboard: partially review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2012-11-19 12:12 UTC by Gervase Markham
Modified: 2014-09-08 14:41 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
COPYING: clarify licensing status of dbus-daemon on Windows (2.21 KB, patch)
2014-09-04 16:30 UTC, Simon McVittie
Details | Splinter Review
Move LGPL parts of dbus-sysdeps-util-win.c into a separate file (12.66 KB, patch)
2014-09-04 17:11 UTC, Simon McVittie
Details | Splinter Review
Add directory test application (4.39 KB, patch)
2014-09-05 15:15 UTC, Ralf Habacker
Details | Splinter Review
Port dbus_directory_function to use win32 native api function (9.95 KB, patch)
2014-09-05 15:16 UTC, Ralf Habacker
Details | Splinter Review
Improve debug message to be able to see empty files (819 bytes, patch)
2014-09-05 16:07 UTC, Ralf Habacker
Details | Splinter Review
Add directory test application (update 1) (4.38 KB, patch)
2014-09-05 16:07 UTC, Ralf Habacker
Details | Splinter Review
Add dbus string convenience function (2.07 KB, patch)
2014-09-05 16:08 UTC, Ralf Habacker
Details | Splinter Review
Port dbus_directory_function to use win32 native api function (update 1) (10.58 KB, patch)
2014-09-05 16:09 UTC, Ralf Habacker
Details | Splinter Review
Add directory test application (update 2) (3.45 KB, patch)
2014-09-08 13:31 UTC, Ralf Habacker
Details | Splinter Review
Add test application 'manual-dir-iter' to autotools build system (1.02 KB, patch)
2014-09-08 14:25 UTC, Ralf Habacker
Details | Splinter Review

Description Gervase Markham 2012-11-19 12:12:20 UTC
The file dbus-sysdeps-util-win.c has a GPL/AFL header at the top, but a LGPL header half way down it, at around line 423. 

This header seems to have been there since the initial creation of that file in 2007:
http://cgit.freedesktop.org/dbus/dbus/log/dbus/dbus-sysdeps-util-win.c

If the code which this block refers to was originally LGPLed and has been imported into dbus from elsewhere, that might be a problem, as you would need the copyright holder's permission to dual-license it with the AFL.

If you obtained that permission when you imported it, then I assume that the dbus team has (quite reasonably) taken advantage of the LGPL's ability to relicense under the GPL for whatever code that license used to cover. If that's so, then could the LGPL license block be removed as unnecessary?

The reason I raise the issue is that I'm doing license compliance work with an automated scanner, and it fires as a positive for "presence of LGPLed code" in dbus.

Many thanks,

Gerv
Comment 1 Simon McVittie 2012-11-19 13:09:10 UTC
So there are two possibilities here:

* explicit permission was granted by the copyright holder to relicense
  under the GPL|AFL dual-license

* that permission was not granted

If permission was granted, then the LGPL header no longer matters and can be deleted.

If permission was not granted, then that file is not AFL-compatible, and will effectively have to be treated as licensed under the GPL only.

*-util-*.c are only used in dbus-daemon and associated tools (dbus-monitor etc.), not in the libdbus-1 shared library, so if we have to treat that file as GPL-only, I don't think that should be harmful for projects like LibreOffice/OpenOffice: if necessary, we can say "on Windows, libdbus-1 is GPL|AFL but dbus-daemon and the tools are GPL-only".

Ralf, you committed that file. Which parts of it are covered by the LGPL header? (Is it just the !defined(HAVE_DIRENT_H) block?) Do you have permission from Werner Almesberger to distribute this code under D-Bus' GPL|AFL dual-license?
Comment 2 Simon McVittie 2012-11-19 13:12:42 UTC
(In reply to comment #1)
> *-util-*.c are only used in dbus-daemon and associated tools (dbus-monitor
> etc.), not in the libdbus-1 shared library

This was always meant to be true, and has always been true under the autotools build system (I think), but when built with CMake it was only made true in commit 301fa55 (between 1.5.8 and 1.5.10).
Comment 3 Simon McVittie 2012-11-19 13:14:28 UTC
(In reply to comment #1)
> Ralf, you committed [dbus-sysdeps-util-win.c].
> Which parts of it are covered by the LGPL
> header? (Is it just the !defined(HAVE_DIRENT_H) block?) Do you have
> permission from Werner Almesberger to distribute this code under D-Bus'
> GPL|AFL dual-license?

Er, it'd help if I cc'd Ralf before asking him questions...

(See previous comments for context.)
Comment 4 Ralf Habacker 2012-11-19 19:33:40 UTC
(In reply to comment #3)
> (In reply to comment #1)
> > Ralf, you committed [dbus-sysdeps-util-win.c].
> > Which parts of it are covered by the LGPL
> > header? (Is it just the !defined(HAVE_DIRENT_H) block?) 

This belongs to the header which came from 
The header comes from http://www.trinitydesktop.org/docs/trinity/current/kdelibs/html/dirent_8h_source.html

> > Do you have
> > permission from Werner Almesberger to distribute this code under D-Bus'
> > GPL|AFL dual-license?

not requested, this has been added while porting dbus to windows under the hood of the KDE-Windows project. 

Please note that the related code is available under a bunch of licenses - I searched google for "Implement dirent-style opendir/readdir/closedir on Window 95/NT" and found

Apache license
http://www.msg.ucsf.edu/local/ganglia/ganglia-monitor-core-2.5.3/lib/readdir.c
http://www.msg.ucsf.edu/local/ganglia/ganglia-monitor-core-2.5.3/lib/readdir.h

PHP License https://github.com/moriyoshi/php-src/blob/master/LICENSE
https://github.com/moriyoshi/php-src/blob/master/win32/readdir.h
https://github.com/moriyoshi/php-src/blob/master/win32/readdir.c

gpl  (slightly modified implementation)
http://devel.squid-cache.org/changesets/squid/patches/10510.patch
Comment 5 Simon McVittie 2014-09-04 16:30:45 UTC
Created attachment 105756 [details] [review]
COPYING: clarify licensing status of dbus-daemon on Windows
Comment 6 Gervase Markham 2014-09-04 16:34:46 UTC
Might it be possible to fix the licensing problem instead of documenting it?

Gerv
Comment 7 Ralf Habacker 2014-09-04 17:08:34 UTC
(In reply to comment #6)
> Might it be possible to fix the licensing problem instead of documenting it?
> 
I see three ways to solve this issue

1. use one of the mentioned licenses if appliable otherwise
2. replace struct dirent and DIR by dbus own typedefs and use them in _dbus_opendir(), _dbus_readdir(), _dbus_closedir() or 
3. migrate directory and directory entry handling to a DBusDir and DBusDirEntry typedef and functions _dbus_dir_open(), _dbus_dir_read(), _dbus_dir_close() optional in a separate dbus_dir.c|h file
Comment 8 Simon McVittie 2014-09-04 17:11:01 UTC
(In reply to comment #6)
> Might it be possible to fix the licensing problem instead of documenting it?

I don't consider it to be a problem that dbus-daemon.exe, when compiled for Windows, is effectively GPL-only. I do consider it to be a problem that it isn't clear that that is the case.

If you happen to have a permissively-licensed implementation of readdir()-style functionality that works on Windows, I'd be happy to review a patch.

Similarly, if you are unhappy with this functionality not being permissively-licensed, and you want to contact Werner Almesberger and ask for this to be relicensed under MIT/X11 or something, please go ahead.

If you're bundling dbus in LibreOffice but not distributing a Windows version of dbus-daemon, I suggest deleting that file, or replacing its contents with "/* deleted because it is GPL-only and unused */", in your copy.
Comment 9 Simon McVittie 2014-09-04 17:11:32 UTC
Created attachment 105761 [details] [review]
Move LGPL parts of dbus-sysdeps-util-win.c into a separate  file
Comment 10 Simon McVittie 2014-09-04 17:14:35 UTC
(In reply to comment #7)
> I see three ways to solve this issue
> 
> 1. use one of the mentioned licenses if appliable otherwise

This requires permission from the copyright holder. Do you know how to contact the copyright holder?

> 2. replace struct dirent and DIR by dbus own typedefs and use them in
> _dbus_opendir(), _dbus_readdir(), _dbus_closedir() or 
> 3. migrate directory and directory entry handling to a DBusDir and
> DBusDirEntry typedef and functions _dbus_dir_open(), _dbus_dir_read(),
> _dbus_dir_close() optional in a separate dbus_dir.c|h file

D-Bus *already* has its own cross-platform directory-listing API, namely _dbus_directory_open(), _dbus_directory_get_next_file(), _dbus_directory_close().

Reimplementing that API using Windows primitives without copying from a LGPL source, instead of going via a weird emulation of Unix APIs, would probably be a code-quality improvement, as well as allowing this LGPL code to be dropped. Patches welcome; I'm not going to write them, because I don't develop on Windows.
Comment 11 Simon McVittie 2014-09-04 17:17:58 UTC
To be clear, D-Bus already defines a cross-platform directory-listing API; that is not the problem. 

The problem is that instead of implementing that directory-listing API using FindFirstFileA() or whatever, the author of this part of the D-Bus Windows support code found a LGPL implementation of the standard Unix opendir(), readdir(), closedir() functions in terms of Windows primitives, pasted it in, then implemented D-Bus' directory API in terms of those functions.
Comment 12 Gervase Markham 2014-09-05 14:20:41 UTC
Hi Simon,

Is it possible to tell who the copyright holder is? Is it the Werner Almesberger you mentioned, or might there be more to it than that?

If this file is only used on Windows, then we could solve our issue (it shows up on license-checking scans) by just deleting it from our copy, but if we import a new version, it might come back, and it just seems easier to regularise the licensing of dbus. I'm happy to contact the necessary people, if someone else can do the source code management archaeology to tell me who I need to contact.

Gerv
Comment 13 Simon McVittie 2014-09-05 14:43:52 UTC
(In reply to comment #12)
> Is it possible to tell who the copyright holder is? Is it the Werner
> Almesberger you mentioned, or might there be more to it than that?

I don't know any more than you do; I wasn't a D-Bus maintainer back when this was added. Hopefully Ralf knows - he committed it, back in the days of svn.

The relevant commit is

http://cgit.freedesktop.org/dbus/dbus/commit/?h=70bfc74e54ac8a9a93885710cd8350d1a58b3406

git blame says Marcus Brinkmann and Tor Lillqvist have also modified that chunk of code.

It is possible that some or all of these people agreed to MIT/X11 licensing for their contributions when Ryan Lortie was trying to get dbus relicensed to MIT/X11 (which fell through because the copyright on some old and rather fundamental bits was owned by CodeFactory AB, which went out of business, and nobody is sure who has ended up owning their assets, so nobody knows who can give permission to relicense their stuff).
Comment 14 Ralf Habacker 2014-09-05 15:15:53 UTC
Created attachment 105806 [details] [review]
Add directory test application
Comment 15 Ralf Habacker 2014-09-05 15:16:46 UTC
Created attachment 105807 [details] [review]
Port dbus_directory_function to use win32 native api function
Comment 16 Ralf Habacker 2014-09-05 15:19:17 UTC
As mentioned in comment 4 does the copyright issue came from the including of the header containing the DIR structure include, which is solved by the "port" patch
Comment 17 Simon McVittie 2014-09-05 15:27:55 UTC
Comment on attachment 105807 [details] [review]
Port dbus_directory_function to use win32 native api function

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

Thanks, I think this is the best approach to resolving this.

Looks good to me, except:

::: dbus/dbus-sysdeps-util-win.c
@@ +453,5 @@
>  
>    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
>  
>    filename_c = _dbus_string_get_const_data (filename);
> +  filespec = dbus_malloc (strlen(filename_c) + 2 + 1);

Code below this point will crash if dbus_malloc() returns NULL here. _DBUS_SET_OOM (error) and return instead.

@@ +454,5 @@
>    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
>  
>    filename_c = _dbus_string_get_const_data (filename);
> +  filespec = dbus_malloc (strlen(filename_c) + 2 + 1);
> +  strcpy (filespec, filename_c);

DBusString stops you needing to think about whether your strcat() has off-by-one errors.

This particular code looks OK at first glance; but I shouldn't have to even think about it, because if it used DBusString, it would be "obviously correct", which is the best sort of correct.
Comment 18 Ralf Habacker 2014-09-05 16:07:13 UTC
Created attachment 105809 [details] [review]
Improve debug message to be able to see empty files

convenience patch
Comment 19 Ralf Habacker 2014-09-05 16:07:50 UTC
Created attachment 105810 [details] [review]
Add directory test application (update 1)

unused label cleanup
Comment 20 Ralf Habacker 2014-09-05 16:08:17 UTC
Created attachment 105811 [details] [review]
Add dbus string convenience function
Comment 21 Ralf Habacker 2014-09-05 16:09:13 UTC
Created attachment 105812 [details] [review]
Port dbus_directory_function to use win32 native api function (update 1)

- use DBusString
- fix additional filename pattern case
Comment 22 Ralf Habacker 2014-09-06 17:53:49 UTC
(In reply to comment #17)
> Comment on attachment 105807 [details] [review] [review]
> Port dbus_directory_function to use win32 native api function
> 
> Review of attachment 105807 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Thanks, I think this is the best approach to resolving this.
> 
> Looks good to me, except:

should be solved now :-)
Comment 23 Ralf Habacker 2014-09-07 08:22:28 UTC
Can I assume that the updated patches are okay for commit to git master ?
Comment 24 Simon McVittie 2014-09-08 10:34:10 UTC
(In reply to comment #23)
> Can I assume that the updated patches are okay for commit to git master ?

I would still like to review them properly, which I will do soon.
Comment 25 Simon McVittie 2014-09-08 11:41:52 UTC
Comment on attachment 105809 [details] [review]
Improve debug message to be able to see empty files

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

sure
Comment 26 Simon McVittie 2014-09-08 11:52:47 UTC
Comment on attachment 105810 [details] [review]
Add directory test application (update 1)

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

::: dbus/dbus-errors.c
@@ +334,5 @@
>    return error->name != NULL;
>  }
>  
> +const char *
> +dbus_error_get_message (const DBusError *error)

Why is this needed? error->message is public API.

I don't think there's any point in having an accessor function here.

::: test/test-dir.c
@@ +1,1 @@
> +#include <config.h>

test-dir.c cannot be run as an automated test (it needs to be run with a command-line option, and its output needs human interpretation) so I'd prefer it to be named matching manual-* - perhaps manual-dir-iter.c or something.

@@ +3,5 @@
> +
> +#include "dbus/dbus-sysdeps.h"
> +
> +static void
> +die (const char *message)

(Not a merge blocker)

If you wrote it like this (requires dbus/dbus-macros.h to be included first), the compiler would understand that it cannot return:

static void die (const char *message) _DBUS_GNUC_NORETURN;

static void
die (const char *message)
{
  ...

@@ +30,5 @@
> +
> +  dbus_error_init (&tmp_error);
> +
> +  if (!_dbus_string_init (&filename))
> +      die ("memory allocation error");

(Not a merge blocker)

This is fine, but I often find that defining

    static void oom (void) _DBUS_GNUC_NORETURN;

adds clarity.

@@ +35,5 @@
> +
> +  if (!_dbus_string_init (&dirname))
> +      die ("memory allocation error");
> +
> +  _dbus_string_append (&dirname, argv[1]);

if (!_dbus_string_append (...))
  oom ();

(or die("..."))

@@ +39,5 @@
> +  _dbus_string_append (&dirname, argv[1]);
> +  dir = _dbus_directory_open (&dirname, &tmp_error);
> +
> +  if (dir == NULL)
> +      die ("could not open directory");

I'd personally do

if (dir == NULL)
  {
    fprintf (stderr, "could not open directory: %s: %s\n", tmp_error.name, tmp_error.message);
    exit (1);
  }

@@ +63,5 @@
> +      _dbus_string_free (&full_path);
> +    }
> +
> +  if (dbus_error_is_set (&tmp_error))
> +      die (dbus_error_get_message (&tmp_error));

You can just use die (tmp_error.message) here, but I'd personally do

if (dir == NULL)
  {
    fprintf (stderr, "could not open directory: %s: %s\n", tmp_error.name, tmp_error.message);
    exit (1);
  }

@@ +68,5 @@
> +
> +  _dbus_string_free (&filename);
> +
> +  if (dir)
> +    _dbus_directory_close (dir);

You don't _dbus_string_free (dirname), although it doesn't really matter, because you're about to exit anyway.
Comment 27 Simon McVittie 2014-09-08 11:53:25 UTC
Comment on attachment 105811 [details] [review]
Add dbus string convenience function

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

sure
Comment 28 Simon McVittie 2014-09-08 11:58:04 UTC
Comment on attachment 105812 [details] [review]
Port dbus_directory_function to use win32 native api function (update 1)

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

Looks OK, some minor adjustments that aren't merge blockers

::: dbus/dbus-sysdeps-util-win.c
@@ +455,3 @@
>      {
> +      dbus_set_error (error, DBUS_ERROR_NO_MEMORY,
> +                      "Could not allocate memory for directory filename copy");

(Not a merge blocker)

You could just use _DBUS_SET_OOM (error) or whatever the macro is called

@@ +462,5 @@
> +    {
> +      if (!_dbus_string_append (&filespec, "*"))
> +        {
> +          dbus_set_error (error, DBUS_ERROR_NO_MEMORY,
> +                          "Could not append filename wildcard");

same here

@@ +471,5 @@
> +    {
> +      if (!_dbus_string_append (&filespec, "\\*"))
> +        {
> +          dbus_set_error (error, DBUS_ERROR_NO_MEMORY,
> +                          "Could not append filename wildcard 2");

same here
Comment 29 Simon McVittie 2014-09-08 11:59:18 UTC
The actual functional change looks good, thanks!

I would prefer some alterations to the manual test (your patch 2/4) before we merge that.
Comment 30 Simon McVittie 2014-09-08 11:59:47 UTC
Comment on attachment 105756 [details] [review]
COPYING: clarify licensing status of dbus-daemon on Windows

obsoleted by Ralf's solution
Comment 31 Simon McVittie 2014-09-08 11:59:54 UTC
Comment on attachment 105761 [details] [review]
Move LGPL parts of dbus-sysdeps-util-win.c into a separate  file

obsoleted by Ralf's solution
Comment 32 Ralf Habacker 2014-09-08 12:02:22 UTC
Comment on attachment 105809 [details] [review]
Improve debug message to be able to see empty files

committed to master
Comment 33 Ralf Habacker 2014-09-08 12:02:34 UTC
Comment on attachment 105811 [details] [review]
Add dbus string convenience function

committed to master
Comment 34 Gervase Markham 2014-09-08 12:39:58 UTC
Thanks so much for solving this :-)

Gerv
Comment 35 Ralf Habacker 2014-09-08 13:31:05 UTC
Created attachment 105893 [details] [review]
Add directory test application (update 2)
Comment 36 Ralf Habacker 2014-09-08 13:36:50 UTC
(In reply to comment #28)
> Comment on attachment 105812 [details] [review] [review]
> Port dbus_directory_function to use win32 native api function (update 1)
> 
> Review of attachment 105812 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks OK, some minor adjustments that aren't merge blockers
> 
> ::: dbus/dbus-sysdeps-util-win.c
> @@ +455,3 @@
> >      {
> > +      dbus_set_error (error, DBUS_ERROR_NO_MEMORY,
> > +                      "Could not allocate memory for directory filename copy");
> 
> (Not a merge blocker)
> 
> You could just use _DBUS_SET_OOM (error) or whatever the macro is called
> 
> @@ +462,5 @@
> > +    {
> > +      if (!_dbus_string_append (&filespec, "*"))
> > +        {
> > +          dbus_set_error (error, DBUS_ERROR_NO_MEMORY,
> > +                          "Could not append filename wildcard");
> 
> same here
> 
> @@ +471,5 @@
> > +    {
> > +      if (!_dbus_string_append (&filespec, "\\*"))
> > +        {
> > +          dbus_set_error (error, DBUS_ERROR_NO_MEMORY,
> > +                          "Could not append filename wildcard 2");
> 
> same here

The drawback with using DBUS_SET_OOM is that the detailed error condition will get lost.
Comment 37 Simon McVittie 2014-09-08 14:01:03 UTC
Comment on attachment 105893 [details] [review]
Add directory test application (update 2)

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

ship it!
Comment 38 Simon McVittie 2014-09-08 14:02:09 UTC
(In reply to comment #36)
> The drawback with using DBUS_SET_OOM is that the detailed error condition
> will get lost.

I don't think that really matters for OOM (if malloc() returns NULL, everything is ruined already, and dbus_set_error() will probably also hit an OOM condition and use a generic error message) but I do see your point.

OK to merge, with or without switching to _DBUS_SET_OOM().
Comment 39 Simon McVittie 2014-09-08 14:04:47 UTC
(In reply to comment #37)
> Add directory test application (update 2)

I'd like this added to test/Makefile.am; you could do that (it's like manual-authz, except that it needs to be linked to libdbus-internal like test-syslog is), or I can do that if you'll review it.
Comment 40 Ralf Habacker 2014-09-08 14:25:54 UTC
Created attachment 105895 [details] [review]
Add test application 'manual-dir-iter' to autotools build system
Comment 41 Ralf Habacker 2014-09-08 14:30:09 UTC
(In reply to comment #40)
> Created attachment 105895 [details] [review] [review]
> Add test application 'manual-dir-iter' to autotools build system

Because I did not commit the base 'manual-dir-iter' patch, I can merge this into it, if you like.
Comment 42 Simon McVittie 2014-09-08 14:31:09 UTC
(In reply to comment #41)
> Because I did not commit the base 'manual-dir-iter' patch, I can merge this
> into it, if you like.

Yes, please squash this in. It looks correct.
Comment 43 Ralf Habacker 2014-09-08 14:35:06 UTC
Comment on attachment 105893 [details] [review]
Add directory test application (update 2)

committed to master
Comment 44 Ralf Habacker 2014-09-08 14:35:18 UTC
Comment on attachment 105895 [details] [review]
Add test application 'manual-dir-iter' to autotools build system

committed to master
Comment 45 Ralf Habacker 2014-09-08 14:40:21 UTC
Comment on attachment 105812 [details] [review]
Port dbus_directory_function to use win32 native api function (update 1)

committed to master
Comment 46 Simon McVittie 2014-09-08 14:41:19 UTC
Fixed in git for 1.9.0, then


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.