Arch Linux

Probably all symlinks of this kind, if you upgraded packages owning files in the corresponding directories.

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.

Since it wasn't very clear, I confirm this bug, I could easily reproduce it.

For fixing it, I wonder if this ARCHIVE_EXTRACT_NO_OVERWRITE option would be ok.

At least, this option didn't exist in libarchive 1.3.1, it appeared after.
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.

I have the same problem. I have a symlink to my var, which I keep on my home partition. Whenever I run pacman, this symlink gets overwrited with a directory the exact same.

Confirming the bug, I totally didn't notice it until I stumbled upon this by accident. Crap. Hoping for a fix.

I've found 2 similar bug reports @ freebsd but no working solution.
http://www.freebsd.org/cgi/query-pr.cgi?pr=93915
http://www.freebsd.org/cgi/query-pr.cgi?pr=109134

That's interesting to know indeed, but this confused me even more :)
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 :)

Hm, maybe another (bigger?) problem is that it looks like these two reports are already too old, compared to libarchive active development.
Maybe that could explain why I wasn't able to reproduce the behavior of bsdtar described there.

I think there are also more differences with the bug reports 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..

I was wrong earlier, libarchive 1.3.1 already had this ARCHIVE_EXTRACT_NO_OVERWRITE option, with the same description,
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.

Thank you for this immensely helpful thread. I'm trying to slog through this now in the hopes of making some helpful progress.

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?

Hmm, here's what I really wonder about:

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.

Sorry I've been out of the loop and discussion on this one. Within the next few days I will try to sit down and take a serious look at the issues we have with all of this. Let me know what you guys continue to discover and test with this.

> 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?

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.

> About overwriting existing regular files, it looks like pacman requires this in some cases, but I'm not 100% sure,

pacman overwrites regular files when forcing to install package that has file conflicts with already installed package.

Oops, sorry, about that later point on SECURE_SYMLINKS, I already forgot something I saw earlier.
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.

> pacman overwrites regular files when forcing to install package that has file conflicts with already installed package.

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.

It's all pretty complex. I did one good test last night, and then concluded I needed to look at the source for libarchive to try to patch it, which I started to do and will continue today.

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.

Sorry, I forgot an important point in my first answer, did you see it : http://bugs.archlinux.org/task/7484#comment17498 ?
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.

I'd like to add a couple comments.

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.

> 1) with the -f flag, it will overwrite that file - afaik pacman doesn't remove that file first

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.

> I was just saying that if it didn't, we could always add it. And anyway, I checked pacman, and it already does that :)

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.

> 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 ?

Finally, the 2) is problematic :
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.

I decided to go with the second way, so not using ARCHIVE_EXTRACT_NO_OVERWRITE for extracting files in backup array,
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).

I built a package for i686 which applies this on top of pacman 3.0.5-2:

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!

Thanks for testing and providing a package.
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.

I was thinking about writing a test suite package, to try a lot of cases. It would be called something like pacmantestpackage, and it would have a pre-install scriptlet that put some stuff in place the way we wanted it, then a post-install scriptlet that ran and checked that things were as they were supposed to be.

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.

Actually, we already have a testsuite in pacman for regression testing, see the pactest/ directory in pacman source tree.
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.

OK- this patch has been pushed to GIT, and I'm guessing it should make it to CVS as well fairly soon, and we should at least release a patched version of pacman that prevents this from happening. Thoughts from anyone else on this one, especially those testing Paul's patched version?

About this behavior disabled by the NO_OVERWRITE flag :
* 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..

I wonder how many more of these special cases there are.. if it's just this one, can't we make pacman create /tmp the first time with the correct permissions?

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.

You might have more problems with directory permissions:

* 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?

In reply to above:
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).

> or would pacman remove the symlink during the package removal process?

Yes, that's what happens, but it can also cause other problems :
http://www.archlinux.org/pipermail/pacman-dev/2007-July/008688.html

Re: 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

For those not subscribed to pacman-dev...

----
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
----

The pacman-dev ML has a lot more info, but it should be fixed with this commit:
http://projects.archlinux.org/git/?p=pacman.git;a=commit;h=591bfabbd38bf4f8f209977f416a4e5fd3cc2baf

A workaround until this is fixed: Use mount --move or mount --bind

Yeah kind of, it still doesn't restore the trust in pacman since it only prevents pacman from messing around with that single link. Is there an update on when 3.1 (or another pacman version with this bug fixed) will be released?
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... :]

Any of the 15 people watching this bug run a test against the pacman-git testing releases yet?

I checked a while ago with the git package that used to install /usr/lib/perl5/site_perl/5.8.8/Git.pm , with 5.8.8 symlink to current. It worked fine.
Now that git module is installed directly to current though :)

I'll probably change that PKGBUILD back once 3.1 is released, as I really don't think it was a bug anymore...but I had to close that other bug. :)

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.

Where's symlink001.py? It's not in git.

Yes, I think that symlink001 pactest is exactly what is needed for closing this bug :)
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

Aaron- see my working branch, as I stated above.

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...

The lstat issue has been taken care of, the symlink001 pactest has been committed and pass, so I think we are alright.

I'm going to agree with you here- a lot of us have been running pacman 3.1 without noticing any of these issues, and all the pactests dealing with this pass fine. Closing as fixed in 3.1 (even though I think we fixed the majority of this between 3.0.5 and 3.0.6).