Re: [pacman-dev] difficult symlink "puzzle"
Hi! IMHO this problem is more general and serious, and this should be solved. Either you specify some strict Arch packaging standards to avoid this bug or you improve pacman. I prefer the 2nd, and that is not easy; I make some suggestions here to discuss: First I define the goal: the content of foo's .FILELIST exists on filesystem iff foo is installed. OK, I assume that the goal above holds for all installed package; we just need to keep this during package install/removal. Install: -If we install /dir/file: That exists until package foo is removed (in normal case). If dir is a symlink, we MUST record the "real" destination of file to localdb/foo/files too, because the dir symlink can be removed/modified during an other package removal (or update!!: you can easily imagine a modified perl example where current is a symlink to the latest version...) or by the user. -If we install /dir/dir_symlink: First, we treat dir2 as a regular file, and do the steps above. Then we check if /dir1/dir2 exists. If no, we simply create it pointing to the requested dir. If yes and that is not a real dir, we can stop with file conflict. If yes and dir_symlink is a real dir, we can move its content to the dir_symlink-pointed dir, then delete/create_link dir_symlink; we must also record the file movements to the localdb as above (this is slow, queryfileowner needed, but this a rare case). Remove: -If we remove /dir/file: no extra work needed, just we need to use "real" path. -If we remove /dir/dir_sl: If dir_sl_pointed is empty or doesn't exist, then simply remove dir_sl. If dir_sl_pointed is not empty then we must create a real dir named dir_sl, and moved some files (we scan localdb for files need to be installed as /dir/dir_sl/file) from dir_sl_pointed to there (and record the movements to localdb again.). Bye, ngaba PS: We can implement these without db conversion (and keep compatibility). ---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program.
... or by the user. If we skip this chance, so we assume that ONLY pacman deletes/renames "package" dir-symlinks, we can leave out the ugly double db store (I mean storing the "real" paths here). With this simplification the above algorithm is quite "programmer friendly" (we have to deal with dir symlinks only);-) Bye, ngaba
---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program.
Well this is how dpkg does it (src/processarc.c:471) /* * Now we unpack the archive, backing things up as we go. * For each file, we check to see if it already exists. * There are several possibilities: * + We are trying to install a non-directory ... * - It doesn't exist. In this case we simply extract it. * - It is a plain file, device, symlink, &c. We do an `atomic * overwrite' using link() and rename(), but leave a backup copy. * Later, when we delete the backup, we remove it from any other * packages' lists. * - It is a directory. In this case it depends on whether we're * trying to install a symlink or something else. * = If we're not trying to install a symlink we move the directory * aside and extract the node. Later, when we recursively remove * the backed-up directory, we remove it from any other packages' * lists. * = If we are trying to install a symlink we do nothing - ie, * dpkg will never replace a directory tree with a symlink. This * is to avoid embarrassing effects such as replacing a directory * tree with a link to a link to the original directory tree. * + We are trying to install a directory ... * - It doesn't exist. We create it with the appropriate modes. * - It exists as a directory or a symlink to one. We do nothing. * - It is a plain file or a symlink (other than to a directory). * We move it aside and create the directory. Later, when we * delete the backup, we remove it from any other packages' lists. * * Install non-dir Install symlink Install dir * Exists not X X X * File/node/symlink LXR LXR BXR * Directory BXR - - * * X: extract file/node/link/directory * LX: atomic overwrite leaving backup * B: ordinary backup * R: later remove from other packages' lists * -: do nothing * * After we've done this we go through the remaining things in the * lists of packages we're trying to remove (including the old * version of the current package). This happens in reverse order, * so that we process files before the directories (or symlinks-to- * directories) containing them. * + If the thing is a conffile then we leave it alone for the purge * operation. * + Otherwise, there are several possibilities too: * - The listed thing does not exist. We ignore it. * - The listed thing is a directory or a symlink to a directory. * We delete it only if it isn't listed in any other package. * - The listed thing is not a directory, but was part of the package * that was upgraded, we check to make sure the files aren't the * same ones from the old package by checking dev/inode * - The listed thing is not a directory or a symlink to one (ie, * it's a plain file, device, pipe, &c, or a symlink to one, or a * dangling symlink). We delete it. * The removed packages' list becomes empty (of course, the new * version of the package we're installing will have a new list, * which replaces the old version's list). * * If at any stage we remove a file from a package's list, and the * package isn't one we're already processing, and the package's * list becomes empty as a result, we `vanish' the package. This * means that we run its postrm with the `disappear' argument, and * put the package in the `not-installed' state. If it had any * conffiles, their hashes and ownership will have been transferred * already, so we just ignore those and forget about them from the * point of view of the disappearing package. * * NOTE THAT THE OLD POSTRM IS RUN AFTER THE NEW PREINST, since the * files get replaced `as we go'. */ Andrew
Ok how about this (see the attached code snippet). _alpm_add_commit() is a BIG function, the code attached only shows the for loop for extracting the files and I've cut out the code for checking backups(). I've used the behaviour for dpkg as a bases for what we should do... * Install non-dir Install symlink Install dir * Exists not X X X * File/node/symlink LXR LXR BXR * Directory BXR - - * * X: extract file/node/link/directory * LX: atomic overwrite leaving backup * B: ordinary backup * R: later remove from other packages' lists * -: do nothing Before I try splicing this into _alpm_add_commit() does this look right? Does it cover all the bases? Andrew int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) { // ... starting @ lib/libalpm/add.c:443 for (i = 0; archive_read_next_header(archive &entry) == ARCHIVE_OK; i++) { const char *entryname = archive_entry_pathname(entry); char filename[PATH_MAX]; int needbackup = 0; mode_t entry_type = archive_entry_filetype(entry); struct stat buf; memset(filename, 0, PATH_MAX); /* just to be sure */ if(strcmp(entryname, ".INSTALL") == 0) { /* The install script goes inside the db. */ sprintf(filename, PATH_MAX, "%s/%s-%s/install", db->path, newpkg->name, newpkg->version); } else if(strcmp(entryname, ".CHANGELOG") == 0) { /* The changelog goes inside the db. */ sprintf(filename, PATH_MAX, "%s/%s-%s/changelog", db->path, newpkg->name, newpkg->version); } else if(*entryname == '.') { /* Ignore all other files in the root directory starting with '.' * that haven't already been handled (for future posibilities). */ archive_read_data_skip(archive); continue; } else if(alpm_list_find_str(handle->noextract, entryname)) { /* If a file is in NoExtract then we never extract it. */ _alpm_log(PM_LOG_DEBUG, _("%s is in NoExtract, skipping extraction"), entryname); alpm_logaction(_("%s is in NoExtract, skipping extraction"), entryname); archive_read_data_skip(archive); continue; } else if(alpm_list_find_str(trans->skip_add, entryname)) { /* If a file is in the add skiplist we never extract it. */ _alpm_log(PM_LOG_DEBUG, _("%s is in trans->skip_add, skipping extraction"), entryname); archive_read_data_skip(archive); continue; } else { sprintf(filename, PATH_MAX, "%s%s", handle->root, entryname ); if(stat(filename, &buf) == 0) { if((S_ISDIR(entry_type) || S_ISLNK(entry_type)) && S_ISDIR(buf.st_mode)) { /* Directories and symlinks are not extracted if a directory exists * on the filesystem with the same name. */ archive_read_data_skip(archive); continue; } else if(S_ISREG(buf.st_mode)) { /* If the file already exists on the filesystem check if we need to * do a backup. */ needbackup = ...; if(needbackup) { // Do compare and backup.... } } } } unlink(filename); archive_entry_set_pathname(entry, filename); int ret = archive_read_extract(archive, entry, archive_flags | ARCHIVE_EXTRACT_NO_OVERWRITE); if(ret == ARCHIVE_WARN) { /* operation succeeded but a non-critical error was encountered */ _alpm_log(PM_LOG_DEBUG, _("warning extracting %s (%s)"), entryname, archive_error_string(archive)); } else if(ret != ARCHIVE_OK) { _alpm_log(PM_LOG_ERROR, _("could not extract %s (%s)"), entryname, archive_error_string(archive)); alpm_logaction(_("could not extract %s (%s)"), entryname, archive_error_string(archive)); errors++; continue; } // update backup checksums } /* for archive_read_next_header(archive, &entry) == ARCHIVE_OK */ archive_read_finish(archive); // ... ending @ lib/libalpm/add.c:762 } // vim:set ts=2 sw=2 noet:
On 7/3/07, Andrew Fyfe <andrew@neptune-one.net> wrote:
Ok how about this (see the attached code snippet).
_alpm_add_commit() is a BIG function, the code attached only shows the for loop for extracting the files and I've cut out the code for checking backups().
<snip> On a side note, Xavier has a clean_add branch you may want to work off of. -Dan
Dan McGee wrote:
<snip> On a side note, Xavier has a clean_add branch you may want to work off of.
Where can I find Xavier's git repo? Andrew
On 7/3/07, Andrew Fyfe <andrew@neptune-one.net> wrote:
Dan McGee wrote:
<snip> On a side note, Xavier has a clean_add branch you may want to work off of.
Where can I find Xavier's git repo?
2007/7/3, Dan McGee <dpmcgee@gmail.com>:
On 7/3/07, Andrew Fyfe <andrew@neptune-one.net> wrote:
Ok how about this (see the attached code snippet).
_alpm_add_commit() is a BIG function, the code attached only shows the for loop for extracting the files and I've cut out the code for checking backups().
<snip> On a side note, Xavier has a clean_add branch you may want to work off of.
Well, that's exactly the reason I wanted to do this refactoring, because I needed to look at add_commit for these extraction problem, and I wanted to isolate things for making the code easier to read and edit. There was also an suggestion from Aaron for refactoring this huge add_commit function that made a lot of sense to me: http://www.archlinux.org/pipermail/pacman-dev/2006-December/006286.html But I miserably failed trying to do this refactoring. It's currently totally broken (by the last commit) :( And I don't think I'm able to fix it, there is just too many code in this function, and too many variables to handle.. But too complicated for me doesn't mean it wouldn't be easy for some people.
2007/7/3, Andrew Fyfe <andrew@neptune-one.net>:
Well this is how dpkg does it (src/processarc.c:471)
Oh that's exactly what I was wondering after trying to read Nagy's mail, how other package managers did it. But well, that really hurts my head badly... I think I'll just go back using dpkg, since everything is already handled there :)
Hi! First, I want to underline again, that the problem is serious, because -U = -Rd + -A.
But well, that really hurts my head badly...
OK, I will try to explain the "programmer friendly" version again;-) The goal: content of foo package must be exist on the HDD iff foo is installed (the filepath can be manipulated using symlinks, that is not(?) important: if /path/file exists in filelist, /path/file must exist on HDD iff foo is installed; /path can be symlink here.). My solution(?): We can assume, that the goal above holds before pacman is invoked, we just need to keep this situation. In this simpilified version, there is nothing interesting with regular file/dir install, we just "follow" symlinks and extract /path/file as /path/file. We just need to be careful during dir_symlink(->dir_pointed) install, when dir_symlink exists and it is a regular dir (else we create it or stop with file conflict): then we simply move the content of dir_symlink to dir_pointed, and replace dir_symlink with a symlink (->dir_pointed) [WARNING: more sophisticated file conflict checking needed here before the commit!]. You can see, that we didn't break our goal: all files in /dir_symlink can be accessed in the same way, the same for /dir_pointed. The removal is similar: Nothing extra with regular file/dir removal. If you remove a dir_symlink, you must scan the db for files which must be moved to the newly created regular dir named dir_symlink to keep our goal. Bye, ngaba ---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program.
Well this is how dpkg does it (src/processarc.c:471) As I see, this won't fix our problems...
---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program.
2007/7/4, ngaba@petra.hos.u-szeged.hu <ngaba@petra.hos.u-szeged.hu>:
Well this is how dpkg does it (src/processarc.c:471) As I see, this won't fix our problems...
I'm confused about what is the actual problem, and what's the solution you propose for fixing it. Anyway, the behavior you described that pacman now overwrites directories symlinks instead of following them is a serious problem, which has been reported there : http://bugs.archlinux.org/task/7484 The user who reported it said he was holding off any package upgrades, for avoiding to break other existing symlinks. What about trying to fix this problem first (ie reverting to the old behavior pacman had with libarchive 1.3.1), and then worry later about the smaller problems the old behavior had ? (ie losing track of files when a symlink is deleted, which shouldn't happen anyway)
I'm confused about what is the actual problem, and what's the solution you propose for fixing it. OK, let me explain (again.-): We have a theoretical coolplayer package, which have the following naming scheme: /usr/lib/coolplayer/2.0/ ... and also have a /usr/lib/coolplayer/current symlink, which points to 2.0. Package coolplugin installs a file to /usr/lib/coolplayer/current (so to /usr/lib/coolplayer/2.0). pacman -S updates coolplayer 2.0 to 2.1, which is a remove then install process. So the symlink /usr/lib/coolplayer/current is deleted and then set to 2.1 dir. And now the content of coolplugin is in /usr/lib/coolplayer/2.0, the package is broken, some files are out of pacman's control! Bye, ngaba
---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program.
2007/7/4, ngaba@petra.hos.u-szeged.hu <ngaba@petra.hos.u-szeged.hu>:
OK, let me explain (again.-): We have a theoretical coolplayer package, which have the following naming scheme: /usr/lib/coolplayer/2.0/ ... and also have a /usr/lib/coolplayer/current symlink, which points to 2.0. Package coolplugin installs a file to /usr/lib/coolplayer/current (so to /usr/lib/coolplayer/2.0). pacman -S updates coolplayer 2.0 to 2.1, which is a remove then install process. So the symlink /usr/lib/coolplayer/current is deleted and then set to 2.1 dir. And now the content of coolplugin is in /usr/lib/coolplayer/2.0, the package is broken, some files are out of pacman's control! Bye, ngaba
Ok, I see what you mean now. But having a package that does this is just silly. If a package contains a symlink where other packages (like plugins) put files, then that symlink shouldn't be changed, otherwise it'll indeed break things. I'm not sure there are situations where this is absolutely needed. But if this happens, then it can be up to the package to fix it itself, for example via the install scriptlet. Similarly to when an user makes a symlink /opt/ -> /usr/local/ , and later removes that symlink, he has to moves back all the files that were installed in /usr/local/ to /opt/ again. Now, maybe pacman should still handle that kind of situation, but I think that problem is more unusual and less critical than the other (overwriting symlinks).
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 From: Andrew Fyfe <andrew@neptune-one.net> Date: Sun, 8 Jul 2007 00:02:48 +0000 (+0100) Subject: FS#7484 - pacman clobbers directory symlinks. X-Git-Url: http://lithium.sol/gitweb/?p=pacman;a=commitdiff_plain;h=c992a5d757efa5aee45... FS#7484 - pacman clobbers directory symlinks. If a file/directory/symlink does not already exist on the filesystem, then it is extracted as normal. When extracting a directory: - If a directory or symlink already exists on the filesystem then the directory is not extracted. When extracting a symlink: - If a directory already exists on the filesystem then the symlink is not extracted. When extracting a special file (FIFO, block dev or char dev): - If the file it is replacing is a special file no backup checks are done. For all other cases when a file exists on the filesystem the normal backuup checks are performed. Signed-off-by: Andrew Fyfe <andrew@neptune-one.net> --- diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 870d1f8..62bc2f7 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -439,6 +439,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) int needbackup = 0, notouch = 0; char *hash_orig = NULL; struct stat buf; + mode_t entrytype = archive_entry_filetype(entry); entryname = archive_entry_pathname(entry); @@ -459,11 +460,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) memset(filename, 0, PATH_MAX); /* just to be sure */ - if(strcmp(entryname, ".PKGINFO") == 0 - || strcmp(entryname, ".FILELIST") == 0) { - archive_read_data_skip(archive); - continue; - } else if(strcmp(entryname, ".INSTALL") == 0) { + if(strcmp(entryname, ".INSTALL") == 0) { /* the install script goes inside the db */ snprintf(filename, PATH_MAX, "%s/%s-%s/install", db->path, newpkg->name, newpkg->version); @@ -498,31 +495,56 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) } /* check is file already exists */ - if(stat(filename, &buf) == 0 && !S_ISDIR(buf.st_mode)) { - /* it does, is it a backup=() file? - * always check the newpkg first, so when we do add a backup=() file, - * we don't have to wait a full upgrade cycle */ - needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); - - if(is_upgrade) { - hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); - if(hash_orig) { - needbackup = 1; - } + if(stat(filename, &buf) == 0) { + if ( S_ISDIR(entrytype) && (S_ISDIR(buf.st_mode) || S_ISLNK(buf.st_mode)) ) { + /* If the package entry is a directory and there's a directory or + * symlink with the same name on the filesystem, skip this entry. */ + _alpm_log(PM_LOG_DEBUG, _("%s (directory) won't be extracted over existing directory or symlink"), entryname); + archive_read_data_skip(archive); + continue; } - /* this is kind of gross. if we force hash_orig to be non-NULL we can - * catch the pro-active backup=() case (when the backup entry is in - * the new package, and not the old */ - if(needbackup && !hash_orig) { - hash_orig = strdup(""); + if ( S_ISLNK(entrytype) && S_ISDIR(buf.st_mode) ) { + /* If the package entry is a symlink and the fs entry is a + * directory, skip this entry. */ + _alpm_log(PM_LOG_DEBUG, _("%s (symlink) won't be extracted over existing directory."), entryname); + archive_read_data_skip(archive); + continue; } - - /* NoUpgrade skips all this backup stuff, because it's just never - * touched */ - if(alpm_list_find_str(handle->noupgrade, entryname)) { - notouch = 1; - needbackup = 0; + + if ( (S_ISFIFO(entrytype) || S_ISBLK(entrytype) || S_ISCHR(entrytype)) && + (S_ISFIFO(buf.st_mode) || S_ISBLK(buf.st_mode) || S_ISCHR(buf.st_mode)) ) { + /* If the package entry is a FIFO, block device or character device + * and the filesystem entry is a FIFO, block devicce or character device + * unlink the fs entry and extract the package entry. */ + _alpm_log(PM_LOG_DEBUG, _("%s is a FIFO/block/character device, extracting as normal."), entryname); + } + else { + /* it does, is it a backup=() file? + * always check the newpkg first, so when we do add a backup=() file, + * we don't have to wait a full upgrade cycle */ + needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); + + if(is_upgrade) { + hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); + if(hash_orig) { + needbackup = 1; + } + } + + /* this is kind of gross. if we force hash_orig to be non-NULL we can + * catch the pro-active backup=() case (when the backup entry is in + * the new package, and not the old */ + if(needbackup && !hash_orig) { + hash_orig = strdup(""); + } + + /* NoUpgrade skips all this backup stuff, because it's just never + * touched */ + if(alpm_list_find_str(handle->noupgrade, entryname)) { + notouch = 1; + needbackup = 0; + } } }
On Sun, Jul 08, 2007 at 01:44:46AM +0100, Andrew Fyfe wrote:
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
From: Andrew Fyfe <andrew@neptune-one.net> Date: Sun, 8 Jul 2007 00:02:48 +0000 (+0100) Subject: FS#7484 - pacman clobbers directory symlinks. X-Git-Url: http://lithium.sol/gitweb/?p=pacman;a=commitdiff_plain;h=c992a5d757efa5aee45...
FS#7484 - pacman clobbers directory symlinks.
If a file/directory/symlink does not already exist on the filesystem, then it is extracted as normal.
When extracting a directory: - If a directory or symlink already exists on the filesystem then the directory is not extracted.
When extracting a symlink: - If a directory already exists on the filesystem then the symlink is not extracted.
That should be used without the NO_OVERWRITE flag ? As I said in the bug report at the beginning, I was hesitating between doing the checks manually, or trying to configure libarchive like we want. I guess the advantage of the first way is that it gives us more control. I'm just realizing there are many odd cases I didn't consider, eg extracting a directory when a file already exists on the filesystem. Using only NO_OVERWRITE, the directory will never be extracted, while with the behavior above, it would be extracted.
When extracting a special file (FIFO, block dev or char dev): - If the file it is replacing is a special file no backup checks are done.
Isn't this handled by filesystem conflicts already ? in libalpm/conflict.c , _alpm_db_find_conflicts. In this case, I think pacman will find a conflict, and so just fail.
For all other cases when a file exists on the filesystem the normal backuup checks are performed.
Signed-off-by: Andrew Fyfe <andrew@neptune-one.net> ---
diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 870d1f8..62bc2f7 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -439,6 +439,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) int needbackup = 0, notouch = 0; char *hash_orig = NULL; struct stat buf; + mode_t entrytype = archive_entry_filetype(entry);
entryname = archive_entry_pathname(entry);
@@ -459,11 +460,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db)
memset(filename, 0, PATH_MAX); /* just to be sure */
- if(strcmp(entryname, ".PKGINFO") == 0 - || strcmp(entryname, ".FILELIST") == 0) { - archive_read_data_skip(archive); - continue; - } else if(strcmp(entryname, ".INSTALL") == 0) { + if(strcmp(entryname, ".INSTALL") == 0) { /* the install script goes inside the db */ snprintf(filename, PATH_MAX, "%s/%s-%s/install", db->path, newpkg->name, newpkg->version); @@ -498,31 +495,56 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) }
/* check is file already exists */ - if(stat(filename, &buf) == 0 && !S_ISDIR(buf.st_mode)) { - /* it does, is it a backup=() file? - * always check the newpkg first, so when we do add a backup=() file, - * we don't have to wait a full upgrade cycle */ - needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); - - if(is_upgrade) { - hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); - if(hash_orig) { - needbackup = 1; - } + if(stat(filename, &buf) == 0) { + if ( S_ISDIR(entrytype) && (S_ISDIR(buf.st_mode) || S_ISLNK(buf.st_mode)) ) { + /* If the package entry is a directory and there's a directory or + * symlink with the same name on the filesystem, skip this entry. */ + _alpm_log(PM_LOG_DEBUG, _("%s (directory) won't be extracted over existing directory or symlink"), entryname); + archive_read_data_skip(archive); + continue; }
Is S_ISLNK ever true when using stat? I think you want to use lstat there, if you want to check for symlinks :) Also, we could check for permission differences there, as suggested in the BR. There is also still the problem with the tmp/ directory, which is first created by pacman with the wrong permissions, and which will keep these wrong permissions if we don't update it there when installing the filesystem package. But that problem could be fixed at the places where pacman creates tmp/
- /* this is kind of gross. if we force hash_orig to be non-NULL we can - * catch the pro-active backup=() case (when the backup entry is in - * the new package, and not the old */ - if(needbackup && !hash_orig) { - hash_orig = strdup(""); + if ( S_ISLNK(entrytype) && S_ISDIR(buf.st_mode) ) { + /* If the package entry is a symlink and the fs entry is a + * directory, skip this entry. */ + _alpm_log(PM_LOG_DEBUG, _("%s (symlink) won't be extracted over existing directory."), entryname); + archive_read_data_skip(archive); + continue; } - - /* NoUpgrade skips all this backup stuff, because it's just never - * touched */ - if(alpm_list_find_str(handle->noupgrade, entryname)) { - notouch = 1; - needbackup = 0; + + if ( (S_ISFIFO(entrytype) || S_ISBLK(entrytype) || S_ISCHR(entrytype)) && + (S_ISFIFO(buf.st_mode) || S_ISBLK(buf.st_mode) || S_ISCHR(buf.st_mode)) ) { + /* If the package entry is a FIFO, block device or character device + * and the filesystem entry is a FIFO, block devicce or character device + * unlink the fs entry and extract the package entry. */ + _alpm_log(PM_LOG_DEBUG, _("%s is a FIFO/block/character device, extracting as normal."), entryname); + }
As I said above, won't pacman find a conflict in this case ? Maybe when using the --force flag then, that condition could be true. And does the code below apply to every cases except the one above ?
+ else { + /* it does, is it a backup=() file? + * always check the newpkg first, so when we do add a backup=() file, + * we don't have to wait a full upgrade cycle */ + needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); + + if(is_upgrade) { + hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); + if(hash_orig) { + needbackup = 1; + } + } + + /* this is kind of gross. if we force hash_orig to be non-NULL we can + * catch the pro-active backup=() case (when the backup entry is in + * the new package, and not the old */ + if(needbackup && !hash_orig) { + hash_orig = strdup(""); + } + + /* NoUpgrade skips all this backup stuff, because it's just never + * touched */ + if(alpm_list_find_str(handle->noupgrade, entryname)) { + notouch = 1; + needbackup = 0; + } } }
Xavier wrote: <snip>
When extracting a symlink: - If a directory already exists on the filesystem then the symlink is not extracted.
That should be used without the NO_OVERWRITE flag ? Oops I knew I forgot something, this patch should remove the need for NO_OVERWRITE.
As I said in the bug report at the beginning, I was hesitating between doing the checks manually, or trying to configure libarchive like we want. I guess the advantage of the first way is that it gives us more control. This is more a pacman problem than a libarchive problem.
I'm just realizing there are many odd cases I didn't consider, eg extracting a directory when a file already exists on the filesystem. Using only NO_OVERWRITE, the directory will never be extracted, while with the behavior above, it would be extracted. I forgot about this as well, I need to go do a little more testing :)
<snip>
+ if(stat(filename, &buf) == 0) { + if ( S_ISDIR(entrytype) && (S_ISDIR(buf.st_mode) || S_ISLNK(buf.st_mode)) ) { + /* If the package entry is a directory and there's a directory or + * symlink with the same name on the filesystem, skip this entry. */ + _alpm_log(PM_LOG_DEBUG, _("%s (directory) won't be extracted over existing directory or symlink"), entryname); + archive_read_data_skip(archive); + continue; }
Is S_ISLNK ever true when using stat? I think you want to use lstat there, if you want to check for symlinks :) Ahhh I didn't know that, I'll give lstat a try.
Also, we could check for permission differences there, as suggested in the BR. Permissions are the next problem I need to tackle.
There is also still the problem with the tmp/ directory, which is first created by pacman with the wrong permissions, and which will keep these wrong permissions if we don't update it there when installing the filesystem package. But that problem could be fixed at the places where pacman creates tmp/ This is a pacman problem, it should set the correct permissions when creating tmp.
<snip>
+ + if ( (S_ISFIFO(entrytype) || S_ISBLK(entrytype) || S_ISCHR(entrytype)) && + (S_ISFIFO(buf.st_mode) || S_ISBLK(buf.st_mode) || S_ISCHR(buf.st_mode)) ) { + /* If the package entry is a FIFO, block device or character device + * and the filesystem entry is a FIFO, block devicce or character device + * unlink the fs entry and extract the package entry. */ + _alpm_log(PM_LOG_DEBUG, _("%s is a FIFO/block/character device, extracting as normal."), entryname); + }
As I said above, won't pacman find a conflict in this case ? Maybe when using the --force flag then, that condition could be true. Yes this probably is caught by the conflict check, it's mainly to avoid doing pointless backup checks on special files. But it still does the backup checks if a special file is replacing a regular file.
And does the code below apply to every cases except the one above ? Yes, This could probably still be refined a little.
+ else { + /* it does, is it a backup=() file? + * always check the newpkg first, so when we do add a backup=() file, + * we don't have to wait a full upgrade cycle */ + needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); + + if(is_upgrade) { + hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); + if(hash_orig) { + needbackup = 1; + } + } + + /* this is kind of gross. if we force hash_orig to be non-NULL we can + * catch the pro-active backup=() case (when the backup entry is in + * the new package, and not the old */ + if(needbackup && !hash_orig) { + hash_orig = strdup(""); + } + + /* NoUpgrade skips all this backup stuff, because it's just never + * touched */ + if(alpm_list_find_str(handle->noupgrade, entryname)) { + notouch = 1; + needbackup = 0; + } } }
_______________________________________________ pacman-dev mailing list pacman-dev@archlinux.org http://archlinux.org/mailman/listinfo/pacman-dev
On Sun, Jul 08, 2007 at 07:36:00PM +0100, Andrew Fyfe wrote:
There is also still the problem with the tmp/ directory, which is first created by pacman with the wrong permissions, and which will keep these wrong permissions if we don't update it there when installing the filesystem package. But that problem could be fixed at the places where pacman creates tmp/ This is a pacman problem, it should set the correct permissions when creating tmp.
Right. I still find it odd that pacman requires the /tmp/ directory (which belongs to the filesystem package) for installing the filesystem package. But well :)
And does the code below apply to every cases except the one above ? Yes, This could probably still be refined a little.
Don't we need regular files on both side (old file on filesystem, and new file from the archive) for computing md5sums anyway ?
Andrew Fyfe wrote:
Xavier wrote:
I'm just realizing there are many odd cases I didn't consider, eg extracting a directory when a file already exists on the filesystem. Using only NO_OVERWRITE, the directory will never be extracted, while with the behavior above, it would be extracted. I forgot about this as well, I need to go do a little more testing :)
Without NO_OVERWRITE it always overwrites (DUH :P), pkg = file/dir in package fs = file/dir on filesystem if isfile(pkg) && isdir(fs) then skip if isdir(pkg) && isfile(fs) then extract Does that look ok or should we skip on both cases? Andrew
On Sun, Jul 08, 2007 at 08:52:48PM +0100, Andrew Fyfe wrote:
Andrew Fyfe wrote:
Xavier wrote:
I'm just realizing there are many odd cases I didn't consider, eg extracting a directory when a file already exists on the filesystem. Using only NO_OVERWRITE, the directory will never be extracted, while with the behavior above, it would be extracted. I forgot about this as well, I need to go do a little more testing :)
Without NO_OVERWRITE it always overwrites (DUH :P),
pkg = file/dir in package fs = file/dir on filesystem
if isfile(pkg) && isdir(fs) then skip if isdir(pkg) && isfile(fs) then extract
Does that look ok or should we skip on both cases?
If we skip the second case, and the package has some files in file/dir, then this is going to be weird :) But what I don't get is why doesn't it totally fail in both cases ? Shouldn't there be a conflict between a regular file and a directory ? It looks like this isn't the case currently. Apparently, as long as there is a directory on one side, conflict check are skipped. So there might be a reason for that, I'm not sure.. I think it's rather between a symlink to a directory and a directory that we can choose between skipping and extracting, but I could be wrong.
Xavier wrote:
On Sun, Jul 08, 2007 at 08:52:48PM +0100, Andrew Fyfe wrote:
Andrew Fyfe wrote:
Xavier wrote:
I'm just realizing there are many odd cases I didn't consider, eg extracting a directory when a file already exists on the filesystem. Using only NO_OVERWRITE, the directory will never be extracted, while with the behavior above, it would be extracted. I forgot about this as well, I need to go do a little more testing :)
Without NO_OVERWRITE it always overwrites (DUH :P),
pkg = file/dir in package fs = file/dir on filesystem
if isfile(pkg) && isdir(fs) then skip if isdir(pkg) && isfile(fs) then extract
Does that look ok or should we skip on both cases?
If we skip the second case, and the package has some files in file/dir, then this is going to be weird :) We could move the file to *.pacold?
But what I don't get is why doesn't it totally fail in both cases ? Shouldn't there be a conflict between a regular file and a directory ? It looks like this isn't the case currently. Apparently, as long as there is a directory on one side, conflict check are skipped. So there might be a reason for that, I'm not sure.. Yes if either side is a directory it's missed by the conflict checks. I think the conflict checks need a similar solution to my patch for add_commit().
I think it's rather between a symlink to a directory and a directory that we can choose between skipping and extracting, but I could be wrong. I don't understand (and a directory??), extracting a symlink over a directory should NOT happen.
Slightly OT: Some work is still needed on permissions as well, I've just noticed usr/man/man{1,4} are 0700 :@, not sure what package did it.
This is an updated patch that fixes the issues Xavier brought up. Andrew
If a file/directory/symlink does not already exist on the filesystem, then it is extracted as normal. Never extract a package entry over a directory. When extracting a directory: - If a symlink already exists on the filesystem then the directory is not extracted. BUT if the symlink does not point to a directory we have a problem! What do we do in this situation? If we abort or continue we can end up breaking the system! When extracting a symlink: - If a directory already exists on the filesystem then the symlink is not extracted. When extracting a reqular file - Do the normal backup checks. For all other cases the conflict checks have probably failed. Signed-off-by: Andrew Fyfe <andrew@neptune-one.net> --- lib/libalpm/add.c | 97 +++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 69 insertions(+), 28 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 870d1f8..dc9b1e6 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -439,6 +439,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) int needbackup = 0, notouch = 0; char *hash_orig = NULL; struct stat buf; + mode_t entrytype = archive_entry_filetype(entry); entryname = archive_entry_pathname(entry); @@ -459,11 +460,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) memset(filename, 0, PATH_MAX); /* just to be sure */ - if(strcmp(entryname, ".PKGINFO") == 0 - || strcmp(entryname, ".FILELIST") == 0) { - archive_read_data_skip(archive); - continue; - } else if(strcmp(entryname, ".INSTALL") == 0) { + if(strcmp(entryname, ".INSTALL") == 0) { /* the install script goes inside the db */ snprintf(filename, PATH_MAX, "%s/%s-%s/install", db->path, newpkg->name, newpkg->version); @@ -497,33 +494,77 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) continue; } - /* check is file already exists */ - if(stat(filename, &buf) == 0 && !S_ISDIR(buf.st_mode)) { - /* it does, is it a backup=() file? - * always check the newpkg first, so when we do add a backup=() file, - * we don't have to wait a full upgrade cycle */ - needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); - - if(is_upgrade) { - hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); - if(hash_orig) { - needbackup = 1; + if(lstat(filename, &buf) == 0) { + /* Entry exists on filesystem... */ + + if(S_ISDIR(buf.st_mode)) { + /* Don't extract anything over an existing directory.*/ + if(!S_ISDIR(entrytype)) { + _alpm_log(PM_LOG_WARNING, _("%s won't be extracted over an existing directory."), entryname); } + + _alpm_log(PM_LOG_DEBUG, _("%s won't be extracted over an existing directory."), entryname); + archive_read_data_skip(archive); + continue; } - /* this is kind of gross. if we force hash_orig to be non-NULL we can - * catch the pro-active backup=() case (when the backup entry is in - * the new package, and not the old */ - if(needbackup && !hash_orig) { - hash_orig = strdup(""); + if(S_ISDIR(entrytype) && S_ISLNK(buf.st_mode)) { + /* Don't extract a directory over an existing symlink. */ + if(stat(filename, &buf) == 0 && !S_ISDIR(buf.st_mode)) { + /* Something's going to break! A symlink exists where we + * want to extract this directory, and it doesn't point to + * a directory. + * + * This will cause problems if this package or any other + * package try to extract files to this directory. */ + + /* TODO: How do we handle this? If we continue we leeave a + * broken system, if we abort we leave a broken system! */ + _alpm_log(PM_LOG_ERROR, _("%s is a symlink, but it does not point to a directory!"), entryname); + } + + _alpm_log(PM_LOG_DEBUG, _("%s (directory) won't be extracted over existing symlink."), entryname); + archive_read_data_skip(archive); + continue; } - - /* NoUpgrade skips all this backup stuff, because it's just never - * touched */ - if(alpm_list_find_str(handle->noupgrade, entryname)) { - notouch = 1; - needbackup = 0; + + if(S_ISLNK(entrytype) && S_ISDIR(buf.st_mode)) { + /* Don't extract a symlink over a directory. */ + _alpm_log(PM_LOG_DEBUG, _("%s (symlink) won't be extracted over existing directory."), entryname); + archive_read_data_skip(archive); + continue; } + + if(S_ISREG(entrytype)) { + /* always check the newpkg first, so when we do add a backup=() file, + * we don't have to wait a full upgrade cycle */ + needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); + + if(is_upgrade) { + hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); + if(hash_orig) { + needbackup = 1; + } + } + + /* this is kind of gross. if we force hash_orig to be non-NULL we can + * catch the pro-active backup=() case (when the backup entry is in + * the new package, and not the old */ + if(needbackup && !hash_orig) { + hash_orig = strdup(""); + } + + /* NoUpgrade skips all this backup stuff, because it's just never + * touched */ + if(alpm_list_find_str(handle->noupgrade, entryname)) { + notouch = 1; + needbackup = 0; + } + } + /* else { + * we shouldn't be here! This should have already been + * caught by the conflict check. + * } */ } if(needbackup) { @@ -705,7 +746,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) * 1) if it's an upgrade, existing files are removed when the old pkg is removed * 2) if there is a file conflict, but --force is used, then files are also removed : see above */ - int ret = archive_read_extract(archive, entry, archive_flags | ARCHIVE_EXTRACT_NO_OVERWRITE); + int ret = archive_read_extract(archive, entry, archive_flags); if(ret == ARCHIVE_WARN) { /* operation succeeded but a non-critical error was encountered */ _alpm_log(PM_LOG_DEBUG, _("warning extracting %s (%s)"), -- 1.5.2.3
On 7/8/07, Andrew Fyfe <andrew@neptune-one.net> wrote: First off, thanks for stepping up to the plate and starting to write this patch up. To be honest this issue hasn't interested me and thats the main reason I haven't worked to fix it.
If a file/directory/symlink does not already exist on the filesystem, then it is extracted as normal.
Never extract a package entry over a directory.
When extracting a directory: - If a symlink already exists on the filesystem then the directory is not extracted.
BUT if the symlink does not point to a directory we have a problem! What do we do in this situation? If we abort or continue we can end up breaking the system!
Oh hard cases. We need to be able to check this, and fail as early as possible, preferably before the old package has been removed and the new package has been partially installed. This is when I begin to hate the current structure of the libalpm code. Can we do an early loop of the filelist in order to check these kind of things, before we start actually extracting files? Perhaps when we are verifying the archive integrity (which is during the package loading process), although I think this would break a few abstraction barriers so that isn't the best solution either.
When extracting a symlink: - If a directory already exists on the filesystem then the symlink is not extracted.
When extracting a reqular file - Do the normal backup checks.
For all other cases the conflict checks have probably failed.
I've never been a fan of the --force option, but we do need to keep this one in mind here so assumptions of this sort could be bad.
Signed-off-by: Andrew Fyfe <andrew@neptune-one.net> --- lib/libalpm/add.c | 97 +++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 69 insertions(+), 28 deletions(-)
diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 870d1f8..dc9b1e6 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -439,6 +439,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) int needbackup = 0, notouch = 0; char *hash_orig = NULL; struct stat buf; + mode_t entrytype = archive_entry_filetype(entry);
entryname = archive_entry_pathname(entry);
@@ -459,11 +460,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db)
memset(filename, 0, PATH_MAX); /* just to be sure */
- if(strcmp(entryname, ".PKGINFO") == 0 - || strcmp(entryname, ".FILELIST") == 0) { - archive_read_data_skip(archive); - continue; - } else if(strcmp(entryname, ".INSTALL") == 0) { + if(strcmp(entryname, ".INSTALL") == 0) { /* the install script goes inside the db */ snprintf(filename, PATH_MAX, "%s/%s-%s/install", db->path, newpkg->name, newpkg->version); @@ -497,33 +494,77 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) continue; }
- /* check is file already exists */ - if(stat(filename, &buf) == 0 && !S_ISDIR(buf.st_mode)) { - /* it does, is it a backup=() file? - * always check the newpkg first, so when we do add a backup=() file, - * we don't have to wait a full upgrade cycle */ - needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); - - if(is_upgrade) { - hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); - if(hash_orig) { - needbackup = 1; + if(lstat(filename, &buf) == 0) { + /* Entry exists on filesystem... */ + + if(S_ISDIR(buf.st_mode)) { + /* Don't extract anything over an existing directory.*/ + if(!S_ISDIR(entrytype)) { + _alpm_log(PM_LOG_WARNING, _("%s won't be extracted over an existing directory."), entryname); } + + _alpm_log(PM_LOG_DEBUG, _("%s won't be extracted over an existing directory."), entryname); + archive_read_data_skip(archive); + continue; }
- /* this is kind of gross. if we force hash_orig to be non-NULL we can - * catch the pro-active backup=() case (when the backup entry is in - * the new package, and not the old */ - if(needbackup && !hash_orig) { - hash_orig = strdup(""); + if(S_ISDIR(entrytype) && S_ISLNK(buf.st_mode)) { + /* Don't extract a directory over an existing symlink. */ + if(stat(filename, &buf) == 0 && !S_ISDIR(buf.st_mode)) { + /* Something's going to break! A symlink exists where we + * want to extract this directory, and it doesn't point to + * a directory. + * + * This will cause problems if this package or any other + * package try to extract files to this directory. */ + + /* TODO: How do we handle this? If we continue we leeave a + * broken system, if we abort we leave a broken system! */ + _alpm_log(PM_LOG_ERROR, _("%s is a symlink, but it does not point to a directory!"), entryname); + }
Looking at this now, perhaps we should print out a big warning, and then kill the dead symlink and put the files where they are supposed to be?
+ + _alpm_log(PM_LOG_DEBUG, _("%s (directory) won't be extracted over existing symlink."), entryname); + archive_read_data_skip(archive); + continue; } - - /* NoUpgrade skips all this backup stuff, because it's just never - * touched */ - if(alpm_list_find_str(handle->noupgrade, entryname)) { - notouch = 1; - needbackup = 0; + + if(S_ISLNK(entrytype) && S_ISDIR(buf.st_mode)) { + /* Don't extract a symlink over a directory. */ + _alpm_log(PM_LOG_DEBUG, _("%s (symlink) won't be extracted over existing directory."), entryname); + archive_read_data_skip(archive); + continue; } + + if(S_ISREG(entrytype)) { + /* always check the newpkg first, so when we do add a backup=() file, + * we don't have to wait a full upgrade cycle */ + needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); + + if(is_upgrade) { + hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); + if(hash_orig) { + needbackup = 1; + } + } + + /* this is kind of gross. if we force hash_orig to be non-NULL we can + * catch the pro-active backup=() case (when the backup entry is in + * the new package, and not the old */ + if(needbackup && !hash_orig) { + hash_orig = strdup(""); + } + + /* NoUpgrade skips all this backup stuff, because it's just never + * touched */ + if(alpm_list_find_str(handle->noupgrade, entryname)) { + notouch = 1; + needbackup = 0; + } + } + /* else { + * we shouldn't be here! This should have already been + * caught by the conflict check. + * } */
If we shouldn't be here, we should print a big fat error too and bail if possible, or do something sane. Having "unreachable code" isn't a bad thing- it makes future debugging easier, and like I said above, the --force option could get us to this code.
}
if(needbackup) { @@ -705,7 +746,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) * 1) if it's an upgrade, existing files are removed when the old pkg is removed * 2) if there is a file conflict, but --force is used, then files are also removed : see above */ - int ret = archive_read_extract(archive, entry, archive_flags | ARCHIVE_EXTRACT_NO_OVERWRITE); + int ret = archive_read_extract(archive, entry, archive_flags); if(ret == ARCHIVE_WARN) { /* operation succeeded but a non-critical error was encountered */ _alpm_log(PM_LOG_DEBUG, _("warning extracting %s (%s)"), -- 1.5.2.3
Thanks again Andrew for looking into this. -Dan
On Mon, Jul 09, 2007 at 12:24:23AM -0400, Dan McGee wrote:
Can we do an early loop of the filelist in order to check these kind of things, before we start actually extracting files? Perhaps when we are verifying the archive integrity (which is during the package loading process), although I think this would break a few abstraction barriers so that isn't the best solution either.
Don't we already have the filesystem conflict check in conflict.c for this? Actually, I saw just wondering how it could work, since we didn't look at the new files from the archive yet. But I forgot about the .FILELIST file, that's probably what is used. Maybe it could also help here to have .FILELIST extended, like Andrew suggested in another mail.
I've replaced Xavier's comment about ARCHIVE_EXTRACT_NO_OVERWRITE, and put in the final else case (as Dan suggested). I ran add012 and add013 and it didn't reached the else case. After 3.1 I'll start looking at fixing the real issue in the conflict checks. Andrew
If a file/directory/symlink does not already exist on the filesystem, then it is extracted as normal. Never extract a package entry over a directory. When extracting a directory: - If a symlink already exists on the filesystem then the directory is not extracted. BUT if the symlink does not point to a directory we have a problem! What do we do in this situation? If we abort or continue we can end up breaking the system! When extracting a symlink: - If a directory already exists on the filesystem then the symlink is not extracted. When extracting a reqular file - Do the normal backup checks. For all other cases the conflict checks have probably failed. Signed-off-by: Andrew Fyfe <andrew@neptune-one.net> --- lib/libalpm/add.c | 110 ++++++++++++++++++++++++++++++++++++---------------- 1 files changed, 76 insertions(+), 34 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index b4345e9..81be53e 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -443,6 +443,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) int needbackup = 0, notouch = 0; char *hash_orig = NULL; struct stat buf; + mode_t entrytype = archive_entry_filetype(entry); entryname = archive_entry_pathname(entry); @@ -464,11 +465,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) memset(filename, 0, PATH_MAX); /* just to be sure */ - if(strcmp(entryname, ".PKGINFO") == 0 - || strcmp(entryname, ".FILELIST") == 0) { - archive_read_data_skip(archive); - continue; - } else if(strcmp(entryname, ".INSTALL") == 0) { + if(strcmp(entryname, ".INSTALL") == 0) { /* the install script goes inside the db */ snprintf(filename, PATH_MAX, "%s/%s-%s/install", db->path, newpkg->name, newpkg->version); @@ -503,32 +500,79 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) continue; } - /* check is file already exists */ - if(stat(filename, &buf) == 0 && !S_ISDIR(buf.st_mode)) { - /* it does, is it a backup=() file? - * always check the newpkg first, so when we do add a backup=() file, - * we don't have to wait a full upgrade cycle */ - needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); - - if(is_upgrade) { - hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); - if(hash_orig) { - needbackup = 1; + if(lstat(filename, &buf) == 0) { + /* Entry exists on filesystem... */ + + if(S_ISDIR(buf.st_mode)) { + /* Don't extract anything over an existing directory.*/ + if(!S_ISDIR(entrytype)) { + _alpm_log(PM_LOG_WARNING, _("%s won't be extracted over an existing directory."), entryname); } + + _alpm_log(PM_LOG_DEBUG, _("%s won't be extracted over an existing directory."), entryname); + archive_read_data_skip(archive); + continue; } - /* this is kind of gross. if we force hash_orig to be non-NULL we can - * catch the pro-active backup=() case (when the backup entry is in - * the new package, and not the old */ - if(needbackup && !hash_orig) { - hash_orig = strdup(""); + if(S_ISDIR(entrytype) && S_ISLNK(buf.st_mode)) { + /* Don't extract a directory over an existing symlink. */ + if(stat(filename, &buf) == 0 && !S_ISDIR(buf.st_mode)) { + /* Something's going to break! A symlink exists where we + * want to extract this directory, and it doesn't point to + * a directory. + * + * This will cause problems if this package or any other + * package try to extract files to this directory. */ + + /* TODO: How do we handle this? If we continue we leeave a + * broken system, if we abort we leave a broken system! */ + _alpm_log(PM_LOG_ERROR, _("%s is a symlink, but it does not point to a directory!"), entryname); + } + + _alpm_log(PM_LOG_DEBUG, _("%s (directory) won't be extracted over existing symlink."), entryname); + archive_read_data_skip(archive); + continue; } - - /* NoUpgrade skips all this backup stuff, because it's just never - * touched */ - if(alpm_list_find_str(handle->noupgrade, entryname)) { - notouch = 1; - needbackup = 0; + + if(S_ISLNK(entrytype) && S_ISDIR(buf.st_mode)) { + /* Don't extract a symlink over a directory. */ + _alpm_log(PM_LOG_DEBUG, _("%s (symlink) won't be extracted over existing directory."), entryname); + archive_read_data_skip(archive); + continue; + } + + if(S_ISREG(entrytype)) { + /* always check the newpkg first, so when we do add a backup=() file, + * we don't have to wait a full upgrade cycle */ + needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); + + if(is_upgrade) { + hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); + if(hash_orig) { + needbackup = 1; + } + } + + /* this is kind of gross. if we force hash_orig to be non-NULL we can + * catch the pro-active backup=() case (when the backup entry is in + * the new package, and not the old */ + if(needbackup && !hash_orig) { + hash_orig = strdup(""); + } + + /* NoUpgrade skips all this backup stuff, because it's just never + * touched */ + if(alpm_list_find_str(handle->noupgrade, entryname)) { + notouch = 1; + needbackup = 0; + } + } + else { + /* We shouldn't be here! This should have already been + * caught by the conflict check. */ + _alpm_log(PM_LOG_ERROR, _("%s already exists on tthe filesystem."), entryname); + _alpm_log(PM_LOG_DEBUG, _("%s should have been caught by the conflict checks!"), entryname); + RET_ERR(PM_ERR_FILE_CONFLICTS, -1); } } @@ -705,14 +749,12 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) archive_entry_set_pathname(entry, filename); /* FS #7484 - * By default, libarchive 2.2.3 overwrites existing symlinks by directories from the archive, - * which isn't the behavior we want. - * This can be avoided by using the ARCHIVE_EXTRACT_NO_OVERWRITE flag, and this works - * fine because all files where an overwrite could be needed are deleted first : - * 1) if it's an upgrade, existing files are removed when the old pkg is removed - * 2) if there is a file conflict, but --force is used, then files are also removed : see above + * Andrew Fyfe: + * Removed ARCHIVE_EXTRACT_NO_OVERWRITE from the next command. + * The new checks above should catch all the directory extract/ + * overwrite issues for now. */ - int ret = archive_read_extract(archive, entry, archive_flags | ARCHIVE_EXTRACT_NO_OVERWRITE); + int ret = archive_read_extract(archive, entry, archive_flags); if(ret == ARCHIVE_WARN) { /* operation succeeded but a non-critical error was encountered */ _alpm_log(PM_LOG_DEBUG, "warning extracting %s (%s)", -- 1.5.2.3
On Thu, Jul 12, 2007 at 08:53:04PM +0100, Andrew Fyfe wrote:
+ else { + /* We shouldn't be here! This should have already been + * caught by the conflict check. */ + _alpm_log(PM_LOG_ERROR, _("%s already exists on tthe filesystem."), entryname); + _alpm_log(PM_LOG_DEBUG, _("%s should have been caught by the conflict checks!"), entryname); + RET_ERR(PM_ERR_FILE_CONFLICTS, -1); } }
little typo tthere :)
If a file/directory/symlink does not already exist on the filesystem, then it is extracted as normal. Never extract a package entry over a directory. When extracting a directory: - If a symlink already exists on the filesystem then the directory is not extracted. BUT if the symlink does not point to a directory we have a problem! What do we do in this situation? If we abort or continue we can end up breaking the system! When extracting a symlink: - If a directory already exists on the filesystem then the symlink is not extracted. When extracting a reqular file - Do the normal backup checks. For all other cases the conflict checks have probably failed. Signed-off-by: Andrew Fyfe <andrew@neptune-one.net> --- lib/libalpm/add.c | 110 ++++++++++++++++++++++++++++++++++++---------------- 1 files changed, 76 insertions(+), 34 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index b4345e9..b24cc00 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -443,6 +443,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) int needbackup = 0, notouch = 0; char *hash_orig = NULL; struct stat buf; + mode_t entrytype = archive_entry_filetype(entry); entryname = archive_entry_pathname(entry); @@ -464,11 +465,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) memset(filename, 0, PATH_MAX); /* just to be sure */ - if(strcmp(entryname, ".PKGINFO") == 0 - || strcmp(entryname, ".FILELIST") == 0) { - archive_read_data_skip(archive); - continue; - } else if(strcmp(entryname, ".INSTALL") == 0) { + if(strcmp(entryname, ".INSTALL") == 0) { /* the install script goes inside the db */ snprintf(filename, PATH_MAX, "%s/%s-%s/install", db->path, newpkg->name, newpkg->version); @@ -503,32 +500,79 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) continue; } - /* check is file already exists */ - if(stat(filename, &buf) == 0 && !S_ISDIR(buf.st_mode)) { - /* it does, is it a backup=() file? - * always check the newpkg first, so when we do add a backup=() file, - * we don't have to wait a full upgrade cycle */ - needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); - - if(is_upgrade) { - hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); - if(hash_orig) { - needbackup = 1; + if(lstat(filename, &buf) == 0) { + /* Entry exists on filesystem... */ + + if(S_ISDIR(buf.st_mode)) { + /* Don't extract anything over an existing directory.*/ + if(!S_ISDIR(entrytype)) { + _alpm_log(PM_LOG_WARNING, _("%s won't be extracted over an existing directory."), entryname); } + + _alpm_log(PM_LOG_DEBUG, _("%s won't be extracted over an existing directory."), entryname); + archive_read_data_skip(archive); + continue; } - /* this is kind of gross. if we force hash_orig to be non-NULL we can - * catch the pro-active backup=() case (when the backup entry is in - * the new package, and not the old */ - if(needbackup && !hash_orig) { - hash_orig = strdup(""); + if(S_ISDIR(entrytype) && S_ISLNK(buf.st_mode)) { + /* Don't extract a directory over an existing symlink. */ + if(stat(filename, &buf) == 0 && !S_ISDIR(buf.st_mode)) { + /* Something's going to break! A symlink exists where we + * want to extract this directory, and it doesn't point to + * a directory. + * + * This will cause problems if this package or any other + * package try to extract files to this directory. */ + + /* TODO: How do we handle this? If we continue we leeave a + * broken system, if we abort we leave a broken system! */ + _alpm_log(PM_LOG_ERROR, _("%s is a symlink, but it does not point to a directory!"), entryname); + } + + _alpm_log(PM_LOG_DEBUG, _("%s (directory) won't be extracted over existing symlink."), entryname); + archive_read_data_skip(archive); + continue; } - - /* NoUpgrade skips all this backup stuff, because it's just never - * touched */ - if(alpm_list_find_str(handle->noupgrade, entryname)) { - notouch = 1; - needbackup = 0; + + if(S_ISLNK(entrytype) && S_ISDIR(buf.st_mode)) { + /* Don't extract a symlink over a directory. */ + _alpm_log(PM_LOG_DEBUG, _("%s (symlink) won't be extracted over existing directory."), entryname); + archive_read_data_skip(archive); + continue; + } + + if(S_ISREG(entrytype)) { + /* always check the newpkg first, so when we do add a backup=() file, + * we don't have to wait a full upgrade cycle */ + needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); + + if(is_upgrade) { + hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); + if(hash_orig) { + needbackup = 1; + } + } + + /* this is kind of gross. if we force hash_orig to be non-NULL we can + * catch the pro-active backup=() case (when the backup entry is in + * the new package, and not the old */ + if(needbackup && !hash_orig) { + hash_orig = strdup(""); + } + + /* NoUpgrade skips all this backup stuff, because it's just never + * touched */ + if(alpm_list_find_str(handle->noupgrade, entryname)) { + notouch = 1; + needbackup = 0; + } + } + else { + /* We shouldn't be here! This should have already been + * caught by the conflict check. */ + _alpm_log(PM_LOG_ERROR, _("%s already exists on the filesystem."), entryname); + _alpm_log(PM_LOG_DEBUG, _("%s should have been caught by the conflict checks!"), entryname); + RET_ERR(PM_ERR_FILE_CONFLICTS, -1); } } @@ -705,14 +749,12 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) archive_entry_set_pathname(entry, filename); /* FS #7484 - * By default, libarchive 2.2.3 overwrites existing symlinks by directories from the archive, - * which isn't the behavior we want. - * This can be avoided by using the ARCHIVE_EXTRACT_NO_OVERWRITE flag, and this works - * fine because all files where an overwrite could be needed are deleted first : - * 1) if it's an upgrade, existing files are removed when the old pkg is removed - * 2) if there is a file conflict, but --force is used, then files are also removed : see above + * Andrew Fyfe: + * Removed ARCHIVE_EXTRACT_NO_OVERWRITE from the next command. + * The new checks above should catch all the directory extract/ + * overwrite issues for now. */ - int ret = archive_read_extract(archive, entry, archive_flags | ARCHIVE_EXTRACT_NO_OVERWRITE); + int ret = archive_read_extract(archive, entry, archive_flags); if(ret == ARCHIVE_WARN) { /* operation succeeded but a non-critical error was encountered */ _alpm_log(PM_LOG_DEBUG, "warning extracting %s (%s)", -- 1.5.2.3
On Thu, Jul 12, 2007 at 08:53:03PM +0100, Andrew Fyfe wrote:
I've replaced Xavier's comment about ARCHIVE_EXTRACT_NO_OVERWRITE, and put in the final else case (as Dan suggested).
I ran add012 and add013 and it didn't reached the else case. After 3.1 I'll start looking at fixing the real issue in the conflict checks.
in add012 or add013 case, there isn't already files on the filesystem, is there? For issue in the conflict checks, are you partly referring to my comments there : http://www.archlinux.org/pipermail/pacman-dev/2007-July/008747.html ? AS I said there I think the --force flag won't really behave as expected with your patch, but it's probably more important to get it right without that --force flag first. There are also still the same issue than with my NO_OVERWRITE patch, that I mentioned there : http://www.archlinux.org/pipermail/pacman-dev/2007-July/008730.html "Also, we could check for permission differences there, as suggested in the BR. There is also still the problem with the tmp/ directory, which is first created by pacman with the wrong permissions, and which will keep these wrong permissions if we don't update it there when installing the filesystem package. But that problem could be fixed at the places where pacman creates tmp/" These two little changes could go just after your patch is merged though.
On 7/12/07, Xavier <shiningxc@gmail.com> wrote:
There is also still the problem with the tmp/ directory, which is first created by pacman with the wrong permissions, and which will keep these wrong permissions if we don't update it there when installing the filesystem package. But that problem could be fixed at the places where pacman creates tmp/"
Two issues here: 1. Pacman should create /tmp with whatever are deemed proper permissions (is it 4777?) 2. When extracting directories, we should compare the mode of the existing and the mode of the one to be extracted, and probably spit a warning if they are different so the user at least knows this could be an issue. I am not sure which set of permissions to go with, however. -Dan
On Thu, Jul 12, 2007 at 05:47:43PM -0400, Dan McGee wrote:
On 7/12/07, Xavier <shiningxc@gmail.com> wrote: 2. When extracting directories, we should compare the mode of the existing and the mode of the one to be extracted, and probably spit a warning if they are different so the user at least knows this could be an issue. I am not sure which set of permissions to go with, however.
What do you mean in that last sentence ?
On 7/12/07, Xavier <shiningxc@gmail.com> wrote:
On Thu, Jul 12, 2007 at 05:47:43PM -0400, Dan McGee wrote:
On 7/12/07, Xavier <shiningxc@gmail.com> wrote: 2. When extracting directories, we should compare the mode of the existing and the mode of the one to be extracted, and probably spit a warning if they are different so the user at least knows this could be an issue. I am not sure which set of permissions to go with, however.
What do you mean in that last sentence ?
Should we assume the directory permissions in the package being installed are correct, or should we assume the permissions on the user's filesystem are correct? Package: rw-r--r-- root root /foo Filesystem: rw-rw-r-- root fooey /foo Which one do we go with? -Dan
On Thu, Jul 12, 2007 at 05:58:20PM -0400, Dan McGee wrote:
On 7/12/07, Xavier <shiningxc@gmail.com> wrote:
On Thu, Jul 12, 2007 at 05:47:43PM -0400, Dan McGee wrote:
On 7/12/07, Xavier <shiningxc@gmail.com> wrote: 2. When extracting directories, we should compare the mode of the existing and the mode of the one to be extracted, and probably spit a warning if they are different so the user at least knows this could be an issue. I am not sure which set of permissions to go with, however.
What do you mean in that last sentence ?
Should we assume the directory permissions in the package being installed are correct, or should we assume the permissions on the user's filesystem are correct?
Package: rw-r--r-- root root /foo Filesystem: rw-rw-r-- root fooey /foo
Which one do we go with?
Ah ok. Well Paul and Jeremy gave their opinion on this in the BR : http://bugs.archlinux.org/task/7484#comment17621 "It seems in general dangerous to change the permissions of existing directories by installing new packages, but it could break things either way." http://bugs.archlinux.org/task/7484#comment17623 "* 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." And both suggested to print a warning in this case, and you agreed on that too ;)
Xavier wrote:
On Thu, Jul 12, 2007 at 05:58:20PM -0400, Dan McGee wrote:
On 7/12/07, Xavier <shiningxc@gmail.com> wrote:
On Thu, Jul 12, 2007 at 05:47:43PM -0400, Dan McGee wrote:
On 7/12/07, Xavier <shiningxc@gmail.com> wrote: 2. When extracting directories, we should compare the mode of the existing and the mode of the one to be extracted, and probably spit a warning if they are different so the user at least knows this could be an issue. I am not sure which set of permissions to go with, however. What do you mean in that last sentence ? Should we assume the directory permissions in the package being installed are correct, or should we assume the permissions on the user's filesystem are correct?
Package: rw-r--r-- root root /foo Filesystem: rw-rw-r-- root fooey /foo
Which one do we go with?
Ah ok. Well Paul and Jeremy gave their opinion on this in the BR : http://bugs.archlinux.org/task/7484#comment17621
"It seems in general dangerous to change the permissions of existing directories by installing new packages, but it could break things either way."
http://bugs.archlinux.org/task/7484#comment17623
"* 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."
And both suggested to print a warning in this case, and you agreed on that too ;)
_______________________________________________ pacman-dev mailing list pacman-dev@archlinux.org http://archlinux.org/mailman/listinfo/pacman-dev
A little C help please :) Am I being really THICK? WTF is wrong with line 61? (http://neptune-one.homeip.net/git?p=pacman;a=blob;f=lib/libalpm/util.h;h=19c...) util.h:61: error: expected ')' before 'mode' Thanks Andrew
On Sat, Jul 14, 2007 at 01:53:37AM +0100, Andrew Fyfe wrote:
A little C help please :)
Am I being really THICK? WTF is wrong with line 61? (http://neptune-one.homeip.net/git?p=pacman;a=blob;f=lib/libalpm/util.h;h=19c...) util.h:61: error: expected ')' before 'mode'
I'm not good either. I first tried chaning mode_t to another fake type, it resulted in the same error, so it let me think mode_t was not defined. I tried to define a fake mode_t with : typedef int mode_t; but this resulted in another error about conflicting types, so I'm confused. Anyway, including <sys/types.h> where mode_t is defined apparently fix the problem, but I'm still confused. I didn't know about that strmode function, I thought about just using something like : if (buf.st_mode != archive_entry_mode(entry)) printf("%o != %o\n", buf.st_mode, archive_entry_mode(entry)); :D
On 7/14/07, Xavier <shiningxc@gmail.com> wrote:
On Sat, Jul 14, 2007 at 01:53:37AM +0100, Andrew Fyfe wrote:
A little C help please :)
Am I being really THICK? WTF is wrong with line 61? (http://neptune-one.homeip.net/git?p=pacman;a=blob;f=lib/libalpm/util.h;h=19c...) util.h:61: error: expected ')' before 'mode'
I'm not good either. I first tried chaning mode_t to another fake type, it resulted in the same error, so it let me think mode_t was not defined. I tried to define a fake mode_t with : typedef int mode_t; but this resulted in another error about conflicting types, so I'm confused.
Anyway, including <sys/types.h> where mode_t is defined apparently fix the problem, but I'm still confused.
I didn't know about that strmode function, I thought about just using something like : if (buf.st_mode != archive_entry_mode(entry)) printf("%o != %o\n", buf.st_mode, archive_entry_mode(entry));
Including <sys/types.h> is the correct way to solve this problem here. If you look at util.h, you will notice that <time.h> was already included to give us the time_t type. Obviously you want to do as little inclusion of headers in other headers as possible, but sometimes it is unavoidable. -Dan
On Mon, Jul 09, 2007 at 12:53:48AM +0100, Andrew Fyfe wrote:
If a file/directory/symlink does not already exist on the filesystem, then it is extracted as normal.
Never extract a package entry over a directory.
When extracting a directory: - If a symlink already exists on the filesystem then the directory is not extracted.
BUT if the symlink does not point to a directory we have a problem! What do we do in this situation? If we abort or continue we can end up breaking the system!
When extracting a symlink: - If a directory already exists on the filesystem then the symlink is not extracted.
When extracting a reqular file - Do the normal backup checks.
For all other cases the conflict checks have probably failed.
Signed-off-by: Andrew Fyfe <andrew@neptune-one.net> --- lib/libalpm/add.c | 97 +++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 69 insertions(+), 28 deletions(-)
diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 870d1f8..dc9b1e6 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -439,6 +439,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) int needbackup = 0, notouch = 0; char *hash_orig = NULL; struct stat buf; + mode_t entrytype = archive_entry_filetype(entry);
entryname = archive_entry_pathname(entry);
@@ -459,11 +460,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db)
memset(filename, 0, PATH_MAX); /* just to be sure */
- if(strcmp(entryname, ".PKGINFO") == 0 - || strcmp(entryname, ".FILELIST") == 0) { - archive_read_data_skip(archive); - continue; - } else if(strcmp(entryname, ".INSTALL") == 0) { + if(strcmp(entryname, ".INSTALL") == 0) { /* the install script goes inside the db */ snprintf(filename, PATH_MAX, "%s/%s-%s/install", db->path, newpkg->name, newpkg->version); @@ -497,33 +494,77 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) continue; }
- /* check is file already exists */ - if(stat(filename, &buf) == 0 && !S_ISDIR(buf.st_mode)) { - /* it does, is it a backup=() file? - * always check the newpkg first, so when we do add a backup=() file, - * we don't have to wait a full upgrade cycle */ - needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); - - if(is_upgrade) { - hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); - if(hash_orig) { - needbackup = 1; + if(lstat(filename, &buf) == 0) { + /* Entry exists on filesystem... */ + + if(S_ISDIR(buf.st_mode)) { + /* Don't extract anything over an existing directory.*/ + if(!S_ISDIR(entrytype)) { + _alpm_log(PM_LOG_WARNING, _("%s won't be extracted over an existing directory."), entryname); } + + _alpm_log(PM_LOG_DEBUG, _("%s won't be extracted over an existing directory."), entryname); + archive_read_data_skip(archive); + continue; }
If we wanted to extract anything other than a directory, or a symlink (to another directory), shouldn't it be a conflict then ? Like if we skip the extraction of a regular file from a package because of an existing directory, then the installed package might be missing a file potentially containing some informations :) Also, if the existing directory is non-empty, afaik it'll never be overwritten. Only an empty directory can apparently be overwritten by libarchive. And in that case, we should maybe be able to overwrite empty directories with other files, using pacman --force flag.
- /* this is kind of gross. if we force hash_orig to be non-NULL we can - * catch the pro-active backup=() case (when the backup entry is in - * the new package, and not the old */ - if(needbackup && !hash_orig) { - hash_orig = strdup(""); + if(S_ISDIR(entrytype) && S_ISLNK(buf.st_mode)) { + /* Don't extract a directory over an existing symlink. */ + if(stat(filename, &buf) == 0 && !S_ISDIR(buf.st_mode)) { + /* Something's going to break! A symlink exists where we + * want to extract this directory, and it doesn't point to + * a directory. + * + * This will cause problems if this package or any other + * package try to extract files to this directory. */ + + /* TODO: How do we handle this? If we continue we leeave a + * broken system, if we abort we leave a broken system! */ + _alpm_log(PM_LOG_ERROR, _("%s is a symlink, but it does not point to a directory!"), entryname); + } + + _alpm_log(PM_LOG_DEBUG, _("%s (directory) won't be extracted over existing symlink."), entryname); + archive_read_data_skip(archive); + continue; }
Couldn't there be a conflict when the symlink doesn't point to a directory ? Since a directory and a symlink that doesn't point to a directory are not compatible, that sounds just like a conflict to me. And maybe also in this case, if --force is used, then that problematic symlink could be overwritten.
- - /* NoUpgrade skips all this backup stuff, because it's just never - * touched */ - if(alpm_list_find_str(handle->noupgrade, entryname)) { - notouch = 1; - needbackup = 0; + + if(S_ISLNK(entrytype) && S_ISDIR(buf.st_mode)) { + /* Don't extract a symlink over a directory. */ + _alpm_log(PM_LOG_DEBUG, _("%s (symlink) won't be extracted over existing directory."), entryname); + archive_read_data_skip(archive); + continue; }
That one is important when the directory on the filesystem is empty, because in that case libarchive could overwrite it. But maybe it could be handled symetrically to the case above: * if the symlink in the archive points to a directory, then we always skip it * if it doesn't, then pacman fails because of a conflict, which can be ignored with --force Oh well, that's just some thoughts, I've no idea how all these corner cases should be handled, it's pretty annoying :p
+ + if(S_ISREG(entrytype)) { + /* always check the newpkg first, so when we do add a backup=() file, + * we don't have to wait a full upgrade cycle */ + needbackup = alpm_list_find_str(alpm_pkg_get_backup(newpkg), entryname); + + if(is_upgrade) { + hash_orig = _alpm_needbackup(entryname, alpm_pkg_get_backup(oldpkg)); + if(hash_orig) { + needbackup = 1; + } + } + + /* this is kind of gross. if we force hash_orig to be non-NULL we can + * catch the pro-active backup=() case (when the backup entry is in + * the new package, and not the old */ + if(needbackup && !hash_orig) { + hash_orig = strdup(""); + } + + /* NoUpgrade skips all this backup stuff, because it's just never + * touched */ + if(alpm_list_find_str(handle->noupgrade, entryname)) { + notouch = 1; + needbackup = 0; + } + } + /* else { + * we shouldn't be here! This should have already been + * caught by the conflict check. + * } */ }
Ah yes, as Dan already commented, with --force it will most probably go there.
if(needbackup) { @@ -705,7 +746,7 @@ int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db) * 1) if it's an upgrade, existing files are removed when the old pkg is removed * 2) if there is a file conflict, but --force is used, then files are also removed : see above */ - int ret = archive_read_extract(archive, entry, archive_flags | ARCHIVE_EXTRACT_NO_OVERWRITE); + int ret = archive_read_extract(archive, entry, archive_flags); if(ret == ARCHIVE_WARN) { /* operation succeeded but a non-critical error was encountered */ _alpm_log(PM_LOG_DEBUG, _("warning extracting %s (%s)"),
hm maybe my comment there should be either adapted or removed then :)
On Sun, Jul 08, 2007 at 10:30:58PM +0100, Andrew Fyfe wrote:
Xavier wrote:
If we skip the second case, and the package has some files in file/dir, then this is going to be weird :) We could move the file to *.pacold?
Hm maybe, but why doesn't it just fail, as I said below? Then the user can remove that file, or move it to whatever he wants, before installing the package.
But what I don't get is why doesn't it totally fail in both cases ? Shouldn't there be a conflict between a regular file and a directory ? It looks like this isn't the case currently. Apparently, as long as there is a directory on one side, conflict check are skipped. So there might be a reason for that, I'm not sure.. Yes if either side is a directory it's missed by the conflict checks. I think the conflict checks need a similar solution to my patch for add_commit().
I think it's rather between a symlink to a directory and a directory that we can choose between skipping and extracting, but I could be wrong. I don't understand (and a directory??), extracting a symlink over a directory should NOT happen.
I was talking about the case where we have "foo -> bar/" on one side, and "foo/" on the other side. We don't want to have a conflict in this case. We have to choose between skipping and extracting, and we choose to skip (at least when the symlink is on the filesystem side). And I guess it could skip as well in the other direction (symlink in the package, and real directory on the filesystem). In that second case, it doesn't really matter though, because afaik libarchive doesn't remove non-empty directories (while it does remove symlinks) before extracting.
Slightly OT: Some work is still needed on permissions as well, I've just noticed usr/man/man{1,4} are 0700 :@, not sure what package did it.
Let us know if you find out how to reproduce it.
On Mon, Jul 09, 2007 at 08:38:25AM +0200, Xavier wrote:
Slightly OT: Some work is still needed on permissions as well, I've just noticed usr/man/man{1,4} are 0700 :@, not sure what package did it.
Let us know if you find out how to reproduce it.
Looks like it isn't caused by pacman/libarchive after all :p http://www.archlinux.org/pipermail/arch-dev-public/2007-July/001152.html
Xavier wrote:
On Mon, Jul 09, 2007 at 08:38:25AM +0200, Xavier wrote:
Slightly OT: Some work is still needed on permissions as well, I've just noticed usr/man/man{1,4} are 0700 :@, not sure what package did it.
Let us know if you find out how to reproduce it.
Looks like it isn't caused by pacman/libarchive after all :p http://www.archlinux.org/pipermail/arch-dev-public/2007-July/001152.html
_______________________________________________ pacman-dev mailing list pacman-dev@archlinux.org http://archlinux.org/mailman/listinfo/pacman-dev Yeah the broken package was wireshark which was built by Thomas :)
I've narrowed down the problem but I don't have a clue how to fix it... http://www.archlinux.org/pipermail/arch-dev-public/2007-July/001165.html Andrew
participants (4)
-
Andrew Fyfe
-
Dan McGee
-
ngaba＠petra.hos.u-szeged.hu
-
Xavier