Bug 1451407 - Review Request: annobin - a gcc plugin to record extra information in compiled files
Summary: Review Request: annobin - a gcc plugin to record extra information in compile...
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Stephen Gallagher
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-05-16 15:04 UTC by Nick Clifton
Modified: 2017-07-27 14:22 UTC (History)
3 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2017-07-27 14:22:59 UTC
sgallagh: fedora-review+


Attachments (Terms of Use)

Description Nick Clifton 2017-05-16 15:04:33 UTC
Spec URL: https://nickc.fedorapeople.org/annobin.spec
SRPM URL: https://nickc.fedorapeople.org/annobin-1.0-1.fc26.src.rpm

Description: 
A plugin for GCC that records extra information in the files that it compiles.
This information can be used to analyze the files, either by static tools that 
scan binaries for specific purposes, or by the dynamic linker when it loads 
the files.

Note - the intent is that, assuming that this package is accepted, the 
redhat-rpm-macros package will be updated to require it, and to add an 
invocation of the plugin on the gcc command line.  In that way all future
compiled binaries will contain the information recorded by this plugin.

The package includes a few example shell scripts that consume the information
generated by the plugin.  Of these hardended.sh is probably of the most
interest.  It checks compiled binaries to make sure that they have been
built with all of the recommended hardening options.


Fedora Account System Username: nickc

Comment 1 Stephen Gallagher 2017-06-16 13:11:25 UTC
The SRPM link is a 403, so it cannot be reviewed. Please correct this.

Comment 2 Nick Clifton 2017-06-16 14:33:21 UTC
Sorry - bad permissions on the file.  Now fixed.

Comment 3 Stephen Gallagher 2017-06-22 13:39:18 UTC
Apologies for the delay on this. Things got hectic.


tl;dr: work needed (see Issues:)


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- All build dependencies are listed in BuildRequires, except for any that
  are listed in the exceptions section of Packaging Guidelines.
  Note: These BR are not needed: gcc gcc-c++
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2

- Some content found to have MIT/X11 (BSD like) licenses. This needs to be in the License: field

- Why are you explicitly excluding the LICENSE and COPYING3 files? Those need to be installed with the %license macro

- This package does not own some of the directories it requires or alternately depend on a package that requires it. In all likelihood, you need to have this package `Requires: gcc` to ensure that its directory structure is properly owned. I also doubt anyone would want to install this plugin without also having gcc available.

- /usr/lib/gcc/x86_64-redhat-linux/7/plugin is also owned by gcc-gdb-plugin. This is *not* a blocker, but it's worth asking the gcc folks if they should just have this owned by some common package if it's going to start being used by multiple plugins.

- Drop the %clean section; it's not required on any supported version of Fedora or EPEL anymore


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
     Note: Unversioned so-files in private %_libdir subdirectory (see
     attachment). Verify they are not in ld path.
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: 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 is included in %license.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "MIT/X11 (BSD like)", "GPL (v2 or later)", "*No copyright* GPL
     (v3)", "Unknown or generated", "GPL (v3 or later)". 43 files have
     unknown license. Detailed output of licensecheck in
     /dev/shm/1451407-annobin/licensecheck.txt
