Bug 225670 - Merge Review: cups
Merge Review: cups
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Adam Tkac
Fedora Package Reviews List
:
Depends On: 643455
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 12:54 EST by Nobody's working on this, feel free to take it
Modified: 2013-04-30 19:35 EDT (History)
4 users (show)

See Also:
Fixed In Version: cups-1.4.6-13.fc16
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-03-08 06:33:44 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
atkac: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 12:54:12 EST
Fedora Merge Review: cups

http://cvs.fedora.redhat.com/viewcvs/devel/cups/
Initial Owner: twaugh@redhat.com
Comment 1 Adam Tkac 2009-12-17 08:40:35 EST
Review of cups-1.4.2-17.fc13:

Legend: "+" means OK, "-" means not OK.

+ MUST: The spec file name must match the base package %{name}, in the format %{name}.spec
- MUST(1): The package must meet the Packaging Guidelines
+ MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
+ MUST: The License field in spec match the actual license
+ MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file must be included in %doc
+ MUST: The spec file written in American English
- MUST(2): The spec file for the package is legible
+ MUST: The sources used to build the package must match the upstream source, as provided in the spec URL
MUST: The package successfully compile
+ MUST: All build dependencies must be listed in BuildRequires
- MUST(3): The spec file handle locales properly
+ MUST: Every package which stores shared library files in any of the dynamic linker's default paths, must call ldconfig in %post and %postun
- MUST(4): Packages does not bundle copies of system libraries
+ MUST: Package own all directories that it creates
+ MUST: Package does not list a file more than once in the spec file
- MUST(5): Permissions on files must be set properly. Every %files section must include a %defattr(...) line
+ MUST: Package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT)
+ MUST: Package use macros consistently
+ MUST: Package contains code, or permissable content
+ MUST: Large documentation files must go in a -doc subpackage
+ MUST: If a package includes something as %doc, it must not affect the runtime of the application
+ MUST: Header files in a -devel package
+ MUST: Static libraries in a -static package
+ MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
+ 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
+ MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
+ MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built
+ MUST: Packages containing GUI applications must include a %{name}.desktop file
+ MUST: Packages must not own files or directories already owned by other packages
+ MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT)
+ MUST: All filenames in rpm packages must be valid UTF-8.

2:
  - don't define initdir macro, use existing %{_initddir} macro
  - don't use /usr/lib/cups for binaries. Directory /usr/libexec/cups should be
    used instead.
  - don't use obsolete Prereq and BuildReq. Use modern Requires and
    BuildRequires instead.
  - remove unneeded versioned BuildRequires (gcc, libselinux, audit-libs, dbus)
  - drop -fstack-protector-all gcc flag. -fstack-protector is sufficient, I
    think. Consider to build cupsd (and other programs exposed on the network) 
    as PIEs.
  - append %{?_smp_mflags} to "make" command
  - don't use hardcoded paths like /usr/bin, /usr/share, /etc. Use appropriate 
    rpm macro (%{_bindir}, %{_datadir}, %{_sysconfdir}) instead.

3:
  - use %find_lang to package locale files. Check
    https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files 
    for more info.

4:
  - fire away Source4 and use system wide pstopdf, please.

5:
  - vast majority of binaries have incorrect perms. Please use 755 permissions 
    for all shared libraries and binaries.

