Bug 421921 - Review Request: argyllcms - ICC compatible color management system
Review Request: argyllcms - ICC compatible color management system
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Daniel Berrange
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-Legal F9Target
  Show dependency treegraph
 
Reported: 2007-12-12 11:40 EST by Nicolas Mailhot
Modified: 2008-02-08 16:09 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-02-08 14:36:08 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
berrange: fedora‑review+
dennis: fedora‑cvs+


Attachments (Terms of Use)
Patch to fix buffer overflow (666 bytes, patch)
2007-12-12 13:34 EST, Daniel Berrange
no flags Details | Diff
Fix buffer overflow in ICC code (836 bytes, patch)
2007-12-12 14:03 EST, Daniel Berrange
no flags Details | Diff
Patch to allow building with FORTIFY_SOURCE (47.44 KB, patch)
2008-01-07 18:51 EST, Stefan Brüns
no flags Details | Diff
Patch to allow building with FORTIFY_SOURCE (44.54 KB, patch)
2008-02-07 18:07 EST, Stefan Brüns
no flags Details | Diff
Avoid an possible double free in scanin (405 bytes, patch)
2008-02-08 15:11 EST, Stefan Brüns
no flags Details | Diff


External Trackers
Tracker ID Priority Status Summary Last Updated
GNU Compiler Collection 34710 None None None Never

  None (edit)
Description Nicolas Mailhot 2007-12-12 11:40:41 EST
Spec URL: http://nim.fedorapeople.org/argyllcms/argyllcms.spec
SRPM URL: http://nim.fedorapeople.org/argyllcms/argyllcms-0.70-0.1.Beta7.fc9.src.rpm
Description:

The Argyll color management system supports accurate ICC profile creation for
scanners, CMYK printers, film recorders and calibration and profiling of
displays.

Spectral sample data is supported, allowing a selection of illuminants observer
types, and paper fluorescent whitener additive compensation. Profiles can also
incorporate source specific gamut mappings for perceptual and saturation
intents. Gamut mapping and profile linking uses the CIECAM02 appearance model,
a unique gamut mapping algorithm, and a wide selection of rendering intents. It
also includes code for the fastest portable 8 bit raster color conversion
engine available anywhere, as well as support for fast, fully accurate 16 bit
conversion. Device color gamuts can also be viewed and compared using a VRML
viewer.
Comment 1 Nicolas Mailhot 2007-12-12 11:42:26 EST
Blocking FE-LEGAL: need third-party opinion on whether the code and licensing
mash-up upstream uses is OK with us (and if not how should upstream fix it)
Comment 2 Daniel Berrange 2007-12-12 13:34:05 EST
Created attachment 285991 [details]
Patch to fix buffer overflow

The 'dispread' program aborts at startup due to a buffer overflow being
detected by GLibc. The attached patch fixes it to use  strlen() instead of
sizeof()
Comment 3 Daniel Berrange 2007-12-12 14:03:09 EST
Created attachment 286051 [details]
Fix buffer overflow in ICC code

The iccdump program abrts with a buffer overflow detected by glibc in the ICC
formatting routines. The attached patch fixes this bug in icc code
Comment 4 Nicolas Mailhot 2007-12-12 14:44:50 EST
Many thanks for the patches. As I wrote in my mail, there are probably several
such problems in upstream's codebase, since it has not been widely built
(Mandriva hid them by disabling FORTIFY_SOURCE), and I'm probably not the best
qualified to fix them.

The few utilities I tested didn't segfault on x86_64, are you building for
another arch or was I incredibly lucky?
Comment 5 Daniel Berrange 2007-12-12 14:55:42 EST
I'm on the i386 so the 'sizeof(buf)' flaw would have come out at '4' for me, but
'8' for you on x86_64. The '8' bytes was probably large enough to hide the bug.

The iccdump bug is variable depending on the data in the ICC file it is run against
Comment 6 Daniel Berrange 2007-12-12 14:57:55 EST
Review based on official guideline list:


MUST: rpmlint must be run on every package.

  $ rpmlint rpm/RPMS/i386/argyllcms-0.70-*
  $ rpmlint argyllcms-0.70-0.1.Beta7.fc9.src.rpm
  argyllcms.src: E: unknown-key GPG#3b29f20d

  - PASS   GPG error is harmless


MUST: The package must be named according to the Package Naming Guidelines.

 - PASS

MUST: The spec file name must match the base package %{name}

 - PASS

MUST: The package must meet the Packaging Guidelines.

 - PASS

