This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 225660 - Merge Review: crash
Merge Review: crash
Status: NEW
Product: Fedora
Classification: Fedora
Component: crash (Show other bugs)
23
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Package Reviews List
:
Depends On: 1044119
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 12:52 EST by Nobody's working on this, feel free to take it
Modified: 2015-07-15 11:27 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
revised spec according to fedora guidelines (2.13 KB, text/plain)
2009-01-09 22:27 EST, manuel wolfshant
no flags Details

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

http://cvs.fedora.redhat.com/viewcvs/devel/crash/
Initial Owner: anderson@redhat.com
Comment 1 manuel wolfshant 2009-01-09 22:27:09 EST
Created attachment 328610 [details]
revised spec according to fedora guidelines

I've taken a look at the current version (http://people.redhat.com/anderson/crash-4.0-7.6.src.rpm) and did a local mock rebuild. I am attaching a slightly revised spec which fixes part of the problems.

The major issues (I have not tried to fix any of these because I am not familiar with the requirements of this special package) are that
1. the package uses it's own versions of several utilities/libs (at least gdb and readline) which contradicts the guidelines. However, according to http://people.redhat.com/anderson/crash_whitepaper/ this is intended.
2.the package builds a static binary. Once again, this is intended
3.fedora compile flags are completely ignored. This also seems to be intended, each of the libraries is built with a different set of flags.


