| Summary: | app-arch/tar-1.26: add patch to support xattr | ||
|---|---|---|---|
| Product: | Gentoo Linux | Reporter: | Anthony Basile <blueness> |
| Component: | Current packages | Assignee: | Gentoo's Team for Core System packages <base-system> |
| Status: | RESOLVED FIXED | ||
| Severity: | enhancement | CC: | alexander, arfrever.fta, hardened, mjo, nikoli, pavel, pruzinat, xdudka00 |
| Priority: | Normal | ||
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Package list: | Runtime testing required: | --- | |
| Bug Depends on: | |||
| Bug Blocks: | 427888, 431848 | ||
| Attachments: |
Add xattr support to tar
Add xattr support to tar Patch to add full xattr support to tar Patch against tar-1.26.ebuild to add the above patch |
||
|
Description
Anthony Basile
2011-09-06 17:29:48 UTC
Created attachment 285715 [details, diff]
Add xattr support to tar
Comment on attachment 285715 [details, diff]
Add xattr support to tar
seems to me your xattr patch is significantly larger than fedora's because you included a lot of .orig files. yours is ~700K while fedora's is ~50K.
Created attachment 285767 [details, diff]
Add xattr support to tar
Yeah, the previous patch included all the .orig files which are unnecessary and bad because it makes it big. Thanks for catching that.
bleh, that patch brings acl, xattr, and selinux support in one patch. but the acl/xattr logic is not conditional. Yeah, I noticed that and want to come back to it to tease out just the xattr support. (In reply to comment #5) > Yeah, I noticed that and want to come back to it to tease out just the xattr > support. I'm able to separate the xattr, acl and selinux support so they're conditionally enabled, either as one patch or three, but I'm bothered by something. I'm unable to get tar to preserve xattr like it does selinux labels. I tested on an vanilla fedora 15 system with their own tar with no monkeying from me and I still wasn't able to get xattr. Steps to reproduce: 0. Vanilla fedora 15, with selinux enabled or disabled (in case some policy is preventing stuff) 1. mount -o user_xattr /dev/sdb1 /mnt/sdb1 2. cd /mnt/sdb1 && mkdir test && cd test 3. touch f1 f2 4. setfattr -n user.x -v "test1" f1 5. setfattr -n user.y -v "test2" f2 6. getfattr -m - * # file: f1 security.selinux user.x #file: f2 security.selinux user.y 7. Tar the directory with tar --xattr -zcvf test.tgz test/ 8. Untar the directory with tar --xattr -xf test.tgz 9. cd test/ && getfattr -m - * Nothing! xattrs go poof! I'm looking into code now and I'll try to figure out what its missing. I'm not sure its looking at all xattr namespaces, like user.*, and we'll want want that. Okay I found the bug causing the problem in comment #6. It is occurs when xattrs.c:xattrs_xattrs_get() gets called with fd=0. This call come from one line in create.c:dump_file0(). There int fd=0 at the top of the function, and gets left as fd=0 in the case of files, or gets set by subfile_open() to a directory file descriptor. In xattrs_xattrs_get(), there is a check for whether to list the xattrs using the file name+patch: xret = llistxattr (file_name, xatrs, xsz) or file descriptor xret = flistxattr (fd, xatrs, xsz)) depending on whether or not fd == -1, not zero! The reason for that is because elsewhere in create.c where xattrs_xattrs_get() is called, there is no fd and we want to list xattrs by file name, these other calls in create.c look like xattrs_xattrs_get(st, p, -1); Okay! So in the case of files, we default to flistxattr() with fd=0 ... badness! The fix is easy, just replace xattrs_xattrs_get(st, p, fd); with if (fd == 0) xattrs_xattrs_get(st, p, -1); else xattrs_xattrs_get(st, p, fd); in create.c:file_dump() with an explanatory comment. I'll send this fedora's way. Nonetheless, I've got a cleaner approach. Since in gentoo we want *all* xattrs, not just a selective few (and the fedora patch does filter), I've written another patch which will archive all xattrs. When I've clean it up, I'll post it for your consideration. I don't want to put this in the URL above because this bug is about getting xattrs into our tar-1.26, not about a bug in fedora's patch: https://bugzilla.redhat.com/show_bug.cgi?id=717684 Still we will want to follow what they have to say. Created attachment 287617 [details, diff]
Patch to add full xattr support to tar
This patch adds full xattr support to tar, meaning that it doesn't support only certain name spaces, like the original patch. Any extended attributes found will be added to the archive using
tar --xattr -zcvf mystuff.tgz mystuff/
and will be extracted using
tar --xattr -xvf mystuff.tgz
The only flags it takes are --xattr and --no-xattr. There are no --selinux or --acl options (or corresponding -no- options). I think this is more in line with what we want in gentoo anyhow, especially since we're thinking of migrating PaX flags from PT_PAX to xattr.
I've tested this on both vanilla and selinux profiles and haven't hit any issues yet.
Created attachment 287623 [details, diff]
Patch against tar-1.26.ebuild to add the above patch
This will --enable-xattr if USE has one or more of acl caps selinux xattr.
should be all set now in the tree; thanks for the report! Commit message: Add xattr support http://sources.gentoo.org/app-arch/tar/files/tar-1.26-xattr.patch?rev=1.1 http://sources.gentoo.org/app-arch/tar/tar-1.26-r1.ebuild?rev=1.1 There is an inconsistency in used header and library. attr/xattr.h and libattr.so belong to sys-apps/attr. sys-libs/glibc provides sys/xattr.h and libc.so. Both libattr.so and libc.so define getxattr(), setxattr() and other functions. app-arch/tar includes attr/xattr.h from sys-apps/attr, but executables of app-arch/tar aren't linked against libattr.so and are linked against libc.so, so functions from sys-libs/glibc are used at run time. At Arfrever's request, I'm reopening this to discuss the header/library mismatch. One of possible solutions is to make the option in configure.ac accept an argument: --enable-xattr=attr --enable-xattr=glibc Used header and library would depend on this argument. --enable-xattr=glibc would be used on systems with "elibc_glibc" USE flag enabled. --enable-xattr=attr would be used on other systems. (In reply to comment #14) > One of possible solutions is to make the option in configure.ac accept an > argument: > --enable-xattr=attr > --enable-xattr=glibc > Used header and library would depend on this argument. > > --enable-xattr=glibc would be used on systems with "elibc_glibc" USE flag > enabled. > --enable-xattr=attr would be used on other systems. These two headers are identical with some minor differences, so we're not introducing horrible breakage, but this needs to be fixed. I checked fedora and they are indeed linking against libattr. Passing -lattr to the ebuild will link our tar against libattr, but I'm a bit confused because it still links against glibc, as should be, but then when a call is made to setxattr or getxattr (and friends), how do we know whether the glibc or the libattr code is invoked? (In reply to comment #15) at link time, the symbol version is recorded, so `tar` will look for it from the library it linked it from $ readelf -sW /lib/libc.so.6 | grep lgetxattr 1530: 00000000000e3190 40 FUNC GLOBAL DEFAULT 11 lgetxattr@@GLIBC_2.3 $ readelf -sW /lib/libattr.so.1 | grep lgetxattr 39: 0000000000003080 24 FUNC GLOBAL DEFAULT 11 lgetxattr@@ATTR_1.0 $ readelf -sW /bin/tar | grep lgetxattr 37: 0000000000000000 0 FUNC GLOBAL DEFAULT UND lgetxattr@GLIBC_2.3 Hello, I have proposed new xattrs patch to upstream GNU tar:
http://lists.gnu.org/archive/html/bug-tar/2012-08/msg00012.html
There are several bugfixes inside, testsuite and some new features (listing,
--xattrs-include=* options). Extended attributes are separated from SELinux
and ACLs support. Any comment is welcomed.
Pavel
*** Bug 431392 has been marked as a duplicate of this bug. *** (In reply to comment #17) > Hello, I have proposed new xattrs patch to upstream GNU tar: > > http://lists.gnu.org/archive/html/bug-tar/2012-08/msg00012.html > > There are several bugfixes inside, testsuite and some new features (listing, > --xattrs-include=* options). Extended attributes are separated from SELinux > and ACLs support. Any comment is welcomed. > > Pavel Hopefully this will get accepted/maintained upstream. I've nver understood though, why xattrs, selinux and acl are always treated separately. BTW, I haven't had time to look but do your patches address Arfrever's point in comment 14. (In reply to comment #19) > (In reply to comment #17) >> Hello, I have proposed new xattrs patch to upstream GNU tar: >> >> http://lists.gnu.org/archive/html/bug-tar/2012-08/msg00012.html >> >> There are several bugfixes inside, testsuite and some new features (listing, >> --xattrs-include=* options). Extended attributes are separated from SELinux >> and ACLs support. Any comment is welcomed. >> >> Pavel Anthony, > Hopefully this will get accepted/maintained upstream. I've nver understood > though, why xattrs, selinux and acl are always treated separately. the reason for that is that selinux and acls have their own interface. Its implementation may theoretically differ on some platforms (or may be changed in future) but this interfaces should stay the same. At least it should be more portable. Another example may be the linux capabilities -- its stored binary format is human non-readable -- same as acls. The difference: $ touch file $ sudo setcap cap_chown+ep file $ getcap file # used api.. file = cap_chown+ep $ getfattr -d -m. file # file: file security.capability=0sAQAAAgEAAAAAAAAAAAAAAAAAAAA= security.selinux="unconfined_u:object_r:file_t:s0" If the capabilities were stored in a plain text format, it would be probably more portable (non-obfuscated, see man cap_to_text()). > BTW, I haven't had time to look but do your patches address Arfrever's point > in comment 14. Good point, thanks, actually we are using wrong header for libattr but we are linking against glibc. The reason why 'ldd /usr/bin/tar' says libattr is that we are linking against -lacl which implies (maybe also not necessarily, I'm not sure) libattr also. As for the xattrs patch -> the glibc header sys/xattr.h should be used and I'll repair the gnulib m4 file to address that. Pavel (In reply to comment #20) > Its > implementation may theoretically differ on some platforms (or may be changed > in > future) but this interfaces should stay the same. Okay fair enough. should we move forward with stabilizing this ? or just let it sit in ~arch until upstream merges something and then we go with 1.27 ? (In reply to comment #22) > should we move forward with stabilizing this ? or just let it sit in ~arch > until upstream merges something and then we go with 1.27 ? I think we should move to stabilize. The only remaining issue with this patch is Arfrever's point above, which is correct, but isn't introducing any horrible breakage, at least on glibc. I'll prioritize fixing that, but we're going to need this for xattr based pax markings. This patch is in upstream already. There is addressed the glibc/libattr issue also -> using the glibc by default: http://lists.nongnu.org/archive/html/acl-devel/2012-04/msg00001.html Pavel (In reply to comment #24) > This patch is in upstream already. There is addressed the glibc/libattr > issue > also -> using the glibc by default: > > http://lists.nongnu.org/archive/html/acl-devel/2012-04/msg00001.html > > Pavel Can you get us the commit. The commit: 696338043 and the next two are most important: http://git.savannah.gnu.org/cgit/tar.git/commit/?id=696338043e52f440853e1143c52b81b41cd59723 There follows another fixes in the git log. Pavel (In reply to comment #26) > The commit: 696338043 and the next two are most important: > > http://git.savannah.gnu.org/cgit/tar.git/commit/ > ?id=696338043e52f440853e1143c52b81b41cd59723 > > There follows another fixes in the git log. > > Pavel Actually looks like 5 commits in all. If 1.27 is coming out soon, we can wait, else we can backport those patches, or use mine in the mean time. I don't care which as long as we don't wait too long because we really want to get the pieces for xattr based pax flags in place. (In reply to comment #27) i'm fine with the current situation. iiuc, it doesn't make any difference in practice. so let's stabilize 1.26-r1 and we'll drop the whole shebang whenever 1.27 drops. |