[pacman-dev] difficult symlink "puzzle"

[Xavier]

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 at 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=c992a5d757efa5aee45ed4cc48e02218be933101
> 
> 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 at 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;
> +						}
>  					}
>  				}
>