Easily fixable stuff (not done):
- SMP_FLAGS are not used (I did not know if it's OK to use them )
- the source files (included in -debuginfo) have the exec bit set, which makes rpmlint unhappy
- license seems to be GPLv2+. A lot of files are GPL+, some are GPLv2+, some have no license at all. A cleanup of those would be nice

Other problems (fixed)
- usage of forbidden tags (packager, vendor) or incorrect values (buildroot, source0)
- some aesthetic warnings from rpmlint
- missing license file from the binary rpm
- there was no changelog. I have added one but it should be properly populated
- timestamps (for docs/man) were not preserved
- defattr was missing in %files
Comment 2 Lubomir Rintel 2009-01-18 16:45:00 EST
(In reply to comment #1)
> - license seems to be GPLv2+. A lot of files are GPL+, some are GPLv2+, some
> have no license at all. A cleanup of those would be nice

Certain files (xen_hyper*) use GPLv2 (only), spot already fixed this in CVS.

> Other problems (fixed)

There are yet more:

- You use "Revision" tag to mark upstream release, which is wrong. It is meant to be used to version the SPEC file. Given you (package owner, "crash" group, seem to be upstream, you can definitely fix this by changing the versioning scheme. (e.g use 4.0.8 instead of 4.0-8))

- The bundled gdb is old and has issues. It is likely that some of older GDB security problems affect it:

http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2005-1704
http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2005-1705
http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2006-4146

Please address those, if they are relevant. Notify your SRT that you bundle GDB code and communicate with GDB upstream (or people involved in Archer, your colleagues) to avoid having to bundle GDB in longer run.
Comment 3 Dave Anderson 2009-01-23 12:11:22 EST
I'm not sure why the review was made of the crash.spec file from the
upstream package instead of the Fedora version?  When updating the Fedora
package, the upstream spec file is not pulled into Fedora.  In any case,
I'm presuming that the relevant issues brought up re: the upstream spec 
file should be addressed in the Fedora version if they haven't already.  

The Fedora crash utility NVR scheme has (up until now) simply mirrored the
upstream version upon which it was based, but I can see the validity in
encapsulating the upstream NVR into the Fedora version.  It's been an
annoyance in the past when somebody has come in (unbeknownst to me) and
bumped up the release number, which in turn screwed up the upstream NVR
relationship.

With respect to the CVE's, both 1704 and 4146 are highly unlikely
to be issues given that the crash utility does a number of checks
on the vmlinux object file prior to the embedded gdb module ever
being invoked.  (i.e., using the supplied sample hand-carved object
files, they get rejected by the crash code as not being legitimate vmlinux files)  Nonetheless, the patches for both of those issues are reasonable and
safe to add, and I have done so.  I've also incorporated the patch for
the .gdbinit-related 1705 issue.    

When I update the upstream version, I will follow up with a Fedora
update -- after I figure out how to check into my account given that
I haven't been there since the Fedora break-in.

Thanks,
  Dave Anderson
Comment 4 manuel wolfshant 2009-01-23 12:22:30 EST
(In reply to comment #3)
I reviewed the upstream version because by definition so to say Fedora wants to have the latest version of the software, and the content in the cvs is very old. I assumed it will be easier for you to maintain a single or at least very similar spec for both places. I apologize if I made an error by this assumption. Let me know what is the URL of the files that I should have reviewed and I will do it.

And yes, the relevant issues brought up re: the upstream spec file must be addressed in the Fedora version. And no, I did not touch the spec exactly because I knew that there is an active maintainer who might have a well defined agenda.

As of relying on a bundled application, this is not usually permitted in Fedora and therefore they should be discussed.
Comment 5 manuel wolfshant 2009-01-23 12:23:29 EST
I meant here "relying on a bundled version of an existing application"
Comment 6 Dave Anderson 2009-01-23 14:06:25 EST
> As of relying on a bundled application, this is not usually permitted in Fedora
> and therefore they should be discussed.

The crash utility package is essentially built around, on-top-of, 
wrapped-around, or however you want to put it, and it's designed that
way on purpose.  There's no way it's ever going to be "unbundled".
Comment 7 Dave Anderson 2009-01-23 14:20:56 EST
> I reviewed the upstream version because by definition so to say Fedora wants to
> have the latest version of the software, and the content in the cvs is very
> old. I assumed it will be easier for you to maintain a single or at least very
> similar spec for both places. 

Not really.  The upstream src.rpm (and its minimal .spec file) was added
as a convenience to those users who prefer that format to the tar.gz file.

> I apologize if I made an error by this assumption. Let me know what is
> the URL of the files that I should have reviewed and I will do it.

The "content in the cvs" (Fedora) is currently based upon the upstream
4.0-6.3 release from last April, and its .spec file updated when Tom Calloway
rebuilt it as 4.0-7 this past August.  Anyway, the .spec file in the devel
branch was -- as I recall -- carried over from a RHEL package version some
years ago.  Anyway, that's the only .spec file I've ever touched w/respect
to Fedora, and that's the one that I would have presumed would be the one
that would be "review-able".
Comment 8 manuel wolfshant 2009-03-07 21:58:32 EST
It would have been nice if a mention of the existence of a new version in the CVS + a link to the new spec and src.rpm would have been provided here. Back where I come from this is called courtesy.

Current issues, as of version crash-4_0-8_7_2_fc11
1. 
$ rpmlint *rpm
crash.src: E: summary-too-long Kernel analysis utility for live systems, netdump, diskdump, kdump, LKCD or mcore dumpfiles
crash.x86_64: W: spurious-executable-perm /usr/share/doc/crash-4.0/COPYING
crash.x86_64: E: summary-too-long Kernel analysis utility for live systems, netdump, diskdump, kdump, LKCD or mcore dumpfiles
crash-devel.x86_64: W: no-documentation
crash-devel.x86_64: W: summary-not-capitalized kernel crash analysis utility for live systems, netdump, diskdump, kdump, LKCD or mcore dumpfiles
crash-devel.x86_64: E: summary-too-long kernel crash analysis utility for live systems, netdump, diskdump, kdump, LKCD or mcore dumpfiles
3 packages and 0 specfiles checked; 3 errors, 3 warnings.
The no documentation for -devel is ignorable. The error messages are easy fixable by shortening the summary lines.

2. Source should be "http://people.redhat.com/anderson/crash-4.0-7.7.tar.gz" (full URL to the source should be provided, not just a filename)

3. ppc is excluded from the list of architectures. However, ppc64 is included (and koji builds happily for it) so I have to ask: is excluding ppc really intended or just an oversight?

4. The package uses its private copies of several programs (gdb, binutils, readline) without providing any reason for that. At least a note should be included in the spec, explaining the reason. And I think that an exception must be requested from the FPC.

5. The package ignores the compiler flags which are mandatory according to the guidelines. Once again, no reason is provided. On top of that, the flags are not consistently used (gdb gets " -c -DHAVE_CONFIG_H -g -O2 -I. -I./../include  -W -Wall -Wtraditional -pedantic", readline gets " -c -DHAVE_CONFIG_H   -I. -I. -DRL_LIBRARY_VERSION='"4.3"' -g -O2",  while crash itself is built using  "gcc -c -g -O2    -I. -I. -I./config -DLOCALEDIR="\"/usr/local/share/locale\"" -DCRASH_MERGE -DHAVE_CONFIG_H -I./../include/opcode -I./../readline/.. -I../bfd -I./../bfd -I./../include -I../intl -I./../intl  -DMI_OUT=1 -DTUI=1 -Wimplicit -Wreturn-type -Wcomment -Wtrigraphs -Wformat -Wparentheses -Wpointer-arith -Wuninitialized -Wformat-nonliteral -Wunused-label -Wunused-function "

6. Parallel make (https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make ) is not used. Once again, no reason provided.
Comment 9 manuel wolfshant 2009-04-10 21:18:15 EDT
I am releasing the review.
Due to $JOB issues I no longer have the time and the willingness to watch over the shoulder of an unresponsive maintainer.
Comment 10 Cole Robinson 2015-02-11 15:35:48 EST
Mass reassigning all merge reviews to their component. For more details, see this FESCO ticket:

  https://fedorahosted.org/fesco/ticket/1269

If you don't know what merge reviews are about, please see:

  https://fedoraproject.org/wiki/Merge_Reviews

How to handle this bug is left to the discretion of the package maintainer.
Comment 11 Jan Kurik 2015-07-15 11:27:10 EDT
This bug appears to have been reported against 'rawhide' during the Fedora 23 development cycle.
Changing version to '23'.

(As we did not run this process for some time, it could affect also pre-Fedora 23 development
cycle bugs. We are very sorry. It will help us with cleanup during Fedora 23 End Of Life. Thank you.)

More information and reason for this action is here:
https://fedoraproject.org/wiki/BugZappers/HouseKeeping/Fedora23

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