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.
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)
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()
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
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?
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
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
- 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
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 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.
(In reply to comment #8) > I'd recommend mailing the fedora-legal-list 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?
Yes, once the license stuff is clarified I've no problem marking this review approved.
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
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
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.
Created attachment 291027 [details] Patch to allow building with FORTIFY_SOURCE
(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.
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
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.
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
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
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
CVS Done
argyllcms-0.70-0.8.Beta8.fc8 has been submitted as an update for Fedora 8
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
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
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