[!]: License file installed when any subpackage combination is installed.
[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/lib/gcc/x86_64-redhat-
     linux/7, /usr/lib/gcc, /usr/lib/gcc/x86_64-redhat-linux
[!]: Package does not own files or directories owned by other packages.
     Note: Dirs in package are owned also by: /usr/lib/gcc/x86_64-redhat-
     linux/7/plugin(gcc-gdb-plugin)
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[!]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package requires other packages for directories it uses.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[!]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: %clean present but not required
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[!]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in annobin-
     debuginfo
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: annobin-1.0-1.fc27.x86_64.rpm
          annobin-debuginfo-1.0-1.fc27.x86_64.rpm
          annobin-1.0-1.fc27.src.rpm
annobin.x86_64: W: incoherent-version-in-changelog 1.0-1.fc26 ['1.0-1.fc27', '1.0-1']
annobin.x86_64: W: no-documentation
annobin.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
annobin.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
annobin.x86_64: W: no-manual-page-for-binary built-by.sh
annobin.x86_64: W: no-manual-page-for-binary check-abi.sh
annobin.x86_64: W: no-manual-page-for-binary hardened.sh
annobin.src: W: strange-permission annobin-1.0.tar.xz 660
3 packages and 0 specfiles checked; 0 errors, 8 warnings.




Rpmlint (debuginfo)
-------------------
Checking: annobin-debuginfo-1.0-1.fc27.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
sh: /usr/bin/python: No such file or directory
annobin.x86_64: W: incoherent-version-in-changelog 1.0-1.fc26 ['1.0-1.fc27', '1.0-1']
annobin.x86_64: W: no-documentation
annobin.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
annobin.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
annobin.x86_64: W: no-manual-page-for-binary built-by.sh
annobin.x86_64: W: no-manual-page-for-binary check-abi.sh
annobin.x86_64: W: no-manual-page-for-binary hardened.sh
2 packages and 0 specfiles checked; 0 errors, 7 warnings.



Requires
--------
annobin (rpmlib, GLIBC filtered):
    /bin/bash
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libm.so.6()(64bit)
    libstdc++.so.6()(64bit)
    rtld(GNU_HASH)

annobin-debuginfo (rpmlib, GLIBC filtered):



Provides
--------
annobin:
    annobin
    annobin(x86-64)

annobin-debuginfo:
    annobin-debuginfo
    annobin-debuginfo(x86-64)



Unversioned so-files
--------------------
annobin: /usr/lib/gcc/x86_64-redhat-linux/7/plugin/annobin.so

Source checksums
----------------
https://nickc.fedorapeople.org/annobin-1.0.tar.xz :
  CHECKSUM(SHA256) this package     : b2e232a91158f9e949655375281fd091c360b3cfae51e561997d635dec3ac1ed
  CHECKSUM(SHA256) upstream package : b2e232a91158f9e949655375281fd091c360b3cfae51e561997d635dec3ac1ed


Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -b 1451407 -m fedora-rawhide-x86_64
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

Comment 4 Nick Clifton 2017-06-29 15:04:49 UTC
Hi Stephen,

> Apologies for the delay on this. Things got hectic.

No worries - thanks very much for taking the time to run the review.


> tl;dr: work needed (see Issues:)

No surprises there - this is my first contribution. :-)


Anyway, on to the review.  I have created an updated version of the
submissions which addresses most of the points raised:

  Spec URL: https://nickc.fedorapeople.org/annobin.spec
  SRPM URL: https://nickc.fedorapeople.org/annobin-2.0-1.fc26.src.rpm


> Issues:
> =======
> - All build dependencies are listed in BuildRequires, except for any that
>    are listed in the exceptions section of Packaging Guidelines.
>    Note: These BR are not needed: gcc gcc-c++
>    See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2

OK, I will remove them.  (Does it matter if they are present ?)

(FYI: I know that the Fedora Review Guidelines webpage contains that link above,
but the Exceptions_2 entry on the Guidelines page does not actually specify which
requirements can be omitted from the BuildRequires tag).


> - Some content found to have MIT/X11 (BSD like) licenses. This needs to be in
> the License: field

OK, this is now done.

Note - the fedora-review tool flags quite a few files as "Unknown or generated".
I assume that I can ignore these.  Also it flags the LICENSE file as having
"*No copyright* GPL (v3)".  I assume that this is OK too.


> - Why are you explicitly excluding the LICENSE and COPYING3 files? Those need
> to be installed with the %license macro

I was basing my annobin.spec file on the spec file used by the odb package.
(This is another gcc plugin, so it seemed like a good idea at the time).  That
package excludes the LICENSE/COPYING3 files and does not have a %license entry,
so I thought that that was the correct thing to do for plugins.

Anyway I have now corrected this issue and added the %license entry.


> - This package does not own some of the directories it requires or alternately
> depend on a package that requires it. In all likelihood, you need to have this
> package `Requires: gcc` to ensure that its directory structure is properly
> owned. I also doubt anyone would want to install this plugin without also
> having gcc available.

True.

> - /usr/lib/gcc/x86_64-redhat-linux/7/plugin is also owned by gcc-gdb-plugin.
> This is *not* a blocker, but it's worth asking the gcc folks if they should
> just have this owned by some common package if it's going to start being used
> by multiple plugins.

