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

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; + } } }

On 7/8/07, Andrew Fyfe 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 --- 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: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 --- 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 :)