FS#7484 - pacman clobbers directory symlinks
Attached to Project:
Pacman
Opened by Jeremy (loserMcloser) - Thursday, 21 June 2007, 05:43 GMT
Last edited by Dan McGee (toofishes) - Thursday, 29 November 2007, 19:01 GMT
Opened by Jeremy (loserMcloser) - Thursday, 21 June 2007, 05:43 GMT
Last edited by Dan McGee (toofishes) - Thursday, 29 November 2007, 19:01 GMT
|
Details
Description:
I have /opt symlinked to /usr/local. Doing (for example) pacman -S jre replaces this symlink with an actual directory, then proceeds to install java into this new directory. Additional info: * package version(s) pacman 3.0.5-2 (3.0.4-2 does not have this problem, didn't check 3.0.5-1) I upgraded many packages before I noticed this was happening, I wonder what other directory symlinks on my system are trashed??? |
This task depends upon
Closed by Dan McGee (toofishes)
Thursday, 29 November 2007, 19:01 GMT
Reason for closing: Fixed
Additional comments about closing: fixed in 3.1/git, and probably even 3.0.6
Thursday, 29 November 2007, 19:01 GMT
Reason for closing: Fixed
Additional comments about closing: fixed in 3.1/git, and probably even 3.0.6
If you prefer, you can :
* downgrade to 3.0.5-1 for getting possible security problems : http://www.archlinux.org/pipermail/pacman-dev/2007-June/008579.html
* downgrade to an older version, like 3.0.4, for getting wrong permission on some directories (like /tmp) causing many applications to fail : http://bugs.archlinux.org/task/7323
* help fixing this problem, without breaking anything else
The first change (in 3.0.5-1) was probably not a good idea, it was said to be hackish anyway :
http://cvs.archlinux.org/cgi-bin/viewcvs.cgi/lib/libalpm/add.c?cvsroot=Pacman
3.0.5-2 just reverted this hack, and upgraded libarchive to the last release instead, which fixed the /tmp problem.
But it also caused this issue. Probably libarchive behavior changed between 1.3.1 and 2.2.3
From archive_write_disk man page :
ARCHIVE_EXTRACT_NO_OVERWRITE
Existing files on disk will not be overwritten. By default, existing regular files are truncated and overwritten; exist-
ing directories will have their permissions updated; other pre-existing objects are unlinked and recreated from scratch.
Maybe the last point comes into play here.
If so, maybe it's possible to configure libarchive for changing this behavior.
For fixing it, I wonder if this ARCHIVE_EXTRACT_NO_OVERWRITE option would be ok.
And it fix this particular problem, but it could also introduce other unwanted behavior, I've no idea.
I made this change in lib/libalm/util.h :
< #define ARCHIVE_EXTRACT_FLAGS ARCHIVE_EXTRACT_OWNER | ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_TIME
---
> #define ARCHIVE_EXTRACT_FLAGS ARCHIVE_EXTRACT_OWNER | ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_TIME | ARCHIVE_EXTRACT_NO_OVERWRITE
About
bug 7323, using bsdtar from libarchive 1.3.1 for extracting the filesystem package, it gives the correct permission to tmp :bsdtar xvpzf /var/cache/pacman/pkg/filesystem-0.8-7.pkg.tar.gz tmp && stat -c %a tmp
x tmp/
1777
So I'm not sure if there was really a bug in it, maybe pacman incorrectly uses libarchive.. Though with libarchive 2.2.3, pacman gives correct permission to /tmp, but I still find it strange.
http://www.freebsd.org/cgi/query-pr.cgi?pr=93915
http://www.freebsd.org/cgi/query-pr.cgi?pr=109134
Problem is that the problem is already a bit complicated, and then they talk about bsdtar options,
while we are interested in libarchive option, so the translation needs to be made :)
Maybe that could explain why I wasn't able to reproduce the behavior of bsdtar described there.
1) apparently, all our packages contains directories
For example, the jre package has an /opt entry. I'm not sure this matters, but it was in relation to this comment:
"Assume you have a package containing only the following file (i.e. the
package does not specifically list /usr/local as a component of the
package):"
2) about the second case, when extracting the /usr/local/bin/foo file, while there is already a /usr/local/bin/foo symlink on the filesystem.
-> this shouldn't happen with pacman, since it checks for conflicts.
Also, I tried to get the behavior we wanted using bsdtar for extracting the filesystem package, assuming the following directory structures exist prior to
extracting that package:
etc -> foo
Using tar -xvzf /var/cache/pacman/pkg/filesystem-0.8-7.pkg.tar.gz will do what we want (extracting all etc/* files in foo/).
bsdtar -xvzf /var/cache/pacman/pkg/filesystem-0.8-7.pkg.tar.gz will remove the etc symlink, and create an etc/ directory.
Same with adding only k option, or only P option.
Only when adding both kP, so bsdtar -xvzkPf /var/cache/pacman/pkg/filesystem-0.8-7.pkg.tar.gz , it will behave like we want, by extracting etc/* files in foo/
Anyway, the result is the same as adding the ARCHIVE_EXTRACT_NO_OVERWRITE flag, like I already suggested above.
According to the man page, setting this flag will apparently disable these three points :
1) existing regular files are truncated and overwritten;
2) existing directories will have their permissions updated;
3) other pre-existing objects are unlinked and recreated from scratch.
I believe losing 1) shouldn't matter, since pacman remove the old file first in case of an upgrade.
And if the file is already there, it'll fail because of a conflict. Correct me if i'm wrong.
2) do we need this ? (maybe...)
3) I believe this bug report is about avoiding this behavior (in the case of symlinks at least), so that's what we want to remove.
So either this ARCHIVE_EXTRACT_NO_OVERWRITE does what we want, then it's perfect.
Otherwise, maybe I missed the magic libarchive option / flag ?
Otherwise, maybe this could be done in pacman manually..
it just moved from archive_read to archive_write_disk man page.
So, the behavior of libarchive 1.3.1 doesn't match the description at all.
By default, it apparently has neither 2) nor 3). Only 1)
So enabling ARCHIVE_EXTRACT_NO_OVERWRITE in libarchive 1.3.1 only disables point 1)
On the other hand, with libarchive 2.2.3 , the three points seem to be respected by default.
And enabling that NO_OVERWRITE flag will effectively disable the three.
To sum up, the only difference I see between libarchive 1.3.1 without the flag, and libarchive 2.2.3 with the flag is :
1) existing regular files are truncated and overwritten;
I first thought this wouldn't matter, since generally the old file is removed, and new one is extracted, but the NoUpgrade / backup files
seem to complicate things. Just adding ARCHIVE_EXTRACT_NO_OVERWRITE flag doesn't seem to work in all cases because of these particular files.
So it sounds like what we really want is to overwrite existing regular files, but not update directory permissions (probably prudent) or unlink other pre-existing objects (like symlinks) and replace them.
Which would be behavior 1 but not behaviors 2 and 3.
Am I following you?
ARCHIVE_EXTRACT_SECURE_SYMLINKS
Refuse to extract any object whose final location would
be altered by a symlink on disk. This is intended to
help guard against a variety of mischief caused by ar-
chives that (deliberately or otherwise) extract files
outside of the current directory. The default is not to
perform this check.
So if the default is not to perform this check, you would think that the default would be to allow the alteration of final location by symlinks normally.. but in fact, it would appear that instead the symlink is truncated and replaced by a directory. Isn't that what we're seeing?
If so, this really must be a bug.
> or unlink other pre-existing objects (like symlinks) and replace them.
> Which would be behavior 1 but not behaviors 2 and 3.
> Am I following you?
Yes, it seems like this was the old behavior of libarchive 1.3.1 , and afaik, there weren't any problems then.
About overwriting existing regular files, it looks like pacman requires this in some cases, but I'm not 100% sure,
I didn't figure it out what happens exactly during an upgrade yet.
> So if the default is not to perform this check, you would think that the default would be to allow the alteration of final location by symlinks normally..
> but in fact, it would appear that instead the symlink is truncated and replaced by a directory. Isn't that what we're seeing?
Yes, that confuses me as well, it looks a bit like if SECURE_SYMLINKS and UNLINK were enabled by default (according to their descriptions), but they aren't.
I guess it would be interesting to see what happens when they are enabled.
For testing options, I found untar.c in the examples/ dir of libarchive source tree a little more practical than pacman directly.
On the other hand, libarchive 2.2.3 match more the description of ARCHIVE_EXTRACT_NO_OVERWRITE option.
pacman overwrites regular files when forcing to install package that has file conflicts with already installed package.
I didn't insist on it, but it was very important :
"1) apparently, all our packages contains directories"
Suppose you've the following structure :
foo/ (directory)
foo/bar (regular file)
There are two possibilities when you make an archive of it :
1) the archive contains both foo/ and foo/bar
2) the archive contains only foo/bar
This will lead to different behavior when extracting this archive on a foo/ symlink :
1) in this case, when foo/ dir is extracted from the archive, it'll delete the foo symlink, unless NO_OVERWRITE is specified
2) in this case, it'll work just fine, unless the SECURE_SYMLINKS option is used.
With pacman packages, we are always in the first case.
yes good point, but in this case too, the conflicting files could be deleted before extracting the new one.
Not sure how good that is though.
My test scenario:
# mkdir /root/arf
# cd /usr
# ln -s /root/arf testsymlink
So now I have /usr/testsymlink -> /root/arf
Then I created testpackage, with one source file, foo, which the build() installs as /usr/testsymlink/bar/foo.
With pacman-3.0.5-2, when I install testpackage, I get no errors, and the symlink is replaced with a directory called testsymlink. Based on the fact that there exists a SECURE_SYMLINKS option, I believe this is an bug in libarchive and an incorrect behavior; it should be following the symlink. Otherwise there would be no security issue in the first place to need the SECURE_SYMLINKS option.
Then I applied a patch that adds NO_OVERWRITE and built my own test pacman, pacman-3.0.5-2.1.
I reset everything back to the test state (/usr/testsymlink -> /root/arf).
This time, when I ran pacman, the right thing happened, and the symlink was followed, however I got errors extracting /usr/ and /usr/testsymlink because they already existed.
So it looks like the NO_OVERWRITE option does basically what it is supposed to do, but that clobbering symlinks in general is a bug in libarchive.
Since it may be the case that some use libarchive and depend on the silly current behavior, I figure out how to fix the behavior and then for now add a FOLLOW_SYMLINKS option which causes that behavior to be followed. Then we can have pacman use this new flag and everything will work.
Seems like the problem actually happens when the dir itself is extracted, it's at this point that the symlink is deleted, and the dir extracted instead.
And so SECURE_SYMLINKS doesn't even play a role, because when the regular file is extracted, the dir symlink has already been deleted.
1) Expanding on what Romashka said: If a file exists on the filesystem that's not owned by the old version of the package, but is owned by the new version of the package, pacman will fail citing conflicts. If you run pacman with the -f flag, it will overwrite that file - afaik pacman doesn't remove that file first (though I haven't dug through the code to verify it), because it wasn't owned by the old version of the package. This behaviour should remain unchanged.
example:
Package foo-1 contains /usr/bin/foo
Some industrious user finds a manpage for foo online, and downloads it to /usr/man/man1/foo.1.gz
Package foo-2 decides to include this manpage, because it's an excellent manpage, and so foo-2 contains both /usr/bin/foo and /usr/man/man1/foo.1.gz.
pacman -Su will say "/usr/man/man1/foo.1.gz exists in filesystem, aborting due to conflicts" - if you run pacman -Suf it will overwrite that file. However, that file was never a part of foo-1, so when foo-1 is removed pre-upgrade, the manpage will probably still exist in the system, and must be overwritten by the version in foo-2.
2) Sometimes it's prudent for a package to actually include an empty dir - we probably shouldn't force removal of all empty dirs from packages.
Example: Say (poorly-written) application bar expects, nay demands, that /var/bar/1.2.3/datadir/ exists in the filesystem, or else it won't run and just errors out; it doesn't bother creating the dir (and if it's run as non-root, maybe it can't!). However, the default install doesn't bother adding any files to /var/bar/1.2.3/datadir/, so it's an empty dir, however the app needs it to run.
Yeah, contrived example, I agree, but I'm just saying it could possibly happen.
I was just saying that if it didn't, we could always add it. And anyway, I checked pacman, and it already does that :)
libalpm/add.c :
if(trans->flags & PM_TRANS_FLAG_FORCE) {
/* if FORCE was used, then unlink() each file (whether it's there
* or not) before extracting. this prevents the old "Text file busy"
* error that crops up if one tries to --force a glibc or pacman
* upgrade.
*/
unlink(filename);
}
> 2) Sometimes it's prudent for a package to actually include an empty dir - we probably shouldn't force removal of all empty dirs from packages.
is this related to what I said about the different behavior depending on whether the archive contains the dir or not ?
Or is it a different problem? I remember reading about that somewhere, but don't remember when/where.
Beautiful - thanks for doing the code-checking work for me. ;) That definitely resolves my first problem.
Regarding number two, I was just justifying the existence of dirs in the archive - they might be there, and we have to accommodate that.
Ah ok, that's more clear :)
Yes, I also think directories should be there. Also creating a package is just a natural tar -cvzf pkg/ , and that includes all directories in the structure anyway.
But since directories are part of archives, I was beginning to think that libarchive behavior is maybe not total non-sense.
Its goal by default seems to get all files and directories extracted, no matter what. If a file already exists, it's overwritten, if a symlink is there instead of a directory, it's replaced,
if a directory is already there but with different permissions, permissions are updated, etc..
But well, that's not really what pacman wants.
Anyway, I still didn't find a case where ARCHIVE_EXTRACT_NO_OVERWRITE would cause problems.
If the file already exists on the filesystem, I see the following cases :
1) normal file owned by a package being upgraded : pacman will remove the file first when removing the old package
2) backup file owned by a package being upgraded : pacman extracts to /tmp/alpm_XXXX (so libarchive doesn't overwrite any existing files)
3) it's a conflict : pacman just fails
4) it's a conflict and --force is used : the file is removed first (see my previous comment)
5) it's a directory (or symlink to a dir) : we want to use the existing one, not the one from the archive, so it's ok. Only problem is that libarchive will show some errors when trying to extract it, but I suppose that could be taken care of.
I could have missed some cases though.
But before modifying libarchive, I would just like to make sure there isn't a simple way for doing what we want :)
If libarchive default behavior was really wrong, or it's lacking some flags for being more configurable, maybe its author should also be contacted, to know his opinion ?
2) backup file owned by a package being upgraded : pacman first creates and open a /tmp/alpm_XXXX file (with mkstemp), then libarchive overwrites that file.
There are probably several ways to work around that, for example :
* don't use mkstemp : it seems like it's used in a weird way here anyway, since we don't even use the file descriptor returned by mkstemp.
I wonder if using tmpnam or tempnam would be really worse here, but well, I don't know much about this stuff..
* don't use ARCHIVE_EXTRACT_NO_OVERWRITE there (only for other files)
* don't extract the file on disk, but only in a buffer, and computes the md5 / sha1 that way.
I've been thinking about the third way, but that's by far the most complicated. the first two should be pretty easy.
but using it for all others files.
When extracting a directory that already exists, libarchive now returns ARCHIVE_WARN, which I handle just like a success (ARCHIVE_OK),
so that errors like "File/Directory already exists" are not displayed.
Any objections about going this way ? Things I overlooked ?
If not, it would be nice if this received a lot of testing..
Attaching a patch for pacman cvs (3.0 branch), and one for pacman git (3.1 branch).
0001-libalpm-add.c-fix-for-FS... (1.4 KiB)
http://www.archlinux.org/~paul/pacman-3.0.5-2.2-i686.pkg.tar.gz
In case others want to test. My initial test shows that this indeed fixes the issue. The question is if anything else is wrong!
I also made a package in local that I installed on my main box for further testing,
and now I installed yours on my 2 arch box.
I couldn't notice any problems yet, but that doesn't mean there isn't any.
Then we could install it each time we have a new version, to make sure the semantics are consistent. As soon as I have something, I'll put it up for people to try.
Of course, it is premised on the preinstall and postinstall working right! But at least it lets us test the rest of the semantics.
I don't know about its internals at all, no idea how it works, just know how to use it,
and it's currently not used for these situations.
But Dan said it could / should be extended for testing this as well.
* existing directories will have their permissions updated;
This is required at least in one case : the /tmp/ directory
This one has a very special status, because it's used by pacman for putting temporary files.
Suppose you don't have the /tmp/ dir, and you install the filesystem package.
pacman needs to extract the preinstall scriptlet first, for running it, and it extracts it to /tmp/.
If /tmp/ doesn't exist, it's created first, with default permission (755) :
_alpm_makepath(tmpdir); from _alpm_runscriptlet function, in libalpm/trans.c , line 593.
Only then, the other files from the filesystem package are extracted,
including the /tmp/ directory (where it has the correct 1777 permission),
but if NO_OVERWRITE flag is used, the permissions are not updated..
On the other hand, suppose I create a package with wrong permissions on all directories,
should all pre-existing directories be updated with wrong permissions ?
That problem is unsolvable..
It seems in general dangerous to change the permissions of existing directories by installing new packages, but it could break things either way. Maybe we should just start by having pacman issue warnings on install when the directory permissions do not match when installing new packages, so we'll know how often this is really happening.
* what if a directory contains subdirectories/files from both package foo and package bar; package foo thinks the common directory should have permissions XXXX, package bar thinks the common directory should have permissions YYYY -- order of installation will determine who wins, possibly breaking the losing package.
* directories are more likely than regular files to have user-modified permissions/ownership -- having pacman force update of permissions/ownership may undo something the user has carefully set up.
I think a warning from pacman on inconsistent permissions is probably the way to go.
> Warning: Package foo thinks directory /bar should be owned by USER1.GROUP1 and have permissions XXXX
> currently directory /bar is owned by USER2.GROUP2 and has permissions YYYY
There may also be a gotcha with always following directory symlinks: what if package foo version 1 purposefully contains a directory symlink. Maintainer of package foo thinks that in version 2, the symlink should be replaced by an actual directory. I guess this could be worked around by removing the symlink in the pre-install scriptlet, or would pacman remove the symlink during the package removal process?
Yes, permissions on directories are quite tricky. I think warnings about them may be the best idea, and warnings about files that are in the backup array but have different permissions between the original and new should be noticed as well.
The gotcha isn't a gotcha- if a package at version 1 contains a dir symlink, and version 2 does not, it should not matter if that is a directory only for that package. If pacman notices a dir is empty after removing files, it deletes the dir (and this should apply to symlinks as well).
Yes, that's what happens, but it can also cause other problems :
http://www.archlinux.org/pipermail/pacman-dev/2007-July/008688.html
I'm not on the pacman-dev mailing list, so I'll make a small comment about it here, though it may not be the proper place:
Seems to me ngaba's problem is more poor package/program design than a problem with pacman
* if the plugins in the coolplugin package are coolplayer-version dependent, they should be installed to /usr/lib/coolplayer/2.0/ not /usr/lib/coolplayer/current/
* if the plugins in the coolplugin package are coolplayer-version independent, they should be installed to a path that doesn't contain the coolplayer version like /usr/lib/coolplayer/plugins or something
----
Attached is a fix for this problem. I THINK it covers all the bases but
it needs testing to make sure there aren't any cases that I've missed.
See the patch header for full details.
For those that rather get it through git, checkout the FS7484 branch
(http://neptune-one.homeip.net/git?p=pacman;a=shortlog;h=FS7484).
When testing run pacman with the --debug option and look out for...
"%s (directory) won't be extracted over existing directory or symlink."
"%s (symlink) won't be extracted over existing directory."
Andrew
----
http://projects.archlinux.org/git/?p=pacman.git;a=commit;h=591bfabbd38bf4f8f209977f416a4e5fd3cc2baf
All I know for 3.1 is, that a month ago, someone said it might be released late july or early august. Any updates on that? Cuz system updates are piling up here... :]
Now that git module is installed directly to current though :)
So in your estimation, we think this is closeable? Isn't this what that symlink001.py pactest that I made testing? I'll add that to my working branch.
However, I just ran it, and I noticed the recent lstat change you made had a little typo : in _alpm_lstat, lstat should be run on newpath instead of path.
After making this change, symlink001 doesn't pass anymore, because pacman now detects a fileconflict with this new _alpm_lstat.
Maybe in fileconflict, stat or lstat should be used instead of _alpm_lstat. I am not sure yet :P
Xavier- _great_ catch. I'll look into this issue and report back here, although I might not get to it until later this weekend. Note that with the change, fileconflict001 passes! Interesting. At first glance, it does appear we maybe need to do both a stat and an lstat call...