[pacman-dev] [PATCH] FS#7484 - pacman clobbers directory symlinks.
Dan McGee
dpmcgee at gmail.com
Mon Jul 9 00:24:23 EDT 2007
On 7/8/07, Andrew Fyfe <andrew at 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 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;
> }
>
> - /* 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
More information about the pacman-dev
mailing list