MUST: The package must be licensed with a Fedora approved license and meet the
Licensing Guidelines.

      cgats/License.txt   - BSD-like
      doc/License.txt     - GPLv3+
      gamut/License.txt   - GPLv3+
      icc/License.txt     - BSD-like
      imdi/License.txt    - GPLv3+
      License.txt         - GPLv3+
      link/License.txt    - GPLv3+
      numlib/License.txt  - GPLv3+
      plot/License.txt    - GPLv3+
      profile/License.txt - GPLv3+
      render/License.txt  - GPLv3+
      rspl/License.txt    - GPLv3+
      scanin/License.txt  - GPLv3+
      spectro/License.txt - GPLv3+
      target/License.txt  - GPLv3+
      tweak/License.txt   - GPLv3+
      xicc/License.txt    - GPLv3+

   Links against libtiff  (libtiff license - BSD-like) and libusb (LGPLv2+)

 - VERIFY - Legal should verify text of the cgats/License.txt & icc/License.txt
 BSD-like files

MUST: The License field in the package spec file must match the actual license.

 - FAIL - Should be  'GPLv3+ and BSD-like' instead of merely 'GPLv3+'

MUST: If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package must be included in %doc.

 - FAIL - %doc should include the cgats/License.txt and icc/License.txt files too

MUST: The spec file must be written in American English.

 - PASS

MUST: The spec file for the package MUST be legible.

 - PASS

MUST: The sources used to build the package must match the upstream source

 - PASS md5sum = cfcfb4d4cd1663ae7b87a68e5f957cee

MUST: The package must successfully compile and build into binary rpms on at
least one supported architecture.

 - PASS built on i686 & x86_64

MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in ExcludeArch.

 - PASS N/A

MUST: All build dependencies must be listed in BuildRequires

 - PASS

MUST: The spec file MUST handle locales properly

 - PASS N/A

MUST: Every binary RPM package which stores shared library files (not just
symlinks) in any of the dynamic linker's default paths, must call ldconfig in
%post and %postun.

 - PASS N/A

MUST: If the package is designed to be relocatable, the packager must state this
fact in the request for review

 - PASS N/A

