Bug 223943 (nemiver)
Summary: | Review Request: Nemiver - A C/C++ Debugger for GNOME - point, click, debug! | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Peter Gordon <peter> | ||||
Component: | Package Review | Assignee: | Gianluca Sforna <giallu> | ||||
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | bugs.michael, dodji, paul | ||||
Target Milestone: | --- | Flags: | giallu:
fedora-review+
dennis: fedora-cvs+ |
||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2007-04-07 20:13:49 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | |||||||
Bug Blocks: | 163779 | ||||||
Attachments: |
|
Description
Peter Gordon
2007-01-23 07:03:39 UTC
[ Adding alias. ] * missing "BuildRequires: gettext" * in -devel package, missing "Requires: gnome-vfs2-devel glib2-devel" These are "Requires" in the pkgconfig file. glib2-devel is redundant (due to glibmm24-devel), but a direct requirement. * Avoid duplicating the licence file "COPYING" in sub-packages. Here the -devel package requires the main package. * GConf schema files are no %config files and should not be marked as such. * In the scriptlets, you list the two schema files without wildcards. You ought to do the same in the %files section, so you will notice when the set of schema files changes. * %defattr missing in -devel package (In reply to comment #2) > * missing "BuildRequires: gettext" > * %defattr missing in -devel package > * in -devel package, missing "Requires: gnome-vfs2-devel glib2-devel" > These are "Requires" in the pkgconfig file. glib2-devel is redundant > (due to glibmm24-devel), but a direct requirement. Oopsie. Fixed in release 2. > * Avoid duplicating the licence file "COPYING" in sub-packages. > Here the -devel package requires the main package. I'm of the opinion that such license files should always be included in any subpackage ot make the licensing explicitly clear. However, since there is not officially any such guideline, I don't think this a blocker at all. > * GConf schema files are no %config files and should not be > marked as such. My understanding is that schemas should be marked %config, as they are the base configurations for the package which the system administrator can change as he or she sees fit, etc. Several other of my packages which use gconf schemas have them marked as %config(noreplace). Why is it invalid? Thanks. > * In the scriptlets, you list the two schema files without wildcards. > You ought to do the same in the %files section, so you will notice > when the set of schema files changes. Oooooooh. Good catch; thanks. Fixed in release 2. URLs for release 2: Spec: http://thecodergeek.com/downloads/fedora/nemiver.spec SRPM: http://thecodergeek.com/downloads/fedora/nemiver-0.3.0-2.src.rpm You would not edit installed schema files if you wanted to change the system-wide GConf defaults. You would edit the key/value pairs in the database, e.g. with gconf-editor or gconftool-2. Modifying schema files leads to non-trivial upgrade scenarios, especially when they are marked as %config(noreplace). (In reply to comment #4) > You would not edit installed schema files if you wanted to change > the system-wide GConf defaults. You would edit the key/value > pairs in the database, e.g. with gconf-editor or gconftool-2. > Modifying schema files leads to non-trivial upgrade scenarios, > especially when they are marked as %config(noreplace). Aaahh that makes a lot of sense. Thanks for the explanation. I fixed the schemas stuff in release 3: Spec: http://thecodergeek.com/download/fedora/nemiver.spec Spec: http://thecodergeek.com/download/fedora/nemiver-0.3.0-3.src.rpm However, now rpmlint gives me two messages about that: W: nemiver non-conffile-in-etc /etc/gconf/schemas/nemiver-workbench.schemas W: nemiver non-conffile-in-etc /etc/gconf/schemas/nemiver-dbgperspective.schemas I take it there are safe to ignore for GConf schemas then? Thanks. (In reply to comment #5) > > However, now rpmlint gives me two messages about that: > W: nemiver non-conffile-in-etc /etc/gconf/schemas/nemiver-workbench.schemas > W: nemiver non-conffile-in-etc /etc/gconf/schemas/nemiver-dbgperspective.schemas > I take it there are safe to ignore for GConf schemas then? > Yap, that's the usual warnings on about schema files. If everyone agrees on that, rpmlint could be modified to check the reverse condition, i.e.: everything under /etc/gconf/schema triggers a warning if it is actually marked as %config (In reply to comment #5) > > Spec: http://thecodergeek.com/download/fedora/nemiver.spec > Spec: http://thecodergeek.com/download/fedora/nemiver-0.3.0-3.src.rpm > Correct links seems to be: Spec: http://thecodergeek.com/downloads/fedora/nemiver.spec SRPM: http://thecodergeek.com/downloads/fedora/nemiver-0.3.0-3.src.rpm (In reply to comment #7) > > Correct links seems to be: > Spec: http://thecodergeek.com/downloads/fedora/nemiver.spec > SRPM: http://thecodergeek.com/downloads/fedora/nemiver-0.3.0-3.src.rpm > Err...dang typos. Thanks, Gianluca. Build successfully in mock for FC5. rpmlint on src.rpm has only one small complaint /var/lib/mock/fedora-5-i386-core/result/nemiver-0.3.0-3.fc5.src.rpm W: nemiver mixed-use-of-spaces-and-tabs (spaces: line 150, tab: line 1) I'd like to try it out, but there are some issues related to SELinux; this is what I have in audit.log when running nemiver: type=AVC msg=audit(1169803885.025:2173): avc: denied { execmod } for pid=32735 comm="nemiver" name="libdbgperspectiveplugin.so" dev=dm-0 ino=12452120 scontext=user_u:system_r:unconfined_t:s0 tcontext=system_u:object_r:lib_t:s0 tclass=file type=SYSCALL msg=audit(1169803885.025:2173): arch=40000003 syscall=125 success=no exit=-13 a0=d0c000 a1=20d000 a2=5 a3=bfc34b00 items=0 ppid=1 pid=32735 auid=500 uid=500 gid=500 euid=500 suid=500 fsuid=500 egid=500 sgid=500 fsgid=500 tty=(none) comm="nemiver" exe="/usr/bin/nemiver" subj=user_u:system_r:unconfined_t:s0 key=(null) type=AVC_PATH msg=audit(1169803885.025:2173): path="/usr/lib/nemiver/plugins/dbgperspective/libdbgperspectiveplugin.so" Try getting the plugins to compile with -fPIC and see if that helps with the SELinux issue. The main code uses -fPIC but it isn't propagated down to the plugins. Paul, how do I go about this? I've tried adding '--with-pic' to the %configure call, but there is nothing different in the build log about PIC/non-PIC code generation from not having that. Poking through the configure.ac reveals nothing obvious either. :S Thanks. Add -fPIC to CFLAGS/CXXFLAGS maybe? Adding the following three lines to %build before the %configure line certainly results in all compiles getting -fPIC; hopefully Gianluca can let us know if that fixes the execmod problem. # Make sure everything is compiled with -fPIC to avoid execmod problems export CFLAGS="%{optflags} -fPIC" export CXXFLAGS="%{optflags} -fPIC" Created attachment 147096 [details] mock build log with -fPIC Still no luck. [giallu@molzilla SPECS]$ nemiver |X|virtual GModule* nemiver::common::DynamicModule::Loader::load_library_from_path(const nemiver::common::UString&):nmv-dynamic-module.cc:284:raised exception: failed to load shared library /usr/lib/nemiver/plugins/dbgperspective/libdbgperspectiveplugin.so: /usr/lib/nemiver/plugins/dbgperspective/libdbgperspectiveplugin.so: cannot restore segment prot after reloc: Permission denied |X|void nemiver::common::Plugin::load_entry_point():nmv-plugin.cc:201:raised exception: failed to load plugin entry point 'dbgperspective for plugin 'dbgperspective' |E|nemiver::common::PluginSafePtr nemiver::common::PluginManager::load_plugin_from_path(const nemiver::common::UString&, std::vector<nemiver::common::SafePtr<nemiver::common::Plugin, nemiver::common::ObjectRef, nemiver::common::ObjectUnref>, std::allocator<nemiver::common::SafePtr<nemiver::common::Plugin, nemiver::common::ObjectRef, nemiver::common::ObjectUnref> > >&):nmv-plugin.cc:562:Failed to load dependant plugin 'dbgperspective and (in audit.log) type=AVC msg=audit(1170328411.204:2435): avc: denied { execmod } for pid=21468 comm="nemiver" name="libdbgperspectiveplugin.so" dev=dm-0 ino=25724931 scontext=user_u:system_r:unconfined_t:s0 tcontext=system_u:object_r:lib_t:s0 tclass=file type=SYSCALL msg=audit(1170328411.204:2435): arch=40000003 syscall=125 success=no exit=-13 a0=641000 a1=215000 a2=5 a3=bfb3d6e0 items=0 ppid=20331 pid=21468 auid=500 uid=500 gid=500 euid=500 suid=500 fsuid=500 egid=500 sgid=500 fsgid=500 tty=pts4 comm="nemiver" exe="/usr/bin/nemiver" subj=user_u:system_r:unconfined_t:s0 key=(null) type=AVC_PATH msg=audit(1170328411.204:2435): path="/usr/lib/nemiver/plugins/dbgperspective/libdbgperspectiveplugin.so" Please also grep for "rpath" in the build log. IIRC not having hardcoded rpaths is a MUST item rpaths are OK as long as they're not for a standard library path such as /usr/lib or /usr/lib64. The SELinux issue is probably due to the way that the libdbgperspectiveplugin object is coded; what does it actually do? It may be worth pointing upstream to http://people.redhat.com/drepper/selinux-mem.html which may give some clue as to how to avoid the problem. In the short term, a fix would probably be to do: chcon -t textrel_shlib_t /usr/lib/nemiver/plugins/dbgperspective/libdbgperspectiveplugin.so Hello; and sorry for not getting back to this bug earlier. I was swamped with work and class stuff most of last week. :( The RPATH in the library is for the %_libdir/nemiver/plugins directory, so this seems intended and harmless. In regards to the execmod denials, I've marked it as textrel_shlib_t for the time being; and once I figure out what it is doing to cause the error (or, more importantly, how to reproduce it), I'll send a mail to the upstream devs about this. I've posted updated files to my webspace: SRPM: http://thecodergeek.com/downloads/fedora/SRPMs/nemiver-0.3.0-4.src.rpm Spec: http://thecodergeek.com/downloads/fedora/SPECs/nemiver.spec Using chcon in %install doesn't help because (a) file contexts aren't stored in RPM packages, and (b) rpm sets the file contexts based on the currently-running policy at package install time, which would override any file context set previously. The quick fix for this is to fix the file context in %post. The better fix for this is to request that /usr/lib(64)?/nemiver/plugins/dbgperspective/libdbgperspectiveplugin.so is set to context type textrel_shlib_t in the main selinux-policy package (request this on fedora-selinux-list or raise a bug on selinux-policy), so you don't need to adjust the context type in your own package. The bext fix is of course to get the underlying memory access fixed upstream as mentioned before. Hopefully they will have a clue what is going on. (In reply to comment #18) > The better fix for this is to request that > /usr/lib(64)?/nemiver/plugins/dbgperspective/libdbgperspectiveplugin.so is set > to context type textrel_shlib_t in the main selinux-policy package (request this > on fedora-selinux-list or raise a bug on selinux-policy), so you don't need to > adjust the context type in your own package. +1 this is always preferable if .so really needs such context I've posted bug #228628 for the selinux-policy modification, and moved the chcon call to the %post scriptlet. (Thanks for the information regarding RPM context management.) Updated files are on my webspace: Spec: http://thecodergeek.com/downloads/fedora/SPECs/nemiver.spec SRPM: http://thecodergeek.com/downloads/fedora/SRPMs/nemiver-0.3.0-5.src.rpm maybe too many % in: chcon -t textrel_shlib_t \ %%{_libdir}/%{name}/plugins/dbgperspective/libdbgperspectiveplugin.so ? What is necessary to reproduce the selinux error? I don't get any such output or audit messages when starting nemiver. The context of the DSO file is the default system_u:object_r:lib_t I'd also follow the chcon command with the usual "&> /dev/null || :" to avoid issues on systems with SELinux not installed or running a policy not containing the textrel_shlib_ context type. Hmm. I've been playing with it for a while with all of the memory protection features of the targeted policy active; and I can't seem to reproduce the execmod denial you are seeing. Gianluca: What policy/settings and versions are you using? As eu-findtextrel says it doesn't need text relocations either, I'm very tempted to simply remove that chcon invocation entirely, as it doesn't appear to need it. Unfortunately, I can't fix/workaround a bug that I can't find. :o This(In reply to comment #24) > > Gianluca: What policy/settings and versions are you using? The problem was seen in a (supposedly up-to-date) FC5 box, running selinux-policy-2.3.7-2.fc5 [root@molzilla ~]# sestatus SELinux status: enabled SELinuxfs mount: /selinux Current mode: enforcing Mode from config file: enforcing Policy version: 21 Policy from config file: targeted > > As eu-findtextrel says it doesn't need text relocations either, I'm very tempted > to simply remove that chcon invocation entirely, as it doesn't appear to need it. +1. I could always open another BZ if I later find a problem with the published bits > > Unfortunately, I can't fix/workaround a bug that I can't find. :o sure :) thanks a lot for your help Note that the context change went into the Rawhide selinux-policy package earlier this week (selinux-policy-2.5.3-2.fc7). If the textrel_shlib_t context isn't needed, this change should be reverted. (In reply to comment #26) > Note that the context change went into the Rawhide selinux-policy package > earlier this week (selinux-policy-2.5.3-2.fc7). If the textrel_shlib_t context > isn't needed, this change should be reverted. > > I've posted a modification to bug 228628, asking that the change be reverted. I'm going to be gone most of today, so I'll post an updated spec/SRPM first thing tomorrow. Thanks! Ok. I edited the spec to not perform the labeling and rebuilt for FC6: I can confirm the selinux problem is definetely not present here. Good morning! :] I've uploaded 0.3.0-6, which removes the chcon invocation. Spec: http://thecodergeek.com/downloads/fedora/SPECs/nemiver.spec SRPM: http://thecodergeek.com/downloads/fedora/SRPMs/nemiver-0.3.0-6.src.rpm Thanks. ping? (In reply to comment #30) > ping? Pong. :) I'd really appreciate a review of this, as I'd love to get it into the Package Collection in time for Fedora 7... Ok, since I am probably going to be a user of this, I'll try to review the package. Please note this is my first official review, so anyone watching over my shoulders will be really welcome... Here is the review: MUST ITEMS: + Package name is valid + spec file name match base package name + spec file is written in American English + spec file is legible (very!) + Sources matches upstream (289d46e97c125b95fdc5de9dd9736d7c nemiver-0.3.0.tar.bz2) + License is acceptable (GPL) + License is included in the package + Package builds (in mock) for FC6 i386 + BuildRequires list looks sane + Macros are consistently used + Use %find_lang macro for locale files + use ldconfig for installed .so files + package is not relocatable + headers and .so files correctly packaged in -devel + -devel correctly Requires base package + .la files excluded from package + .desktop file present and correctly installed rpmlint output: /var/lib/mock/fedora-6-i386-core/result/nemiver-0.3.0-6.fc6.i386.rpm W: nemiver non-conffile-in-etc /etc/gconf/schemas/nemiver-workbench.schemas W: nemiver non-conffile-in-etc /etc/gconf/schemas/nemiver-dbgperspective.schemas This are usually to be ignored (and probably rpmlint should as well...) /var/lib/mock/fedora-6-i386-core/result/nemiver-0.3.0-6.fc6.src.rpm W: nemiver mixed-use-of-spaces-and-tabs (spaces: line 167, tab: line 1) line 167 is in the changelog section, so I think we can ignore this Just a couple questions: is the "touch" trick for forcing the icon cache update the usual/preferred/best way to do that? are you going to remove COPYING from the -devel subpackage as suggested in comment #2? ping. I am going to approve this as soon as you can provide some info about my remarks Pong :) Hi, Gianluca and Thanks for the review! >(In reply to comment #33) > Just a couple questions: > is the "touch" trick for forcing the icon cache update the > usual/preferred/best way to do that? It is, according to the Packaging/ScriptletSnippets page on the wiki. > are you going to remove COPYING from the -devel subpackage as suggested in > comment #2? No. I'm of the opinon that such license texts should be included in development subpackages to make the license explicitly clear (and not have to backreference that of the original SRPM's main package). I know of no guideline for or against thish, though; and it seems more sensible to me tokeep it in there. (In reply to comment #35) > >(In reply to comment #33) > > Just a couple questions: > > is the "touch" trick for forcing the icon cache update the > > usual/preferred/best way to do that? > > It is, according to the Packaging/ScriptletSnippets page on the wiki. Ok. seems I missed it until now... > > > are you going to remove COPYING from the -devel subpackage as suggested in > > comment #2? > No. I'm of the opinon that such license texts should be included in development > subpackages to make the license explicitly clear (and not have to backreference > that of the original SRPM's main package). I know of no guideline for or against > thish, though; and it seems more sensible to me tokeep it in there. > I have no strong opinion against this either (though I did not notice other packages doing the same). In the lack of an official guideline about this and given there was more than enough time for further comments the package is APPROVED Great; thanks for your review & comments! New Package CVS Request ======================= Package Name: nemiver Short Description: A C/C++ Debugger for GNOME - point, click, debug! Owners: peter Branches: FC-6 devel Thanks for the branching, Josh. Built successfully for devel; comps entry done too. Package Change Request ====================== Package Name: nemiver New Branches: el6 Owners: dodji I am requesting an EPEL 6 branch for the Nemiver package. I have also requested an EPEL 6 branch for its dependencies ghex and gtksourceviewmm as they are the ones missing from EPEL 6 at the moment. Git done (by process-git-requests). |