After 'emerge -1v python johntheripper' only john has xattr marking: # paxctl-ng -v /usr/bin/python2.7 /usr/sbin/john /usr/bin/python2.7: PT_PAX : -em-- XATTR_PAX: not found /usr/sbin/john: PT_PAX : -emr- XATTR_PAX: -emr- sys-boot/grub-2.00-r2 has same problem. johntheripper does pax-mark in src_install, but both python and grub do in src_compile. Seems like wrong usage of cp or install in sys-apps/portage-2.1.11.55 and/or eclasses. cp does not copy xattr, but cp -a does. # mount|grep tmpfs|grep /tmp none on /tmp type tmpfs (rw,relatime,noexec,nodev,nosuid,size=2048M,nr_inodes=1M) none on /var/tmp/portage type tmpfs (rw,relatime,nodev,nosuid,size=16G) kernel 3.8.5-hardened $ grep XATTR /boot/config-3.8.5-hardened CONFIG_REISERFS_FS_XATTR=y CONFIG_TMPFS_XATTR=y CONFIG_SQUASHFS_XATTR=y CONFIG_F2FS_FS_XATTR=y CONFIG_PAX_XATTR_PAX_FLAGS=y
We've narrowed the problem down to coreutils' install which does not preserve xattrs when copying. (Neither does copy unless invoked with -a). So any xattr pax markings before make install are lost, after they are preserved.
I have exactly the same problem. I installed Hardened Gentoo with pure XATTR_PAX, and USE="-ptpax xtpax xattr", but the extended attributes are not copied for grub2 and python (for the moment I found only these two packages with this problem). # ebuild grub-2.00-r2.ebuild compile Sets the PAX markings, but they get lost after install. Copying grub-probe for example with cp --preserve=xattr retains the markings. The same happens with python as well. Apart from reverting to PT_PAX, are there any workarounds for the moment?
(In reply to comment #2) > > Apart from reverting to PT_PAX, are there any workarounds for the moment? You can mark *after* installation. Right now I'm working on two solutions. The best is to make sure install preserves xattrs. Its an easy patch actually but I'm not sure if it will be accepted. The other will be to search through the tree and find places where pax-mark is run before install and open bugs. Thank you for your patience! I'm moving slowly because I want to catch all the problems in round 2. We have the luxury of PT_PAX fallback.
Okay, I've tried to generate some interest downstream (bug #470660) and upstream (http://lists.gnu.org/archive/html/coreutils/2013-06/msg00034.html) in terms of getting coretutil's install to presever at least the "user.pax.flags" namespace and that effort seems to be stalling. Here are our options for solving this problem: 1) Change the default behavior of install. vapier doesn't think this is wise unless upstream okays it (bug #470660) and on second thought neither do I. with PT_PAX we had to custom patch binutils, now with XATTR_PAX we would be patching coreutils. The latter is safer than the former, but let's assume install stays as is, how do we proceed? 2) The path of least resistance is to identify all ebuilds in the tree that do pax-marking and if they *must* do pax-marking before install (because eg, they need it for src_test) then we pax-mark twice. We mark again *after* install. 3) We add intelligence to portage where the install function is wrapped as follows: install src dst parse for user.pax.lfags getfatter -d src setfatter -n user.pax.flags -v flags dst when the file systems can handle xattr and USE="xattr". 4) We add intelligence to pax-utils.eclass so that pax-markings can be done anywhere, an array of the marked files is kept and then at the end of src_install() or even at pkg_postinst() we do the markings. I'm cc-ing zac and mike for opinions since they're portage and coreutils' maintainers. If 2 seems the way to go, I'll identify the packages, open bugs and push out the fixes. Related question: is there a way for a eclass function like pax-mark in pax-utils.eclass to know what phase of ebuild it was called from. So we can add a warning like ... pax-mark was called from src_compile() phase which is before src_install(). Please invoke pax-mark after files are installed to ${D}.
(In reply to Anthony Basile from comment #4) > I'm cc-ing zac and mike for opinions since they're portage and coreutils' > maintainers. If 2 seems the way to go, I'll identify the packages, open > bugs and push out the fixes. Any way is fine with me. If we do 3, then it would probably be simplest to make it conditional on FEATURES=xattr alone, since with that feature enabled we can simply assume that the user has taken care of enabling xattrs wherever necessary. We could have it simply preserve all xattrs except those excluded by PORTAGE_XATTR_EXCLUDE (see bug 461868). > Related question: is there a way for a eclass function like pax-mark in > pax-utils.eclass to know what phase of ebuild it was called from. So we can > add a warning like ... You can use the EBUILD_PHASE variable which is defined by PMS: http://dev.gentoo.org/~ulm/pms/5/pms.html#x1-11800011.1
(In reply to Zac Medico from comment #5) > (In reply to Anthony Basile from comment #4) > > I'm cc-ing zac and mike for opinions since they're portage and coreutils' > > maintainers. If 2 seems the way to go, I'll identify the packages, open > > bugs and push out the fixes. > > Any way is fine with me. If we do 3, then it would probably be simplest to > make it conditional on FEATURES=xattr alone, since with that feature enabled > we can simply assume that the user has taken care of enabling xattrs > wherever necessary. We could have it simply preserve all xattrs except those > excluded by PORTAGE_XATTR_EXCLUDE (see bug 461868). > > > Related question: is there a way for a eclass function like pax-mark in > > pax-utils.eclass to know what phase of ebuild it was called from. So we can > > add a warning like ... > > You can use the EBUILD_PHASE variable which is defined by PMS: > > http://dev.gentoo.org/~ulm/pms/5/pms.html#x1-11800011.1 Okay, I've written a wrapper for install which preserves *only* user.* namespace. It would be placed at /usr/lib/portage/bin/ebuild-helpers/install I wrote it in bash because all the other ebuild-helpers are in bash. As a result it does depend on /bin/attr existing, ie whether or not sys-apps/attr is installed. Arfrever suggested writing it in python so no other deps are needed. I can rewrite it in python, but I'm not sure what the side effects might be? Comments?
Created attachment 351430 [details] user.* xattr preserving wrapper for install
Sorry, a bug wrt "install -txxx" vs "install -t xxx" in the previous patch. Here's the fix: --- /home/blueness/Desktop/install 2013-06-19 18:07:52.000000000 -0400 +++ install 2013-06-19 18:23:06.000000000 -0400 @@ -63,6 +63,7 @@ if [[ "$skip" == "yes" ]]; then skip="no" __target="$arg" + continue fi # Skip flags that we don't need to parse out source/destination @@ -90,10 +91,10 @@ # there is one or more, then we'll get the target directory on the next iteration. if [[ -z "${arg/-t/}" ]]; then skip="yes" - continue else __target="${arg#-t}" fi + continue fi __source_files+=( "$arg" )
Comment on attachment 351430 [details] user.* xattr preserving wrapper for install this looks like route 3, and it seems fine to me ... probably saner long term than the other options since things for the most part "just work" w/out requiring the devs to do any additional work general feedback: - use ${braces} with non-builtin vars - you can't use associative arrays w/out updating portage to depend on >=bash-4.0+ ... not a problem in itself, just noting because it's a new requirement - you don't need to namespace things like __foo since this isn't running in an environment we care about -- it's a bash script which means it gets control over everything, and when it exits, it's done - you need to save+restore the exit value of `install` - use a main() func to wrap the main logic ... makes reading easier
(In reply to SpanKY from comment #9) > Comment on attachment 351430 [details] > user.* xattr preserving wrapper for install > > this looks like route 3, and it seems fine to me ... probably saner long > term than the other options since things for the most part "just work" w/out > requiring the devs to do any additional work > > general feedback: > - use ${braces} with non-builtin vars > - you can't use associative arrays w/out updating portage to depend on > >=bash-4.0+ ... not a problem in itself, just noting because it's a new > requirement > - you don't need to namespace things like __foo since this isn't running in > an environment we care about -- it's a bash script which means it gets > control over everything, and when it exits, it's done > - you need to save+restore the exit value of `install` > - use a main() func to wrap the main logic ... makes reading easier Arfrever and zmedico have convinced me to rewrite this in python because of the bash 4 issue which I didn't know about. It could block this for a while. I did write a non-assoc array version (two arrays mapped by same index) but then there's also the extra dep on sys-apps/attr. I'll incorporate your comments in so far as they are relavent in python. Thanks :)
(In reply to Anthony Basile from comment #10) yes, getting people to write more scripts in python would be good. now if only our code stuck to some decent style guidelines ;). is it a matter of python not being your lang ? i one of us could flesh out something quickly and you could build on top of that ...
(In reply to SpanKY from comment #11) > (In reply to Anthony Basile from comment #10) > > yes, getting people to write more scripts in python would be good. now if > only our code stuck to some decent style guidelines ;). > > is it a matter of python not being your lang ? i one of us could flesh out > something quickly and you could build on top of that ... I think I'm better at python than bash, I just followed how most ebuild-helpers were written. I'll ask zmedico about style. I'll have it done by the end of today.
For python3, I was concerned about being able being unable to access sys.argv as raw bytes, but it turns out that it's possible to to do it with [arg.encode(sys.getfilesystemencoding(), 'surrogateescape') for arg in sys.argv]. So, even if arguments have an encoding that python isn't expecting, we should still be able to handle it properly.
Created attachment 351470 [details] python version of install wrapper Thanks zac for the optparse stuff. optparse >> getopt. The encoding should also work. Please review.
> The encoding should also work. Please review. Actually no, its broken. Doing [arg.encode(sys.getfilesystemencoding(), 'surrogateescape') for arg in sys.argv[1:]] before optparse breaks the option parsing. The following diff makes it work, but now I'm not sure about encoding: --- /home/blueness/Desktop/install.py 2013-06-20 09:37:24.000000000 -0400 +++ install 2013-06-20 10:22:14.000000000 -0400 @@ -137,10 +137,7 @@ try: if target_is_directory: for source in args: - abs_path = "{t}/{f}".format( - t=target, - f=os.path.basename(source) - ).encode(sys.getfilesystemencoding()) + abs_path = target + b'/' + os.path.basename(source) _copyxattr(source, abs_path) else: _copyxattr(args[0], target) @@ -159,5 +156,4 @@ return returncode if __name__ == "__main__": - encoded_args = [arg.encode(sys.getfilesystemencoding(), 'surrogateescape') for arg in sys.argv[1:]] - sys.exit(install_main(encoded_args)) + sys.exit(install_main(sys.argv[1:]))
Created attachment 351474 [details] python version of install wrapper I'm not sure the encoding is an issue. Here's the test I did with this attachement: yellow zzz # touch peñón yellow zzz # setfattr -n user.pax.flags -v "m" peñón yellow zzz # mkdir mydir yellow zzz # ./install.py -t mydir/ peñón yellow zzz # ls -al mydir/ total 8 drwxr-xr-x 2 root root 4096 Jun 20 10:42 . drwxr-xr-x 6 blueness blueness 4096 Jun 20 10:42 .. -rwxr-xr-x 1 root root 0 Jun 20 10:42 peñón yellow zzz # getfattr -d mydir/peñón # file: mydir/peñón user.pax.flags="m"
Comment on attachment 351474 [details] python version of install wrapper >import optparse use argparse instead as optparse is dead >from portage.exception import OperationNotSupported > >def parse_args(args): keep two lines between the "import" section and the func defs, and keep two lines between each func def. > parser = optparse.OptionParser(add_help_option=False) i don't think we need to list all flags. argparse has a parse_known_args() func we can use instead. that way you can list only the flags you care about while ignoring the rest. you'll then have to walk the returned list looking for the first arg that doesn't start with a - (or the first one after a --), but that's a lot easier. >def copy_xattrs(opts, args): there should be a docstring explaining things: def copy_xattrs(opts, args): """Copy the extended attributes (if possible) from the source to the dest Args: opts: ... args: ... """ > target = args.pop() this ends up modifying the list passed in. instead do: target, args = args[0], args[1:] > abs_path = target + '/' + os.path.basename(source) abs_path = os.path.join(target, os.path.basename(source)) > _copyxattr(source, abs_path) > else: > _copyxattr(args[0], target) > return os.EX_OK > > except OperationNotSupported: > return os.EX_OSERR i think you should just let the exception bubble up rather than catching it here and normalizing it into return values >def install_main(args): call it main(argv) > opts, pargs = parse_args(args) opts, args = parse_args(args) > cmdline = [ '/usr/bin/install' ] hmm, do we not have a Which() func ? should be easy to write one: def Which(filename, path=None, all=False): if path is None: path = os.environ.get('PATH', '') ret = [] for p in path.split(':'): p = os.path.join(p, filename) if os.access(p, os.X_OK): if all: ret.append(p) else: return p if all: return ret else: return None then you can use Which('install', all=True) and throw away the first result (since it'll be this script) > cmdline.extend(args) personally i like this better, but it's the same: cmdline += args
(In reply to SpanKY from comment #17) > Comment on attachment 351474 [details] > python version of install wrapper > > >import optparse > > use argparse instead as optparse is dead If we want to support python2.6, then we would need to depend on dev-python/argparse. Is it worth it to pull in that dependency, even though optparse works well enough and is included with python2.6? > i don't think we need to list all flags. argparse has a parse_known_args() > func we can use instead. that way you can list only the flags you care > about while ignoring the rest. you'll then have to walk the returned list > looking for the first arg that doesn't start with a - (or the first one > after a --), but that's a lot easier. I don't think optparse has an equivalent of parse_known_args. Listing all of the flags explicitly is somewhat annoying, but it seems manageable. > > > > except OperationNotSupported: > > return os.EX_OSERR > > i think you should just let the exception bubble up rather than catching it > here and normalizing it into return values Sounds good to me. > hmm, do we not have a Which() func ? should be easy to write one: We have portage.process.find_binary(), but it doesn't quite fit this use case. I 'd like to use something like your Which() func, and optimize it for our use case. The search algorithm should be equivalent to the one that we use in bin/ebuild-helpers/unprivileged/chown.
(In reply to Zac Medico from comment #18) > (In reply to SpanKY from comment #17) > > > > > > except OperationNotSupported: > > > return os.EX_OSERR > > > > i think you should just let the exception bubble up rather than catching it > > here and normalizing it into return values > > Sounds good to me. > To what extend do we want the wrapper to emulate the real install which would emit an exit value? You guys know the portage environment better so I'll code it that way.
(In reply to SpanKY from comment #17) > do we not have a Which() func ? http://docs.python.org/3.3/library/shutil.html#shutil.which
(In reply to Zac Medico from comment #18) i think we should start moving to argparse. i thought that was the point of virtual/python-argparse ? if the version is too old and doesn't include support, it'll pull in the dedicated dev-python/argparse package.
(In reply to Anthony Basile from comment #19) the python install wrapper should return the exit code that `install` itself returned with. that leaves the case when xattr fails (i don't mean unsupported which we'd silently swallow). i'm not sure throwing an exception or exiting a random value would be foolproof as some packages are dumb and do "-install" in their Makefile. maybe we should have it communicate `die` to emerge since this really should be an unusual use case we'd want to know about and update the code to handle ?
(In reply to Arfrever Frehtes Taifersar Arahesis from comment #20) ok, so it's new to python 3.3 which is why i've never seen it. effectively makes it useless to us ;). only input that might be useful there is if we decided to stick to that prototype and implemented the same API.
(In reply to SpanKY from comment #21) Well, virtual/python-argparse uses EAPI 5. So that breaks the upgrade path from <python-2.6, since people won't be able to install python-2.6 + argparse without an EAPI 5 package manager. So, we may as well drop support for python-2.6. We can do that.
(In reply to Zac Medico from comment #24) maybe we need to finally invest in a script that takes care of bootstrapping up from old systems. keep that package as EAPI=0 so anyone can install it. otherwise i don't really have a problem with dropping python-2.6 since it's a dead branch. python-2.7 is viable.
(In reply to SpanKY from comment #25) > maybe we need to finally invest in a script that takes care of bootstrapping > up from old systems. keep that package as EAPI=0 so anyone can install it. That should be covered by the procedure suggested in bug 457148. > otherwise i don't really have a problem with dropping python-2.6 since it's > a dead branch. python-2.7 is viable. Yeah, that's fine with me.
(In reply to Zac Medico from comment #24) > (In reply to SpanKY from comment #21) > Well, virtual/python-argparse uses EAPI 5. So that breaks the upgrade path > from <python-2.6, since people won't be able to install python-2.6 + > argparse without an EAPI 5 package manager. So, we may as well drop support > for python-2.6. We can do that. What about checking `if sys.version_info < (2, 7):` and falling back on optparse for 2.6 while using argparse above? No need for python-argparse for 2.6 and blow.
(In reply to Anthony Basile from comment #27) > What about checking `if sys.version_info < (2, 7):` and falling back on > optparse for 2.6 while using argparse above? No need for python-argparse > for 2.6 and blow. Sounds good for me.
(In reply to Anthony Basile from comment #27) that wouldn't help if you rely on the proposed parse_known_args() func (which i think you should). the API is also slightly different when it comes to adding known options between optparse and argparse.
(In reply to SpanKY from comment #29) > (In reply to Anthony Basile from comment #27) > > that wouldn't help if you rely on the proposed parse_known_args() func > (which i think you should). the API is also slightly different when it > comes to adding known options between optparse and argparse. if sys.version_info < (2, 7): opts, pargs = parse_args_compat(args) else: opts, pargs = parse_args(args) Have two functions. But, that point aside, there is another issue with argparse. When using optparse, the following install aaa -d bbb is parsed as { 'directory': True, ... } and ['aaa', 'bbb'] as the list of directories to be created. While argparse with parse_known_args() prases this { 'directory': True, ... }, ['aaa'] as the list of directories to be created, and ['bbb'] as the list of unknown parameters. The only way I see around this is still to list all the parameters, force prefix_chars='-' and then grab the list of files/directories from the unknown paramter list.
(In reply to SpanKY from comment #29) > (In reply to Anthony Basile from comment #27) > > that wouldn't help if you rely on the proposed parse_known_args() func > (which i think you should). the API is also slightly different when it > comes to adding known options between optparse and argparse. I should add that obviously optparse's behavior follows that of real install. There's more: 1) install aaa -d bbb -- ccc -t optparse: {'directory': True, ... } files = ['aaa', 'bbb', 'ccc', '-t'] argparse: {'directory': True, ... } known = ['aaa'] unknown = ['bbb', '--', 'ccc', '-t'] 2) install aaa -t xxx bbb optparse: { 'target_directory': 'xxx', ... } files = ['aaa', 'bbb'] argparse: { target_directory='xxx', ... } known = ['aaa'] unknonw = ['bbb'] Here's my function for parsing with argparse: def parse_args(args): """ Parse the command line arguments Args: args: list of the white space delimited command line parameters Returns: Tuple of a dictionary of parsed options and a list of files """ parser = argparse.ArgumentParser(add_help=False, prefix_chars='-') parser.add_argument( "files", nargs='+' ) parser.add_argument( "--directory", "-d", action="store_true", dest="directory" ) parser.add_argument( "--target-directory", "-t", action="store", dest="target_directory" ) parsed_args = parser.parse_known_args() print(parsed_args,"\n") parsed_args = parsed_args[0] # discard unknown options files = parsed_args.files # strip out the files from the Namespace del parsed_args.files return (vars(parsed_args), files) # return Namespace as dictionary
Created attachment 351600 [details] python version of install wrapper Okay, this addresses all the above issues (I hope). There was no way to implement argparse without listing all the parameters. The alternative was some rather ugly parsing of the unknown parameter list. I did not go with both optparse and argparse. Its easy to add both functions with a check on the version, but then we'd have two really long lists of options. It would help to move this forward since it would be backwards compat with 2.6. As for the exception handling, I'm both generating the exception code and doing a traceback.print_exc() to print the exception out. zmedico: can you take a look at the encoding issue because you get that better than I do.
(In reply to Anthony Basile from comment #32) > Created attachment 351600 [details] > python version of install wrapper Thanks, it's in git now: http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=0ca18b45ca14e94ba18a29c8cf897d82ee9eabfd I write a patch to integrate it with FEATURES=xattr, and that will complete the fix.
I made few other changes, and this is commit integrates the wrapper with FEATURE=xattr and PORTAGE_XATTR_EXCLUDE: http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=8a78f4d99606a2aa15ee0c049449d98ec27a2c41 It's released in portage-2.1.12.9 and 2.2.0_alpha184.
(In reply to Zac Medico from comment #34) > I made few other changes, and this is commit integrates the wrapper with > FEATURE=xattr and PORTAGE_XATTR_EXCLUDE: > > http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit; > h=8a78f4d99606a2aa15ee0c049449d98ec27a2c41 > > It's released in portage-2.1.12.9 and 2.2.0_alpha184. Zac, the wrapper has been in production for a while now, and it is very slow compared to the compiled version of install. Is there any way we can replace install.py with a compiled version of install which we patch?
Let me give a feedback on what I experience right now. Both PT and XT markings are enabled on my system in make.conf and the kernel. Until recently, the installed bineries had only PT marking and there were no XT fields carried along. Lately I see some changes. Now if a binary gets marked for mprotect during emerge, the file installed will have proper XT markings, but the PT marking will be the default "e". So there are both "e" and "m" for XT, but only "e" for PT. Or for another example: cc1 and cc1plus after a gcc upgrade had "e" and "r" for XT, but only "e" for PT. And the kernel has support for both, and it seems to perfer PT over XT. So I had to manually paxctl-ng some binaries after install. I don't know if it is intentional. You may say, that if I disable PT in kernel, my system will behave. But I think for the transitional period the binaries should reflect a consistent marking for both PT and XT. If possible.
(In reply to Attila Tóth from comment #36) > Let me give a feedback on what I experience right now. > Both PT and XT markings are enabled on my system in make.conf and the kernel. > Until recently, the installed bineries had only PT marking and there were no > XT fields carried along. > Lately I see some changes. Now if a binary gets marked for mprotect during > emerge, the file installed will have proper XT markings, but the PT marking > will be the default "e". So there are both "e" and "m" for XT, but only "e" > for PT. Or for another example: cc1 and cc1plus after a gcc upgrade had "e" > and "r" for XT, but only "e" for PT. And the kernel has support for both, > and it seems to perfer PT over XT. So I had to manually paxctl-ng some > binaries after install. > > I don't know if it is intentional. You may say, that if I disable PT in > kernel, my system will behave. But I think for the transitional period the > binaries should reflect a consistent marking for both PT and XT. If possible. Yes, if you disable PT pax your kernel will behave. The only problme right now is the slownless of install.py. If that is solved I believe a full XT pax systems could be considered stable.
(In reply to Anthony Basile from comment #35) > (In reply to Zac Medico from comment #34) > > I made few other changes, and this is commit integrates the wrapper with > > FEATURE=xattr and PORTAGE_XATTR_EXCLUDE: > > > > http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit; > > h=8a78f4d99606a2aa15ee0c049449d98ec27a2c41 > > > > It's released in portage-2.1.12.9 and 2.2.0_alpha184. > > Zac, the wrapper has been in production for a while now, and it is very slow > compared to the compiled version of install. Is there any way we can > replace install.py with a compiled version of install which we patch? Okay I've rewritten the wrapper in C. When emerging something with lots of files, like www-apps/moodle, the python wrapper takes about 30 mins (yes!) while the C wrapper only takes 3 mins. To test, remove the python script at /usr/lib/portage/bin/install.py and replace the bash script at /usr/lib/portage/bin/ebuild-helpers/xattr/install with the compiled C wrapper which I'll upload in my next post.
Created attachment 366866 [details] c version of the wrapper Note: I'm not 100% sure how the $PATH should look in the portage environment. I just hard coded the search path for coreutil's "install" as char *path = strdup("/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"); (yes the strdup is correct there). However, it would be better to have something like: char *path, *env_path = getenv("PATH"); // The string of the $PATH to search // We strdup the path string because we're going to play // with it below using the pointer 'dir', and then free it if (env_path == NULL) path = strdup("/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"); else path = strdup(env_path); to grab the env $PATH. However, the wrapper finds itself and wraps itself ad-infinitum that way so obviously we don't want to use the raw $PATH. What should go there though?
(In reply to Anthony Basile from comment #38) have you tried writing a python version w/out using portage modules ? that'd be easier to maintain than a C wrapper.
(In reply to SpanKY from comment #40) > (In reply to Anthony Basile from comment #38) > > have you tried writing a python version w/out using portage modules ? > that'd be easier to maintain than a C wrapper. I applied the following quicky patch to the current python wrapper. This time moodle took 17 mins. That's a factor of 2 improvement but no where near factor 10+ with the C wrapper. Unfortunate, a python wrapper is nicer. But no joy. --- install.py.orig 2013-12-27 10:00:26.552594964 -0500 +++ install.py.orig.noportage 2014-01-04 11:12:57.157397743 -0500 @@ -7,11 +7,13 @@ import sys import subprocess import traceback +import optparse, argparse +import xattr -import portage -from portage.util._argparse import ArgumentParser -from portage.util.movefile import _copyxattr -from portage.exception import OperationNotSupported +#import portage +#from portage.util._argparse import ArgumentParser +#from portage.util.movefile import _copyxattr +#from portage.exception import OperationNotSupported # Change back to original cwd _after_ all imports (bug #469338). os.chdir(os.environ["__PORTAGE_HELPER_CWD"]) @@ -24,7 +26,7 @@ Returns: tuple of the Namespace of parsed options, and a list of order parameters """ - parser = ArgumentParser(add_help=False) + parser = argparse.ArgumentParser(add_help=False) parser.add_argument( "-b", @@ -151,6 +153,11 @@ return (opts, files) +def my_copyxattr(source, target): + attrs = xattr.list(source) + for attr in attrs: + xattr.set(target, attr, xattr.get(source, attr)) + def copy_xattrs(opts, files): """ @@ -177,9 +184,9 @@ if target_is_directory: for s in source: abs_path = os.path.join(target, os.path.basename(s)) - _copyxattr(s, abs_path, exclude=exclude) + my_copyxattr(s, abs_path) else: - _copyxattr(source[0], target, exclude=exclude) + my_copyxattr(source[0], target) return os.EX_OK except OperationNotSupported:
1. C wrapper. `time emerge moodle` gives real 2m4.268s user 0m59.021s sys 0m26.233s 2. install.py as is currently in tree: real 28m26.682s user 23m44.901s sys 4m17.701s 3. install.py without the portage modules imported: real 16m47.290s user 13m33.593s sys 2m29.487s 4. BTW, there is a small memory leak in the C wrapper I just noticed. We need a free(value); after line 66. I'll add it when/if we adopt it.
(In reply to Anthony Basile from comment #42) thanks, that's the kind of data we need to make informed decisions ;)
Comment on attachment 366866 [details] c version of the wrapper file needs a good bit of clean up: - use /* comments */ - use tabs for indentation - add a copyright/license comment block to the top - fix warnings when built with `gcc -Wall -O3` >void >copyxattr(const char *source, const char *target) mark all funcs static where possible > for (i=0; i<len; i++) lines like this need proper spacing: for (i = 0; i < len; ++i) > /* FIXME: I don't know what path portage should search so I'll hard code it for now > > char *path, *env_path = getenv("PATH"); // The string of the $PATH to search > > // We strdup the path string because we're going to play > // with it below using the pointer 'dir', and then free it > if (env_path == NULL) > path = strdup("/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"); > else > path = strdup(env_path); > */ > > // FIXME: This should be searching $PATH or somethin --- zmedico help! > char *path = strdup("/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"); just search $PATH, but ignore the result that matches dirname(argv[0]). use strtok_r to split on : and the rest should be easy. > while (1) > { cuddle that brace up: while (1) { > static struct option long_options[] = > { set opterr=0 to silence warnings from the library, and then ignore '?' yourself rather than hard code all possible flags. only list options you care about here. > while (optind < argc) > asprintf (&cmd_line, "%s %s", cmd_line, argv[optind++]); what's the point of cmd_line ? you never use it. > if (fork()) { need to handle -1 too switch (fork()) { case -1: abort(); case 0: /* child */ ... default: /* parent */ ...
Created attachment 367452 [details] c version of the wrapper (In reply to SpanKY from comment #44) > lines like this need proper spacing: > for (i = 0; i < len; ++i) Do we have a C style guide that we use in Gentoo? > set opterr=0 to silence warnings from the library, and then ignore '?' > yourself rather than hard code all possible flags. only list options you > care about here. Yeah since install itself will warn about incorrect arguments, there's no need for this. The code is a lot nicer. > > > while (optind < argc) > > asprintf (&cmd_line, "%s %s", cmd_line, argv[optind++]); > > what's the point of cmd_line ? you never use it. It was for debugging and I forgot to remove it. I've put the code in the proj/elfix repo for safe keeping: http://git.overlays.gentoo.org/gitweb/?p=proj/elfix.git;a=tree;f=misc/install.wrapper.c;h=c6ebbd275c023e42926c294ca3b6096fcd794c56;hb=c396e12b234cc54748f9b05a4c35f0c304597830
Comment on attachment 367452 [details] c version of the wrapper i don't think we have a style guide. would be easy to write one and keep it in the portage tree. basically, just think LKML style :p. >#define _GNU_SOURCE i think the prefix peeps are going to complain about this (and the asprintf usage). you only use it to concat paths, so might be easy to write your own helper ... char *path_join(const char *p1, const char *p2) { size_t l1 = strlen(p1); size_t l2 = strlen(p2); char *ret = malloc(l1 + l2 + 2); memcpy(ret, p1, l1); ret[l1] = '/'; memcpy(ret + l1 + 1, p2, l2); ret[l1 + l2 + 1] = '\0'; return ret; } >#include <getopt.h> > >#include <unistd.h> omit the blank lines between includes >#include <sys/types.h> >#include <sys/wait.h> >#include <sys/stat.h> >#include <sys/xattr.h> > >static void >copyxattr(const char *source, const char *target) >{ > int i, j; > char *exclude, *portage_xattr_exclude; /* strings of excluded xattr names */ > int len; /* length of the string of excluded xattr names */ use size_t since you hold strlen() > ssize_t lsize, xsize; /* size in bytes of the list of xattrs and the values */ > char *lxattr, *value; /* string of xattr names and the values */ > > lsize = listxattr(source, 0, 0); this file seems to suffer from a lot of error checking. i would add some x* helpers that die() when an error is hit. #include <err.h> ... static ssize_t xlistxattr(const char *path, char *list, size_t size) { ssize_t ret = listxattr(path, list, size); if (ret < 0) err(1, "listxattr() failed"); return ret; } > lxattr = (char *)malloc(lsize); no need for the cast. malloc() returns a void* and C handles that sanely. > /* We convert exclude[] to an array of concatenated null */ > /* terminated strings. lxattr[] is already such an array. */ use /* .... * .... */ for multiline comments > for (i = 0; i < len; i++) i will have to be a size_t too > i = 0 ; no spaces before the ;. this shows up a few times in this file. > while(1) { needs a space before the ( > while (lxattr[i++] == 0); put a continue; there so it's obvious what's going on. the ; at the end is easy to miss. > skip: indent labels with just one space > > return; >} that return statement is pointless, so punt it >static char * >which(char *mydir) mark it const >{ > char *mycandir = realpath(mydir, NULL); /* canonical value of argv[0]'s dirname */ > char *path, *env_path = getenv("PATH"); /* full $PATH string */ > > /* If we don't have $PATH in our environment, then pick a sane path. */ > if (env_path == NULL) > path = strdup("/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"); use confstr(_CS_PATH) instead > /* You don't get a canonical path for ~/bin etc., so skip. */ > if (!candir) > goto skip; PATH would not contain a literal "~/". it gets expanded before being placed into the $PATH env var. > if ( !fnmatch(mycandir, candir, 0) ) no spaces there: if (!fnmatch(...)) > if (asprintf(&file,"%s/%s", candir, "install") == -1) > abort(); use err() instead of abort() > int option_index = 0; no need to assign to 0 here > switch (c) { > case 0 : > case '?': spacing in that first label looks weird to me > break; be nice to add a comment here that you're skipping flags you don't care about > if (asprintf(&target, "%s", optarg) == -1) iirc, the string optarg points to stays viable, so you could just take it: target = optarg; looking in the rest of the code below, you never modify this pointer, so don't waste time duplicating it > abort(); > break; > > default: > abort(); > } > } > > first = optind; > last = argc-1; spaces around that - > install = which(dirname(argv[0])); > execve(install, argv, envp); > free(install); no need to waste time on the free. the child is going to exit and the kernel will take care of this for us. exit(execv(install, argv)); > wait(&status); shouldn't you check the exit status ? if the install failed, we shouldn't go trying to copy/restore things. we should exit the same way the child did. > if (!opts_target_directory) { > target = strdup(argv[last]); no need to dupe it. just assign target=argv[last]. > target_is_directory = s.st_mode & S_IFDIR; this is what S_ISDIR() is for (in fact you already use it below)
(In reply to SpanKY from comment #46) > Comment on attachment 367452 [details] > c version of the wrapper Okay I've incorporated all these changes but before I resubmit, some points: > char *path_join(const char *p1, const char *p2) > { > size_t l1 = strlen(p1); > size_t l2 = strlen(p2); > char *ret = malloc(l1 + l2 + 2); > memcpy(ret, p1, l1); > ret[l1] = '/'; > memcpy(ret + l1 + 1, p2, l2); > ret[l1 + l2 + 1] = '\0'; > return ret; > } If we were checking the return code for asprintf, should we do the same with malloc? I use it in 4 places and could write a helper. > > > skip: > > indent labels with just one space Okay, one space instead of one tab. FYI, I looked at pax-util's style for reference and there we find no indentation for labels. > > /* You don't get a canonical path for ~/bin etc., so skip. */ > > if (!candir) > > goto skip; > > PATH would not contain a literal "~/". it gets expanded before being placed > into the $PATH env var. I'm not sure about this. Here's a little test program: #include <stdio.h> #include <stdlib.h> #include <string.h> int main(int argc, char *argv[]) { char *savedptr; char *path = getenv("PATH"); char *token = strtok_r(path, ":", &savedptr); while (token) { printf("token = %s\n", token); token = strtok_r(NULL, ":", &savedptr); } } and here's the result token = ~/bin token = /usr/local/bin token = /usr/bin token = /bin token = /opt/bin token = /usr/x86_64-pc-linux-gnu/gcc-bin/4.8.2 token = /usr/lib64/subversion/bin token = /usr/games/bin Here $PATH was equal to ~/bin:/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/4.8.2:/usr/lib64/subversion/bin:/usr/games/bin > use err() instead of abort() Everywhere? I think I still want abort() if fork() returns -1, and in the which() function, for the SIGABRT. Everywhere else I can use err(). > > iirc, the string optarg points to stays viable, so you could just take it: > target = optarg; > That leads to a seg fault. So does removing strdup() from "target = strdup(argv[last])". To demonstrate that "target = optarg;" leads to a segfault, do ./install -t <target directory> <source file> To demonstrate that "target = argv[last];" leads to a invalid ponter, do ./install <source file> <target file> (I can't gdb effectively on my hardened box, so I didn't push the analysis through. I could fire up my vanilla vm and see if we really need to figure this one out.) > > free(install); > > no need to waste time on the free. the child is going to exit and the > kernel will take care of this for us I know but it bothers me aesthetically ;) I put a comment in there so the reader knows we have considered what the fate of that memory is. > > wait(&status); > > shouldn't you check the exit status ? if the install failed, we shouldn't > go trying to copy/restore things. we should exit the same way the child did. The return code is not so easy to interpret and I don't see it documented anywhere. I'm going to read install's code now to see what's going on. Here are some examples: touch a setfattr -n user.pax.flags -v "mr" a setfattr -n user.foo -v "bar" a mkdir d getfattr -d a # file: a user.foo="bar" user.pax.flags="mr" install a b ; echo $? 0 # As expected install a d ; echo $? 0 # As expected rm d/a # Let's do it again, but wrong this time rm: remove regular empty file ‘d/a’? y install d a d ; echo $? # This is meaningless ... install the directory d and file a to d ??? install: omitting directory ‘d’ # In cases like this the extra directory is ignored 1 # and we do get an err code but .... ls d/ # the file *is* copied to d !!! a In light of the fact that install doesn't simply fail if the command line syntax is wrong, I opted for ignoring the return and just looking to see if the file(s) was installed at the target and then copy the xattrs. Basically non-zero exit code from vanilla install does NOT imply that the file was not installed, ie that it failed. Blech.
(In reply to Anthony Basile from comment #47) btw, you should also test these: ./install-xattr ./install-xattr --help seems to misbehave in both cases for me ... > If we were checking the return code for asprintf, should we do the same with > malloc? I use it in 4 places and could write a helper. yes, there should be a xmalloc() helper. i meant that when i said "write some helpers". any func that should have its return value checked and you plan on just dying when it fails should have an x* helper. > Okay, one space instead of one tab. FYI, I looked at pax-util's style for > reference and there we find no indentation for labels. i've been getting away from labels in general. the ones that are left i either convert to use one space or delete. the reason we want one space is that `diff -p` get's confused otherwise. > > > /* You don't get a canonical path for ~/bin etc., so skip. */ > > > if (!candir) > > > goto skip; > > > > PATH would not contain a literal "~/". it gets expanded before being placed > > into the $PATH env var. > > I'm not sure about this. Here's a little test program: i think you misinterpreted me. if you do `export PATH="~/bin:$PATH"`, then your ~/bin is not going to get searched. that is a bug in your shell init if you think it is going to work. only the shell does ~/ expansion, and it has to do it before setting $PATH. C libraries do not treat "~" specially. instead they'll look for a literal "~/bin" dir. my point being that your comment is misleading and i would just rephrase it to "ignore invalid paths that cannot be cannoicalized". > > use err() instead of abort() > > Everywhere? I think I still want abort() if fork() returns -1, and in the > which() function, for the SIGABRT. Everywhere else I can use err(). abort produces no error message and simply kills itself with SIGABRT. err() produces a friendly error message and exits with a non-zero value you provide. so yes, i can't think of any place in this code you should be using abort(). > > iirc, the string optarg points to stays viable, so you could just take it: > > target = optarg; > > That leads to a seg fault. works fine for me. did you also delete the free(target) ? > > > wait(&status); > > > > shouldn't you check the exit status ? if the install failed, we shouldn't > > go trying to copy/restore things. we should exit the same way the child did. > > The return code is not so easy to interpret and I don't see it documented > anywhere. well, there's two things. certainly whatever install exits/signals, we need to do the same at the very end of the wrapper. we don't want install doing exit(1) and then our wrapper doing exit(0). as for trying to copy xattr's on files even after install fails, i'm on the fence about it. if `install` fails, then won't whatever is calling it also fail ? so is it critical that we try to copy over things anyways ? i guess it shouldn't hurt.
Next version coming in a sec ... some responses: (In reply to SpanKY from comment #48) > (In reply to Anthony Basile from comment #47) > > btw, you should also test these: > ./install-xattr > ./install-xattr --help > > seems to misbehave in both cases for me ... Yep. Fixed. Thanks for catching that. > > > If we were checking the return code for asprintf, should we do the same with > > malloc? I use it in 4 places and could write a helper. > > yes, there should be a xmalloc() helper. i meant that when i said "write > some helpers". any func that should have its return value checked and you > plan on just dying when it fails should have an x* helper. > Done. > > Okay, one space instead of one tab. FYI, I looked at pax-util's style for > > reference and there we find no indentation for labels. > > i've been getting away from labels in general. the ones that are left i > either convert to use one space or delete. the reason we want one space is > that `diff -p` get's confused otherwise. Ah! good to know. I was wondering why just one space. > > > > > /* You don't get a canonical path for ~/bin etc., so skip. */ > > > > if (!candir) > > > > goto skip; > > > > > > PATH would not contain a literal "~/". it gets expanded before being placed > > > into the $PATH env var. > > > > I'm not sure about this. Here's a little test program: > > i think you misinterpreted me. I did. > > my point being that your comment is misleading and i would just rephrase it > to "ignore invalid paths that cannot be cannoicalized". I get what you mean now. I just fixed the comment. > > > > use err() instead of abort() > > > > Everywhere? I think I still want abort() if fork() returns -1, and in the > > which() function, for the SIGABRT. Everywhere else I can use err(). > > abort produces no error message and simply kills itself with SIGABRT. err() > produces a friendly error message and exits with a non-zero value you > provide. so yes, i can't think of any place in this code you should be > using abort(). Done. > > > > iirc, the string optarg points to stays viable, so you could just take it: > > > target = optarg; > > > > That leads to a seg fault. > > works fine for me. did you also delete the free(target) ? Yep, forgot to remove the free(target). Thanks. /me wipes egg off my face > > > > > wait(&status); > > > > > > shouldn't you check the exit status ? if the install failed, we shouldn't > > > go trying to copy/restore things. we should exit the same way the child did. > > > > The return code is not so easy to interpret and I don't see it documented > > anywhere. > > well, there's two things. certainly whatever install exits/signals, we need > to do the same at the very end of the wrapper. we don't want install doing > exit(1) and then our wrapper doing exit(0). I don't think we have to do anything with signals. Reading the install.c code, it does catch a SIGCHLD but doesn't issue any signals. I also did a quick strace to check. What I did was I simply exited the parent with the same exit code as the child so we are reproducing install's exit(). > > as for trying to copy xattr's on files even after install fails, i'm on the > fence about it. if `install` fails, then won't whatever is calling it also > fail ? so is it critical that we try to copy over things anyways ? i guess > it shouldn't hurt. I'm continuing to copyxattrs() to files as long as install created them, even if install exit(1) as it does when there are extraneous directories as in my above example in comment #47. In the portage environment, I expect a failure in install would eventually propagate up to a failure in that ebuild stage. As you say, whether xattrs are copied or not will make little difference there. Outside of portage, I don't know.
Created attachment 367656 [details] c version of the wrapper
(In reply to Anthony Basile from comment #49) that doesn't stop other people/programs/kernel from sending signals to `install` :) merely the existence of the target file doesn't mean install is the one that created them. consider: rm -rf d mkdir d touch a b c install a b c d/ rm b install a b c d/ the second invocation will fail and d/b will exist, but the 2nd install didn't install that. this is kind of a stupid example, but think of cases where there are possibly multiple installs running in parallel and they try to install to the same place. one would fail, and maybe incorrect attrs will be copied over, but maybe we don't care because the install itself still failed.
Comment on attachment 367656 [details] c version of the wrapper >static void * >xmalloc(size_t size) >{ > char *ret = malloc(size); should be void* not char* > if (ret < 0) ret is a pointer, so this makes no sense. plus it's incorrect :). if (ret == NULL) >xsetxattr(const char *path, char *list, char *value , size_t size) no space before that , needed > lsize = xlistxattr(source, 0, 0); 2nd arg should probably be NULL > portage_xattr_exclude = getenv("PORTAGE_XATTR_EXCLUDE"); > if (portage_xattr_exclude == NULL) > exclude = strdup("security.* system.nfs4_acl"); > else > exclude = strdup(portage_xattr_exclude); this variable won't change across copyxattr invocations. i'd make it a global and do the initialization in main(). > len = strlen(exclude); > > /* We convert exclude[] to an array of concatenated null > * terminated strings. lxattr[] is already such an array. i think you mean NUL terminated > */ you mix tabs/spaces in a few places like here. please do a scan for it. > for (i = 0; i < len; i++) > if (isspace(exclude[i])) > exclude[i] = 0; could do: char *p; ... p = exclude; while ((p = strchr(p, ' '))) *p++ = '\0'; that'll allow an easy fix for the issue below ... > i = 0; > while (1) { > while (lxattr[i++] == 0) > continue; > if (i >= lsize) > break; i is a size_t while lsize is a ssize_t. in practice, this will probably never come up. once you make the isspace change above, you can just change i/j to a ssize_t. > if (!fnmatch(&exclude[j-1], &lxattr[i-1], 0)) spaces around those - please > if (xsize > 0) { > value = xmalloc(xsize); hmm, you always malloc/free this. how often does this get executed ? would it be better to utilize realloc instead ? set value to NULL at the top of the func, then declare a new value_size and set it to 0. then here do: if (xsize > value_size) value_size = xsize; value = realloc(value, value_size); then move the free(value) to the bottom of this func in the clean up path. > memset(value, 0, xsize); does this really need to be cleared ? if xgetxattr() fails, you'll abort. if it works, then the memory has been written hasn't it ? > skip: > while (lxattr[i++] != 0); put a "continue" as the body. > if (i >= lsize) > break; > } > > free(lxattr); > free(exclude); >} hmm, if copyxattr gets called more than once, you could leverage that xrealloc hack above and mark the buffers as static. then you wouldn't ever free then, just let them grow over time to fit the maximum ... if there's no real performance difference though, then maybe leave it simple ... > char *path, *env_path = getenv("PATH"); /* full $PATH string */ env_path can be const > /* If we don't have $PATH in our environment, then pick a sane path. */ > if (env_path == NULL) { > size_t len = confstr(_CS_PATH, 0, 0); > path = xmalloc(len); > confstr(_CS_PATH, path, len); > } > else cuddle that up: } else > path = strdup(env_path); add a xstrdup helper > /* ignore invalid paths that cannot be cannoicalized */ "canonicalized" > /* If argv[0]'s canonical dirname == the path's canonical dirname, then we > * skip this path otheriwise we get into an infinite self-invocation. > */ > if (!fnmatch(mycandir, candir, 0)) why fnmatch ? isn't this just strcmp() ? > /* If the file exists and is either a regular file or sym link, > * assume we found the system's install. > */ > if (stat(file, &s) == 0) > if (S_ISREG(s.st_mode) || S_ISLNK(s.st_mode)) { does that S_ISLNK make sense ? stat() will dereference things, so you'll never get back a symlink ... if it's broken, stat() will return an error. > return strdup(file); pretty sure you just want to return file. it's already allocated memory, so by strduping it, you're actually leaking the previous file allocation. > err(1, "failed to find system 'install'"); maybe also display current $PATH value ? probably not really needed. >int >main(int argc, char* argv[], char* envp[]) do: int main(int argc, char *argv[]) > install = which(dirname(argv[0])); > argv[0] = "install"; /* so coreutils' lib/program.c behaves */ i think this would work too: argv[0] = install; > execve(install, argv, envp); /* The kernel will free(install). */ pretty sure you don't need the *e variant: execv(install, argv) > default: > wait(&status); > status = WEXITSTATUS(status); you need to check WIFEXITED before using WIFEXITED. how about changing all the "return status" logic below to "goto done" and have the final logic do: /* Do the right thing and pass the signal back up. See: * http://www.cons.org/cracauer/sigint.html */ if (WIFSIGNALED(status)) { int signum = WTERMSIG(status); kill(getpid(), signum); return 128 + signum; } else if (WIFEXITED(status)) return WEXITSTATUS(status); else return EXIT_FAILURE; /* ??? */ > /* Are there enough files/directories on the cmd line to > * proceed? This can happen if install is called with no > * arguments or with just --help. In which case there is > * nothing the parent to do. > */ "... nothing for the parent to do." > if (!opts_target_directory) { > target = argv[last]; > if (stat(target, &s) != 0) > return EXIT_FAILURE; > target_is_directory = S_ISDIR(s.st_mode); > } else { > /* target was set above with the -t option */ > target_is_directory = 1; > } do you need to handle the -D option too ?
*** Bug 482290 has been marked as a duplicate of this bug. ***
I'm tracking these changes here. http://git.overlays.gentoo.org/gitweb/?p=proj/elfix.git;a=tree;f=misc/install.wrapper.c (In reply to SpanKY from comment #52) > Comment on attachment 367656 [details] > c version of the wrapper > > >static void * > >xmalloc(size_t size) > >{ > > char *ret = malloc(size); > > should be void* not char* Of course > > > if (ret < 0) > > ret is a pointer, so this makes no sense. plus it's incorrect :). > > if (ret == NULL) Yep. > > >xsetxattr(const char *path, char *list, char *value , size_t size) > > no space before that , needed > > > lsize = xlistxattr(source, 0, 0); > > 2nd arg should probably be NULL done > > > portage_xattr_exclude = getenv("PORTAGE_XATTR_EXCLUDE"); > > if (portage_xattr_exclude == NULL) > > exclude = strdup("security.* system.nfs4_acl"); > > else > > exclude = strdup(portage_xattr_exclude); > > this variable won't change across copyxattr invocations. i'd make it a > global and do the initialization in main(). agreed. done. > > > len = strlen(exclude); > > > > /* We convert exclude[] to an array of concatenated null > > * terminated strings. lxattr[] is already such an array. > > i think you mean NUL terminated > > > */ > > you mix tabs/spaces in a few places like here. please do a scan for it. Thanks for catching. > > > for (i = 0; i < len; i++) > > if (isspace(exclude[i])) > > exclude[i] = 0; > > could do: > > char *p; > ... > p = exclude; > while ((p = strchr(p, ' '))) > *p++ = '\0'; > > that'll allow an easy fix for the issue below ... > > > i = 0; > > while (1) { > > while (lxattr[i++] == 0) > > continue; > > if (i >= lsize) > > break; > > i is a size_t while lsize is a ssize_t. in practice, this will probably > never come up. once you make the isspace change above, you can just change > i/j to a ssize_t. done > > > if (!fnmatch(&exclude[j-1], &lxattr[i-1], 0)) > > spaces around those - please done > > > if (xsize > 0) { > > value = xmalloc(xsize); > > hmm, you always malloc/free this. how often does this get executed ? would > it be better to utilize realloc instead ? > > set value to NULL at the top of the func, then declare a new value_size and > set it to 0. then here do: > > if (xsize > value_size) > value_size = xsize; > value = realloc(value, value_size); > > then move the free(value) to the bottom of this func in the clean up path. > On any given invocation of install-xattr it will be the number of xattr names on that file, excluding the list in PORTAGE_XATTR_EXCLUDE. This is a small number. However, for packages with a large number of files, where each potentially has some xattr name, it could add up. The code that's on the git repo right now doesn't implement realloc, but I'll try it and see. > > memset(value, 0, xsize); > > does this really need to be cleared ? if xgetxattr() fails, you'll abort. > if it works, then the memory has been written hasn't it ? There were some cases where I got garbage for the value written for a given value, so I added it. However, I can't seem to reproduce now so I've removed it in a separate commit. If I see it happening again, I'll revert. > > > skip: > > while (lxattr[i++] != 0); > > put a "continue" as the body. Missed that one. Done. > > > if (i >= lsize) > > break; > > } > > > > free(lxattr); > > free(exclude); > >} > > hmm, if copyxattr gets called more than once, you could leverage that > xrealloc hack above and mark the buffers as static. then you wouldn't ever > free then, just let them grow over time to fit the maximum ... > > if there's no real performance difference though, then maybe leave it simple > ... I like it ;) I'll just put in a comment to alert the reader why we don't bother free-ing. > > > char *path, *env_path = getenv("PATH"); /* full $PATH string */ > > env_path can be const > > > /* If we don't have $PATH in our environment, then pick a sane path. */ > > if (env_path == NULL) { > > size_t len = confstr(_CS_PATH, 0, 0); > > path = xmalloc(len); > > confstr(_CS_PATH, path, len); > > } > > else > > cuddle that up: done. > > } else > > > path = strdup(env_path); > > add a xstrdup helper > done. > > /* ignore invalid paths that cannot be cannoicalized */ > > "canonicalized" > thanks. > > /* If argv[0]'s canonical dirname == the path's canonical dirname, then we > > * skip this path otheriwise we get into an infinite self-invocation. > > */ > > if (!fnmatch(mycandir, candir, 0)) > > why fnmatch ? isn't this just strcmp() ? Yeah because of realpath() a few lines above, it is here. > > > /* If the file exists and is either a regular file or sym link, > > * assume we found the system's install. > > */ > > if (stat(file, &s) == 0) > > if (S_ISREG(s.st_mode) || S_ISLNK(s.st_mode)) { > > does that S_ISLNK make sense ? stat() will dereference things, so you'll > never get back a symlink ... > > if it's broken, stat() will return an error. > Okay I wasn't sure about stat's behavior in this case, but I just tested with a little toy program and you're right. I punted S_ISLNK(). > > return strdup(file); > > pretty sure you just want to return file. it's already allocated memory, so > by strduping it, you're actually leaking the previous file allocation. > correct. fixed. > > err(1, "failed to find system 'install'"); > > maybe also display current $PATH value ? probably not really needed. can't hurt. > > >int > >main(int argc, char* argv[], char* envp[]) > > do: > > int > main(int argc, char *argv[]) > > > install = which(dirname(argv[0])); > > argv[0] = "install"; /* so coreutils' lib/program.c behaves */ > > i think this would work too: > > argv[0] = install; > it does but why? doesn't it have to be a string? > > execve(install, argv, envp); /* The kernel will free(install). */ > > pretty sure you don't need the *e variant: > > execv(install, argv) > you don't need envp. I took it out. > > default: > > wait(&status); > > status = WEXITSTATUS(status); > > you need to check WIFEXITED before using WIFEXITED. how about changing all > the "return status" logic below to "goto done" and have the final logic do: > > /* Do the right thing and pass the signal back up. See: > * http://www.cons.org/cracauer/sigint.html > */ > if (WIFSIGNALED(status)) { > int signum = WTERMSIG(status); > kill(getpid(), signum); > return 128 + signum; > } else if (WIFEXITED(status)) > return WEXITSTATUS(status); > else > return EXIT_FAILURE; /* ??? */ > > > /* Are there enough files/directories on the cmd line to > > * proceed? This can happen if install is called with no > > * arguments or with just --help. In which case there is > > * nothing the parent to do. > > */ > > "... nothing for the parent to do." This is great stuff! I haven't added it yet, but after playing a bit with some toy code to get a feel for how the signals propagate, I will. > > > if (!opts_target_directory) { > > target = argv[last]; > > if (stat(target, &s) != 0) > > return EXIT_FAILURE; > > target_is_directory = S_ISDIR(s.st_mode); > > } else { > > /* target was set above with the -t option */ > > target_is_directory = 1; > > } > > do you need to handle the -D option too ? No. In the repo I added a checkcopyattrs.sh which is triggered by `make check`. I added a test for this. If you can think of other tests/concerns, we can throw them into checkcopyattrs.sh.
(In reply to SpanKY from comment #52) > you need to check WIFEXITED before using WIFEXITED. how about changing all > the "return status" logic below to "goto done" and have the final logic do: > > /* Do the right thing and pass the signal back up. See: > * http://www.cons.org/cracauer/sigint.html > */ > if (WIFSIGNALED(status)) { > int signum = WTERMSIG(status); > kill(getpid(), signum); > return 128 + signum; > } else if (WIFEXITED(status)) > return WEXITSTATUS(status); > else > return EXIT_FAILURE; /* ??? */ > I read that article and played with this. The return after the kill() is never reached. But you do get the desired exit code of 128+signum. I tested this out using the following code: #include <stdio.h> #include <stdlib.h> #include <unistd.h> #include <err.h> #include <sys/wait.h> #include <sys/stat.h> #include <sys/types.h> #include <unistd.h> int main(int argc, char* argv[]) { int status, signum; pid_t pid; switch (fork()) { case -1: err(1, "fork() failed"); case 0: pid = getpid(); printf("child 0, pid = %d\n", pid); sleep(15); return EXIT_SUCCESS; default: pid = getpid(); printf("parent 0, pid = %d\n", pid); wait(&status); printf("parent 1\n"); if (WIFSIGNALED(status)) { signum = WTERMSIG(status); printf("parent 2, signum = %d\n", signum); kill(pid, signum); printf("parent 3\n"); return 128 + signum; } else if (WIFEXITED(status)) { printf("parent 4\n"); return WEXITSTATUS(status); } else return EXIT_FAILURE; /* We should never get here. */ } } and I ran it in the following bash script: #!/bin/bash ./signal echo $? If I just let it run its course, the child will EXIT_SUCCESS and you get $ ./doit.sh parent 0, pid = 31929 child 0, pid = 31930 parent 1 parent 4 0 If you run it and send the child SIGINT you get $ ./doit.sh parent 0, pid = 32023 child 0, pid = 32024 parent 1 parent 2, signum = 2 130 If you run it and send the chile SIGTERM you get $ ./doit.sh parent 0, pid = 32100 child 0, pid = 32101 parent 1 parent 2, signum = 15 ./doit.sh: line 2: 32100 Terminated ./signal 143 And if you send it SIGHUP you get $ ./doit.sh parent 0, pid = 32165 child 0, pid = 32166 parent 1 parent 2, signum = 1 ./doit.sh: line 2: 32165 Hangup ./signal 129 Notice "parent 3" is never printed out. If we remove that printf and the return 128 + signum after it, you get the same results.
Created attachment 368280 [details] c version of the wrapper This addresses all the issues of comments #52 through #55.
thank you so very, very much for this :) i threw together a skeletal structure to build and experiment with this. it's at: https://github.com/gmt/xattr-overlay I had to change a few things. First, there is a definite bug: https://github.com/gmt/xattr-install/commit/53aba80f9b998833f3f76c8bfdbc952e1f75d34d it just don't work right without that. Also, when run from /usr/bin, it likes to go nuts and fill up my process table with itself. Perhaps that's because it wasn't meant to be there. Anyhow, that was where I put it, so I "had" to do this: https://github.com/gmt/xattr-install/compare/xattr-install-0.1...xattr-install-0.3 To test it out you can try this overlay: https://github.com/gmt/xattr-overlay It's fast. Hey, I did have a question about the portage-side. If xattr is not in ${FEATURES}, then will the bin/ebuild-helpers/xattr directory be excluded from ${PATH}? If not, then I guess the right thing to do is to check and only run the wrapper if the feature isn't there? Or maybe, we should change it to actually only put the helper in PATH when the feature is set (assuming, it doesn't work that way already)? One way or another, it's definitely got to be wrong to require this for everyone, right? HTH, 'cause it's sure helpin' me ;)
(In reply to Greg Turner from comment #57) > thank you so very, very much for this :) > > i threw together a skeletal structure to build and experiment with this. > > it's at: https://github.com/gmt/xattr-overlay > > I had to change a few things. First, there is a definite bug: > > https://github.com/gmt/xattr-install/commit/ > 53aba80f9b998833f3f76c8bfdbc952e1f75d34d > > it just don't work right without that. Yeah this looks right. You can't use '?' for flags that take an argument. We'll have to add a patch something like what you have there. I had all the flags in an earlier version but then dropped them stupidly. > > Also, when run from /usr/bin, it likes to go nuts and fill up my process > table with itself. Perhaps that's because it wasn't meant to be there. > Anyhow, that was where I put it, so I "had" to do this: > > https://github.com/gmt/xattr-install/compare/xattr-install-0.1...xattr- > install-0.3 Yeah, when writing it, I wasn't thinking of /usr/bin. Arfrever suggested that later. So we'll just have to add some intelligence to which() if we do want it in /usr/bin. > > To test it out you can try this overlay: > > https://github.com/gmt/xattr-overlay > > It's fast. > > Hey, I did have a question about the portage-side. If xattr is not in > ${FEATURES}, then will the bin/ebuild-helpers/xattr directory be excluded > from ${PATH}? If not, then I guess the right thing to do is to check and > only run the wrapper if the feature isn't there? Or maybe, we should change > it to actually only put the helper in PATH when the feature is set > (assuming, it doesn't work that way already)? One way or another, it's > definitely got to be wrong to require this for everyone, right? > > HTH, 'cause it's sure helpin' me ;) I don't know about this. It may be okay. If you set user.pax.flags xattrs on some file, and the kernel ignores it, it doesn't hurt. In a sense, we're doing this already with PT_PAX phdr in every ELF object, whether on a vanilla machine or hardened. Try `readelf -l /bin/bash | grep PAX` even on a pure vanilla system.
(In reply to Anthony Basile from comment #58) > (In reply to Greg Turner from comment #57) > > To test it out you can try this overlay: > > > > https://github.com/gmt/xattr-overlay > > > > It's fast. I've used it a bunch, now. It seems to be working perfectly. If anything, the performance impact it has is greater than I expected. > > Hey, I did have a question about the portage-side. If xattr is not in > > ${FEATURES}, then will the bin/ebuild-helpers/xattr directory be excluded > > from ${PATH}? If not, then I guess the right thing to do is to check and > > only run the wrapper if the feature isn't there? Or maybe, we should change > > it to actually only put the helper in PATH when the feature is set > > (assuming, it doesn't work that way already)? One way or another, it's > > definitely got to be wrong to require this for everyone, right? > > > > HTH, 'cause it's sure helpin' me ;) > > I don't know about this. It may be okay. If you set user.pax.flags xattrs > on some file, and the kernel ignores it, it doesn't hurt. In a sense, we're > doing this already with PT_PAX phdr in every ELF object, whether on a > vanilla machine or hardened. Try `readelf -l /bin/bash | grep PAX` even on > a pure vanilla system. In the overlay, the dependency only kicks in if the xattr useflag is set. When the wrapper isn't there, then the old mechanism kicks in as a fallback. Looking at the code, I think the behavior is slightly different -- it looks like the old wrapper only copies xattr's if the xattr feature is active, whereas the binary wrapper does it every time. As you mention, that may be fine -- but if it's going to change the behavior, it's at least something to be aware of. Alternatively, we could check the FEATURES envvar in the binary wrapper and only clone xattrs if it's set. I do wonder whether we have targets in prefix for which the wrapper won't compile, and also how this could affect the prefix bootstrap process. ... one other thought. I have a place in an overlay where I really want to clone the xattr's from one file to another. It's for constructing binary wrappers -- the two files are not the same, but IIUC if I want my wrappers to play nice on hardened this is what I should do, as my wrapper stands in for the original executable. Perhaps there is some prior art on this, but I haven't yet found a recipe to follow, to do the actual cloning. I was thinking about splitting out the part of your code that does the xattr cloning into a library (probably just a build-time-only helper library), and consuming that from a second executable, "xattr-dup", which would specialize in doing precisely that. Any reason you'd advise against giving that a try?
(In reply to Greg Turner from comment #59) > Looking at the code, I think the behavior is slightly different -- it looks > like the old wrapper only copies xattr's if the xattr feature is active, > whereas the binary wrapper does it every time. As you mention, that may be > fine -- but if it's going to change the behavior, it's at least something to > be aware of. Alternatively, we could check the FEATURES envvar in the > binary wrapper and only clone xattrs if it's set. install.py does not check FEATURES. (It might matter only for packages, which use settings stored in files installed by other packages (e.g. INSTALL variable in /usr/lib*/python*/config/Makefile, available as distutils.sysconfig.get_config_var("INSTALL") in Python).)
(In reply to Arfrever Frehtes Taifersar Arahesis from comment #60) > /usr/lib*/python*/config/Makefile /usr/lib*/python*/config*/Makefile
(In reply to Arfrever Frehtes Taifersar Arahesis from comment #60) > (In reply to Greg Turner from comment #59) > > Looking at the code, I think the behavior is slightly different -- it looks > > like the old wrapper only copies xattr's if the xattr feature is active, > > whereas the binary wrapper does it every time. As you mention, that may be > > fine -- but if it's going to change the behavior, it's at least something to > > be aware of. Alternatively, we could check the FEATURES envvar in the > > binary wrapper and only clone xattrs if it's set. > > install.py does not check FEATURES. Just confirming that you're right, I was confused (we already discussed it on IRC, but for the sake of whomever). So no need to check that. Also FTR: I've discovered bin/xattr-helper.py which appears to meet my overlay's needs for xattr cloning; so quite unlikely I'll be going forward with the aformentioned possible re-factoring of the wrapper code.
(In reply to Anthony Basile from comment #54) > > > install = which(dirname(argv[0])); > > > argv[0] = "install"; /* so coreutils' lib/program.c behaves */ > > > > i think this would work too: > > > > argv[0] = install; > > it does but why? doesn't it have to be a string? which() returns a string doesn't it ? :) argv[0] = "/usr/bin/xattr-install" dirname(...) = "install" which(...) = "/usr/bin/install" argv[0] = "/usr/bin/install" which is how it's actually executed with your execv(). and how it's often done w/configure scripts that run /usr/bin/install directly after doing their own PATH/env/etc... search. in practice, the behavior will pretty much be the same. i was just trying to re-use some pointers you already had lying around rather than computing new ones / declaring unique strings.
(In reply to Anthony Basile from comment #55) > (In reply to SpanKY from comment #52) > > you need to check WIFEXITED before using WIFEXITED. how about changing all > > the "return status" logic below to "goto done" and have the final logic do: > > > > /* Do the right thing and pass the signal back up. See: > > * http://www.cons.org/cracauer/sigint.html > > */ > > if (WIFSIGNALED(status)) { > > int signum = WTERMSIG(status); > > kill(getpid(), signum); > > return 128 + signum; > > } else if (WIFEXITED(status)) > > return WEXITSTATUS(status); > > else > > return EXIT_FAILURE; /* ??? */ > > > > I read that article and played with this. The return after the kill() is > never reached. But you do get the desired exit code of 128+signum. the kill() is reachable if you execute the code with signals masked :). an unusual, but not impossible, edge case.
(In reply to Greg Turner from comment #57) there should be a comment in the source above the required_argument section explaining why it's there (just to consume the option that goes with the arg) also, we don't utilize any of the autoconf functionality. until we do, let's avoid autotools altogether. a simple Makefile is sufficient and a lot faster: CFLAGS ?= -O2 -pipe -g CFLAGS += -Wall all: xattr-install xattr-install: xattr-install.c $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) -o $@ $< $(LDLIBS) PREFIX = /usr BINDIR = $(PREFIX)/bin install: xattr-install install -m 755 -D xattr-install $(DESTDIR)$(BINDIR)/xattr-install clean: rm -f *.o *~ xattr-install .PHONY: all clean install
Okay I've included Greg's fix from comment 57/58. We do need to explicitly name command line flags that take an argument, like "-m 755". You don't need "Z::" Just "Z:" is fine. I've reintroduced the return after the kill(getpid(), signum) just in case signals are masked. I did not switch over from autotools to a pure Makefile in the elfix repo since I never meant for that build system to go into portage. Its just there as a convenient wrapper on the code so I can quickly build and test it. We'll have to figure out how to get this code into portage's build system. I changed the name of the c file from install.wrapper.c to install-xattr.c I'm calling the "project" install-xattr. I added some extra tests to checkcopyattrs.sh to make sure we don't mangle command line arguments when parsing them in the wrapper code. Current codebase is at: http://git.overlays.gentoo.org/gitweb/?p=proj/elfix.git;a=tree;f=misc/install-xattr.c;h=cb6540b0f491f297d596e947e7838ed6825706b7;hb=57cede997c62294d571735bfd85a1aa4345e1712
(In reply to SpanKY from comment #65) > a simple Makefile is sufficient and a lot > faster: hmm ... what do you think of just adding sys-apps/install-xattr to the tree as a separate package and have portage rdepend and use it when USE=xatttr?
(In reply to Anthony Basile from comment #67) > (In reply to SpanKY from comment #65) > > a simple Makefile is sufficient and a lot > > faster: > > hmm ... what do you think of just adding sys-apps/install-xattr to the tree > as a separate package and have portage rdepend and use it when USE=xatttr? Okay I've added sys-apps/install-xattr to the tree. Please review.
Okay this bug is pretty much done. We have two wrappers in the tree, a slow python fallback and a fast c version. We've actually got a third above in bash-4 (for when portage supports associated arrays). Portage is now pulling in install-xattr when USE=xattr and everything is either stable or near stabilization.