MUST: A package must own all directories that it creates.

 - PASS  - recommend changing to use an explicit  %dir though, eg

     %dir %{_datadir}/%{name}
     %{_datadir}/%{name}/*

MUST: A package must not contain any duplicate files in the %files listing.

 - PASS

MUST: Permissions on files must be set properly.

 - PASS

MUST: Each package must have a %clean section

 - PASS

MUST: Each package must consistently use macros

 - PASS

MUST: The package must contain code, or permissable content

 - PASS

MUST: Large documentation files should go in a -doc subpackage

 - PASS

MUST: If a package includes something as %doc, it must not affect the runtime of
the application

 - PASS

MUST: Header files must be in a -devel package.

 - PASS N/A

MUST: Static libraries must be in a -static package.

 - PASS N/A

MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'

 - PASS N/A

MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1),
then library files that end in .so (without suffix) must go in a -devel package.

 - PASS N/A

MUST: In the vast majority of cases, devel packages must require the base
package using a fully versioned dependency

 - PASS N/A

MUST: Packages must NOT contain any .la libtool archives

 - PASS N/A

MUST: Packages containing GUI applications must include a %{name}.desktop file

 - PASS N/A

MUST: Packages must not own files or directories already owned by other packages

 - PASS  For the icclink clash I recommend changing 'argyllcms-icclink' to instead
         be 'icclink-argyll' so it tab-completes under shell

MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}

 - PASS

MUST: All filenames in rpm packages must be valid UTF-8.

 - PASS

SHOULD: If the source package does not include license text(s) as a separate
file from upstream, the packager SHOULD query upstream to include it.

 - PASS N/A

SHOULD: The description and summary sections in the package spec file should
contain translations for supported Non-English languages, if available.

 - PASS N/A

SHOULD: The reviewer should test that the package builds in mock.

 - PASS

SHOULD: The package should compile and build into binary rpms on all supported
architectures.

 - PASS  on x86_64 and i686

SHOULD: The reviewer should test that the package functions as described.

 - FAIL Needs 2 aforementioned buffer overflow patches

SHOULD: If scriptlets are used, those scriptlets must be sane.

 - PASS  N/A

SHOULD: Usually, subpackages other than devel should require the base package
using a fully versioned dependency.

 - PASS N/A

SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this
is usually for development purposes, so should be placed in a -devel pkg.

 - PASS N/A

SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin,
/usr/bin, or /usr/sbin consider requiring the package which provides the file
instead of the file itself.

 - PASS N/A



Misc point - the %changelog section contains a number of unusual UTF-8 characters
at the start of each line. Recommend changing them to the standard '-'  prefix for
changelog entries

So in summary

 - Legal needs to review  cgats/License.txt and icc/License.txt
 - License: tag needs fixing
 - Text of the non-GPLv3 License.txt files should be in %doc
 - Fix changelog
 - Rename  arygllcms-icclink to icclink-argyllcms
 - Apply 2 buffer overflow patches
Comment 7 Nicolas Mailhot 2007-12-12 15:47:44 EST
- VERIFY - Legal should verify text of the cgats/License.txt & icc/License.txt
 BSD-like files

Those bits are not linked against anything else, and not exported by Argyll
since it's all statically linked. So the only compatibility problem we may have
would be between the GPLv3 and Argyll, and since their common author judges it's
ok, exposing those licenses to users seems rather useless to me.

But for legal to judge.

The bits I'm actually worried against are the GPLv2 files in spectro linked
against all this GPLv3 codebase, since they are *not* written by the same
author. It's rather unclear to me if they're GPLv2+ and Graeme W. Gill is
pedantic in exposing their license, or not.

MUST: The License field in the package spec file must match the actual license.

- FAIL - Should be  'GPLv3+ and BSD-like' instead of merely 'GPLv3+'

If you want but BSD-like is not a valid license

- FAIL - %doc should include the cgats/License.txt and icc/License.txt files too

OK

- PASS  - recommend changing to use an explicit  %dir

My preference usually too, must have been exposed to too many line counters to
not have done it this way from the beginning

- PASS  For the icclink clash I recommend changing 'argyllcms-icclink' to
instead be 'icclink-argyll' so it tab-completes under shell

Good idea

- FAIL Needs 2 aforementioned buffer overflow patches

More patches like that welcome :p

- the %changelog section contains a number of unusual UTF-8 characters

SPECS are UTF-8, and changelog in particular can contain UTF-8 names (which is
the case there), so asciifying presents no interest

Thanks a lot for this timely review! New files at the same place as previously
Comment 8 Daniel Berrange 2007-12-12 16:26:15 EST
The only GPLv2 file i see is  spectro/filmread.c and this merely says

  * This material is licenced under the GNU GENERAL PUBLIC LICENSE :-
  * see the License2.txt file for licencing details.

According to the licensing guidelines

  http://fedoraproject.org/wiki/Licensing

"A GPL or LGPL licensed package that lacks any statement of what version that 
 it's licensed under in the source code/program output/accompanying docs is
 technically licensed under *any* version of the GPL or LGPL, not just the 
 version in whatever COPYING file they include."

So, IMHO,  spectro/filmread.c is under *any* version of the GPL and thus
trivially compatible with the GPLv3 used by the rest of the code. Even if it can
be argued that it is GPLv2+, then that is still compatible with GPLv3+

As for the cgats / icc directory stuff, I'd recommend mailing the
fedora-legal-list@redhat.com with text of the license to confirm whether it can
be classed as 'BSD'. 

Yes, BSD-Like is not valid - I should have just said  "GPLv3+ and BSD" for the
License: tag.
Comment 9 Nicolas Mailhot 2007-12-12 16:43:09 EST
(In reply to comment #8)

> I'd recommend mailing the fedora-legal-list@redhat.com 

There should be no need I blocked FE-LEGAL already

So, you agree the there are only legal aspects left to do in the review?
Comment 10 Daniel Berrange 2007-12-12 17:07:47 EST
Yes, once the license stuff is clarified I've no problem marking this review
approved.
Comment 11 Nicolas Mailhot 2007-12-13 16:52:13 EST
Updated version with policykit-only permission logic after feedback from
Frédéric Crozat & David Zeuthen

http://nim.fedorapeople.org/argyllcms/argyllcms-0.70-0.4.Beta7.fc9.src.rpm
http://nim.fedorapeople.org/argyllcms/argyllcms.spec
Comment 12 Nicolas Mailhot 2007-12-14 09:40:32 EST
Upstream confirmed the problem with the GPLv2 files so I've removed them,
leaving only the BSD-like licenses to be approved by legal

http://nim.fedorapeople.org/argyllcms/argyllcms-0.70-0.5.Beta7.fc9.src.rpm
http://nim.fedorapeople.org/argyllcms/argyllcms.spec

Comment 13 Tom "spot" Callaway 2007-12-20 13:50:25 EST
The cgats/License.txt & icc/License.txt (they're the same license) is Free, but
GPLv2 and GPLv3 incompatible.

This is because of clause 2 and 4. Since everything else is GPLv3+, at a
minimum, clause 4 would need to be removed/altered.

> 4) These conditions apply to any software derived from or based
>    on the Software, not just to the unmodified library.

The FSF says that it is very easy to read this as a copyleft requirement.  It's
a little weird because it would basically be a copyleft for its own sake -- all
it would do is make sure the license of the project never changed, rather than
accomplishing some end like making sure the source is available.  But it's
certainly within the realm of possibility. 

This probably isn't the author's intent, they probably just want to make sure
that the source is always freely available without additional restrictions. If
clause 4 is removed, or replaced with something more along these lines, it would
probably be GPLv3 compat. Alternately, Mr. Gill might choose to simply relicense
those bits as GPLv3, and remove all compatibility concerns, as the GPLv3
enforces that the source will always remain freely available without additional
restrictions.

As is, this is a showstopper, since icc and cgets are tightly tied into the rest
of the argyllcms code. Sorry.
Comment 14 Stefan Brüns 2008-01-07 18:51:53 EST
Created attachment 291027 [details]
Patch to allow building with FORTIFY_SOURCE
Comment 15 Stefan Brüns 2008-01-07 19:05:04 EST
(In reply to comment #4)
> Many thanks for the patches. As I wrote in my mail, there are probably 
several
> such problems in upstream's codebase, since it has not been widely built
> (Mandriva hid them by disabling FORTIFY_SOURCE), and I'm probably not the 
best
> qualified to fix them.

I have created a patch to allow using FORTIFY_SOURCE. According to gcc bug 
34710, printf may be a macro, so usage of printf as a function name is risky at 
least. I have changed the fn name to icc_fprintf(), this fixes the problem.
Comment 16 Nicolas Mailhot 2008-02-07 17:29:47 EST
Upstream has been nice, merged patches and changed its licensing, so here is a
new spec version that hopefully will pass both legal and technical review:

Spec URL: http://nim.fedorapeople.org/argyllcms/argyllcms.spec
SRPM URL: http://nim.fedorapeople.org/argyllcms/argyllcms-0.70-0.1.Beta8.fc9.src.rpm

I've added a patch based on the suggestion in comment #15 to the mix. Please
yell if it's a bad idea. gcc developer comments tend to eat my brane.

Build-tested on fedora-devel x86-64 only
Comment 17 Daniel Berrange 2008-02-07 17:45:17 EST
The patch from comment #14 is fine - I've done similar things in the past too.
It is quite a large patch so probably worth sending upstream too, since
FORTIFY_SOURCE is becoming ever more widely used.

Confirmed that the cgats/ and icc/ files are now plain MIT licensed, so the
result is simply "GPLv3 and MIT" which is fine for Fedora.

The spec still passes review on tech grounds too, so marking this approved.
Comment 18 Stefan Brüns 2008-02-07 18:07:18 EST
Created attachment 294285 [details]
Patch to allow building with FORTIFY_SOURCE

Maybe you should use this version, it is a little bit shorter, because it only
uses braces to avoid macro expansion instead of changing the function name
Comment 19 Nicolas Mailhot 2008-02-08 01:56:07 EST
Ok, I'll look at Stefan's new patch once the package is imported

New Package CVS Request
=======================
Package Name: argyllcms
Short Description: ICC compatible color management system
Owners: nim
Branches: F-7 F-8
InitialCC: 
Cvsextras Commits: yes
Comment 20 Nicolas Mailhot 2008-02-08 02:26:29 EST
Argh please remove F-7, I don't want to deal with pre-PolicyKit device
permission setup

New Package CVS Request
=======================
Package Name: argyllcms
Short Description: ICC compatible color management system
Owners: nim
Branches: F-8
InitialCC: 
Cvsextras Commits: yes
Comment 21 Dennis Gilmore 2008-02-08 13:30:57 EST
CVS Done
Comment 22 Fedora Update System 2008-02-08 14:31:00 EST
argyllcms-0.70-0.8.Beta8.fc8 has been submitted as an update for Fedora 8
Comment 23 Nicolas Mailhot 2008-02-08 14:35:53 EST
Thanks Dennis (and to every contributor to this review)

I've now pushed argyll for F8 testing and F9.

Since this is a very new package, with little testing, I don't intend to push it
to F8 stable before someone confirms it works.

If people new to color management (useful for pictures, video, graphic work,
etc) are looking for an OEM instrument without paying the proprietary software
usually bundled with it, I suggest taking a look at
http://www.freelists.org/archives/argyllcms/01-2008/msg00369.html
Comment 24 Stefan Brüns 2008-02-08 15:11:07 EST
Created attachment 294410 [details]
Avoid an possible double free in scanin

This double free may happen if the last row of color patches has been cropped
due to printer margins
Comment 25 Nicolas Mailhot 2008-02-08 16:09:44 EST
Thanks Stefan, your auditing is appreciated.

You can continue posting fixes there or on upstream's ML
(http://www.freelists.org/archives/argyllcms/), I'll push them both ways

Note You need to log in before you can comment on or make changes to this bug.