There are so many rpmlint errors. Please fix them or explain why are they invalid.
Comment 2 Tim Waugh 2010-01-13 12:11:30 EST
(In reply to comment #1)
>   - don't use /usr/lib/cups for binaries. Directory /usr/libexec/cups should be
>     used instead.

I asked upstream whether they would change the default to be /usr/libexec, and the answer was that it was not sufficiently standard for them.  As there are plenty of 3rd party CUPS packages, and they need to know where to put their binaries, we deliberately stick to the upstream path for this to avoid incompatibilities.

>   - remove unneeded versioned BuildRequires (gcc, libselinux, audit-libs, dbus)
>   - drop -fstack-protector-all gcc flag. -fstack-protector is sufficient, I
>     think. Consider to build cupsd (and other programs exposed on the network) 
>     as PIEs.

So, firstly, cupsd is already built as a position-independent executable.  That patch was integrated upstream, and we use the --enable-pie option when calling configure.

As for the -fstack-protector-all flag: are you sure -fstack-protector is sufficient?  It seems to offer less security.  The versioned gcc requirement is for -fstack-protector-all.

> 3:
>   - use %find_lang to package locale files. Check
>     https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files 
>     for more info.

Unfortunately CUPS doesn't use gettext and instead has its own mechanism for installing language files.  It is not compatible with %find_lang.

> 4:
>   - fire away Source4 and use system wide pstopdf, please.

There is no system wide pstopdf.  Note that this is not a generic program but a CUPS filter and must operate in a particular way (see man 7 filter).

Working on the other fixes, thanks for pointing them out.
Comment 3 Adam Tkac 2010-01-14 06:14:41 EST
(In reply to comment #2)
> I asked upstream whether they would change the default to be /usr/libexec, and
> the answer was that it was not sufficiently standard for them.  As there are
> plenty of 3rd party CUPS packages, and they need to know where to put their
> binaries, we deliberately stick to the upstream path for this to avoid
> incompatibilities.

OK, I'm not going to block review due this.

> 
> As for the -fstack-protector-all flag: are you sure -fstack-protector is
> sufficient?  It seems to offer less security.  The versioned gcc requirement is
> for -fstack-protector-all.

Well, -fstack-protector is less secure than -fstack-protector-all but -all causes bigger overhead. It is choice between security and performace. No stack protector means the best performace but the worst security and oppositely. Vast majority of network daemons in Fedora are compiled with -fstack-protector but if you prefer stack-protector-all, use it.
Comment 4 Tim Waugh 2010-01-14 07:51:04 EST
Please take another look at cups-1.4.2-24.fc13.  I've added lots of fixes for the issues raised.

As for rpmlint errors:

cups.x86_64: E: file-in-usr-marked-as-conffile /usr/share/cups/templates/es/class-added.tmpl
[...]

All of these are for bug #441719.

cups.x86_64: E: non-readable /usr/lib/cups/backend/lpd 0700
cups.x86_64: E: non-standard-executable-perm /usr/lib/cups/backend/lpd 0700
cups.x86_64: E: non-readable /usr/lib/cups/backend/serial 0700
cups.x86_64: E: non-standard-executable-perm /usr/lib/cups/backend/serial 0700
cups.x86_64: E: non-readable /usr/lib/cups/backend/dnssd 0700
cups.x86_64: E: non-standard-executable-perm /usr/lib/cups/backend/dnssd 0700
cups.x86_64: E: non-readable /usr/lib/cups/backend/ipp 0700
cups.x86_64: E: non-standard-executable-perm /usr/lib/cups/backend/ipp 0700

All these backends must be run as root.  Backends with execute permission only for the owner need to be run as root.  See the PERMISSIONS section of 'man 7 backend'.

cups.x86_64: E: non-readable /etc/cups/classes.conf 0600
cups.x86_64: E: zero-length /etc/cups/classes.conf
cups.x86_64: E: non-readable /etc/cups/printers.conf 0600
cups.x86_64: E: zero-length /etc/cups/printers.conf
cups.x86_64: E: non-readable /etc/cups/cupsd.conf 0640

These files may contain security-sensitive information.

cups.x86_64: E: non-readable /etc/cups/cupsd.conf.default 0640
cups.x86_64: W: non-conffile-in-etc /etc/cups/cupsd.conf.default

To have the same permissions as the cupsd.conf file it has default content for.  Also, this is *the* default content and is not intended to be configured. (Should it live elsewhere?)

cups.x86_64: E: zero-length /etc/cups/lpoptions
cups.x86_64: E: zero-length /etc/cups/subscriptions.conf
cups.x86_64: E: zero-length /etc/cups/client.conf

Making sure that these files exist in the manifest to ensure they are created with the correct SELinux file contexts.

cups.x86_64: E: non-standard-dir-perm /var/run/cups/certs 0511
cups.x86_64: E: non-standard-dir-perm /etc/cups/ssl 0700
cups.x86_64: E: non-standard-dir-perm /var/spool/cups 0710
cups.x86_64: E: non-standard-dir-perm /var/spool/cups/tmp 01770

These are all correct and reflect their intended use.

cups.x86_64: W: devel-file-in-non-devel-package /usr/share/cups/ppdc/label.h
cups.x86_64: W: devel-file-in-non-devel-package /usr/share/cups/ppdc/escp.h
cups.x86_64: W: devel-file-in-non-devel-package /usr/share/cups/ppdc/pcl.h
cups.x86_64: W: devel-file-in-non-devel-package /usr/share/cups/ppdc/hp.h

These are not devel files in fact and are required at run-time by CUPS DDK drivers (e.g. /usr/share/cups/drv/sample.drv).

cups.x86_64: E: non-readable /usr/sbin/cupsd 0500
cups.x86_64: E: non-standard-executable-perm /usr/sbin/cupsd 0500

See http://cups.org/str.php?L3459 and bug #546004.
Comment 5 Adam Tkac 2010-04-26 07:25:27 EDT
rpmlint output is so long so it will be confusing to paste it here. There are
many false positives, as written in comment #4, because cups is very specific
package.

Following rpmlint issues should be addressed:

cups.src:259: W: macro-in-comment %patch28
cups.src: W: patch-not-applied Patch28: cups-gnutls-gcrypt-threads.patch
^^^ please apply the patch or remove it

cups.x86_64: W: obsolete-not-provided LPRng
^^^ I think LPRng should be provided

cups.x86_64: W: file-not-utf8 /usr/share/doc/cups-1.4.3/CREDITS.txt
^^^ Please convert this file to UTF-8 during build process. `man 1 iconv` is
    your friend

cups-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libcupscgi.so.1 exit@GLIBC_2.2.5
^^^ Please explain why it is acceptable (for example self-implemented stack protector
    is a good reason to call exit inside library)

cups-lpd.x86_64: W: spelling-error %description -l en_US provices
cups-php.x86_64: W: spelling-error %description -l en_US provices
^^^ Probably typos

Remaining issues from comment #1:

The PIE problem:
- per buildlog --enable-pie does nothing:
  ./configure ...
  ...
  configure: WARNING: unrecognized options: ..., --enable-pie, ...
  ...
- it seems you use -pie/-fpie flags wrongly. You use only -fPIE during linking
  which is wrong. You should use -fPIE during compilation and -pie during
  linking. `man 1 gcc` clearly says:
  "
  -pie
    Produce a position independent executable on targets which support it.
    For predictable results, you must also specify the same set of options
    that were used to generate code (-fpie, -fPIE, or model suboptions)
    when you specify this option.
  "
- to be precise please use -fpie during compilation on all platforms except
  sparcv9 sparc64 s390 and s390x. In "bind" specfile there are following lines:

# Sparc and s390 arches need to use -fPIE
%ifarch sparcv9 sparc64 s390 s390x
for i in bin/named{,-sdb}/{,unix}/Makefile.in; do
        sed -i 's|fpie|fPIE|g' $i
done
%endif

I don't see other problems.
Comment 6 Tim Waugh 2011-02-25 10:57:42 EST
(In reply to comment #5)
> cups.src:259: W: macro-in-comment %patch28
> cups.src: W: patch-not-applied Patch28: cups-gnutls-gcrypt-threads.patch
> ^^^ please apply the patch or remove it

Gone.

> cups.x86_64: W: obsolete-not-provided LPRng
> ^^^ I think LPRng should be provided

The history behind this is in bug #148757.

> cups.x86_64: W: file-not-utf8 /usr/share/doc/cups-1.4.3/CREDITS.txt
> ^^^ Please convert this file to UTF-8 during build process. `man 1 iconv` is
>     your friend

I believe it is now UTF-8 upstream.

> cups-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libcupscgi.so.1
> exit@GLIBC_2.2.5
> ^^^ Please explain why it is acceptable (for example self-implemented stack
> protector
>     is a good reason to call exit inside library)

This shared library is only used internally by CUPS (it is used by several of the CGI handlers).  It is simply to share code between CUPS programs.

> cups-lpd.x86_64: W: spelling-error %description -l en_US provices
> cups-php.x86_64: W: spelling-error %description -l en_US provices
> ^^^ Probably typos

Both fixed.

> The PIE problem:

I've removed --enable-pie from the configure options as this is (sort of) done automatically upstream.

The upstream build process is not sophisticated enough to handle passing exactly the right parameters.  It uses -fPIC (not -fPIE) when compiling, which Jakub assures me is simply slightly less efficient that -fPIE in some cases.  Indeed, the resulting executables behave as PIEs and seem to have the correct ELF flags set.

The upstream discussion about it is here:
http://cups.org/str.php?L3691

Please take a look at cups-1.4.6-6.fc14.
Comment 7 Adam Tkac 2011-03-08 06:06:04 EST
Ok, I checked cups-1.4.6-13.fc16 and it seems fine for me. You can close the review now.
Comment 8 Tim Waugh 2011-03-08 06:33:44 EST
Thanks!

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