[pacman-dev] [PATCH] FS#7484 - pacman clobbers directory symlinks.
Xavier
shiningxc at gmail.com
Mon Jul 9 04:15:56 EDT 2007
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 at 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 :)
More information about the pacman-dev
mailing list