Oh how I wish... :-)  Does Fedora support meta-packages ?  Ie could we have a
gcc-plugin meta package that owns this directory, and maybe a documentation
directory as well.  It could also link somehow to each of the currently existing
gcc plugin packages.

In a related note, I have been trying to get the gcc folks to take more
responsibility for plugins, and to start treating them as an official part
of the compiler.  But so far I have been blocked each time I try.  Plugins are
seen as being a bit of a mess and not suitable for inclusion in a real compiler
project. :-(


> - Drop the %clean section; it's not required on any supported version of Fedora
> or EPEL anymore

Done.


> [!]: Final provides and requires are sane (see attachments).

Umm, not sure what needs to be done here.  Could you provide me with a
pointer or two please ?


> [!]: %check is present and all tests pass.

I have not done this (yet) as I am still trying to work out a clean
way to test the plugin.  I hope however that this is not a showstopper.


> annobin.x86_64: W: no-documentation

This should be done.

> annobin.x86_64: W: no-manual-page-for-binary built-by.sh
> annobin.x86_64: W: no-manual-page-for-binary check-abi.sh
> annobin.x86_64: W: no-manual-page-for-binary hardened.sh

These are shell scripts.  As such I have tried to make them
self-documenting.  Is there a way to tell rpmlint that this is the
situation ?


Cheers
  Nick

Comment 5 Stephen Gallagher 2017-06-29 18:18:15 UTC
(In reply to Nick Clifton from comment #4)
...
> > Issues:
> > =======
> > - All build dependencies are listed in BuildRequires, except for any that
> >    are listed in the exceptions section of Packaging Guidelines.
> >    Note: These BR are not needed: gcc gcc-c++
> >    See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2
> 
> OK, I will remove them.  (Does it matter if they are present ?)
> 

They're not harmful. Actually I meant not to include that in the final list of issues, but forgot to delete it.

> (FYI: I know that the Fedora Review Guidelines webpage contains that link
> above,
> but the Exceptions_2 entry on the Guidelines page does not actually specify
> which
> requirements can be omitted from the BuildRequires tag).
> 

Yeah, actually in retrospect it's probably better to include them, on the off-chance they ever got removed from the default buildroot.

> 
> > - Some content found to have MIT/X11 (BSD like) licenses. This needs to be in
> > the License: field
> 
> OK, this is now done.
> 
> Note - the fedora-review tool flags quite a few files as "Unknown or
> generated".
> I assume that I can ignore these.  Also it flags the LICENSE file as having
> "*No copyright* GPL (v3)".  I assume that this is OK too.
> 

Don't assume that the "Unknown or generated" ones are ignorable. Best to double-check. In general, we assume that if they have no license information, they're covered by the project license.

> 
> > - Why are you explicitly excluding the LICENSE and COPYING3 files? Those need
> > to be installed with the %license macro
> 
> I was basing my annobin.spec file on the spec file used by the odb package.
> (This is another gcc plugin, so it seemed like a good idea at the time). 
> That
> package excludes the LICENSE/COPYING3 files and does not have a %license
> entry,
> so I thought that that was the correct thing to do for plugins.
> 
> Anyway I have now corrected this issue and added the %license entry.
> 

Thanks
> 
> > - This package does not own some of the directories it requires or alternately
> > depend on a package that requires it. In all likelihood, you need to have this
> > package `Requires: gcc` to ensure that its directory structure is properly
> > owned. I also doubt anyone would want to install this plugin without also
> > having gcc available.
> 
> True.
> 
> > - /usr/lib/gcc/x86_64-redhat-linux/7/plugin is also owned by gcc-gdb-plugin.
> > This is *not* a blocker, but it's worth asking the gcc folks if they should
> > just have this owned by some common package if it's going to start being used
> > by multiple plugins.
> 
> Oh how I wish... :-)  Does Fedora support meta-packages ?  Ie could we have a
> gcc-plugin meta package that owns this directory, and maybe a documentation
> directory as well.  It could also link somehow to each of the currently
> existing
> gcc plugin packages.

As I said, this isn't a blocker, but it's completely acceptable to have a subpackage like `foo-filesystem` that contains only empty directory structures and have packages depend on that. It's recommended that anything that supports plugins should have either the main package or a -filesystem package provide the necessary framework.

> 
> In a related note, I have been trying to get the gcc folks to take more
> responsibility for plugins, and to start treating them as an official part
> of the compiler.  But so far I have been blocked each time I try.  Plugins
> are
> seen as being a bit of a mess and not suitable for inclusion in a real
> compiler
> project. :-(
> 

Above my pay-grade.

> 
> > - Drop the %clean section; it's not required on any supported version of Fedora
> > or EPEL anymore
> 
> Done.
> 
> 
> > [!]: Final provides and requires are sane (see attachments).
> 
> Umm, not sure what needs to be done here.  Could you provide me with a
> pointer or two please ?
> 

Sorry, I should have mentioned that this was just fallout from not having `Requires: gcc`

> 
> > [!]: %check is present and all tests pass.
> 
> I have not done this (yet) as I am still trying to work out a clean
> way to test the plugin.  I hope however that this is not a showstopper.
>

This is non-blocking. It's a SHOULD criterion. I marked it as [!] because %check wasn't present.
 
> 
> > annobin.x86_64: W: no-documentation
> 
> This should be done.
> 
> > annobin.x86_64: W: no-manual-page-for-binary built-by.sh
> > annobin.x86_64: W: no-manual-page-for-binary check-abi.sh
> > annobin.x86_64: W: no-manual-page-for-binary hardened.sh
> 
> These are shell scripts.  As such I have tried to make them
> self-documenting.  Is there a way to tell rpmlint that this is the
> situation ?
> 

They're warnings, not errors. It's mostly a reminder in case you forgot to package the manual.

Comment 6 Stephen Gallagher 2017-06-29 18:33:40 UTC
OK, review time:

The `License:` field is not in the required format. See https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#License:_field

It should be:
```
License: GPLv3+ and MIT
```
When you have both GPLv2+ and GPLv3+ code together, GPLv3+ covers both. The MIT/X11 license is represented by "MIT" in Fedora.


Are `built-by.sh`, `check-abi.sh` and `hardened.sh` really tools that belong in /usr/bin, or are they internal to the package?

Comment 7 Nick Clifton 2017-06-30 15:49:28 UTC
Hi Stephan,

  Thanks for the second review.  I have uploaded a revised annobin.spec file with the License tag corrected as you indicated.

  The scripts (built-by, check-abi and hardended) are real tools that are intended to be run by users.  (Well users of the annobin-plugin package, which in practice will only be developers and distributors).  What they actually do is process the information that the annobin plugin inserts into compiled binaries.  So the built-by script for example extract the information about the compiler used, whilst the hardended script checks that all of the appropriate hardening options have been enabled, and so on.

  Finally all of the "unknown/generated" results in the licensecheck.txt file are suitable for being covered by the project license.

Cheers
  Nick

Comment 8 Stephen Gallagher 2017-06-30 19:37:43 UTC
(In reply to Nick Clifton from comment #7)
> Hi Stephan,
> 
>   Thanks for the second review.  I have uploaded a revised annobin.spec file
> with the License tag corrected as you indicated.
> 
>   The scripts (built-by, check-abi and hardended) are real tools that are
> intended to be run by users.  (Well users of the annobin-plugin package,
> which in practice will only be developers and distributors).  What they
> actually do is process the information that the annobin plugin inserts into
> compiled binaries.  So the built-by script for example extract the
> information about the compiler used, whilst the hardended script checks that
> all of the appropriate hardening options have been enabled, and so on.
> 
>   Finally all of the "unknown/generated" results in the licensecheck.txt
> file are suitable for being covered by the project license.
> 


https://nickc.fedorapeople.org/annobin.spec still shows the wrong License: field.

Comment 9 Nick Clifton 2017-07-14 09:45:36 UTC
(In reply to Stephen Gallagher from comment #8)
 
> https://nickc.fedorapeople.org/annobin.spec still shows the wrong License:
> field.

Doh!  Sorry about that.  This should now be fixed.

Cheers
  Nick

Comment 10 Stephen Gallagher 2017-07-14 11:02:51 UTC
Package is approved.

Comment 11 Gwyn Ciesla 2017-07-26 12:12:50 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/annobin

Comment 12 Nick Clifton 2017-07-27 14:22:59 UTC
Package built


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