[pacman-dev] Huge cleanup of add.c and my attempt at fixing FS#7484

[Xavier]

On Sun, Jul 15, 2007 at 12:09:37PM -0400, Dan McGee wrote:
> > About cases 6 and 10, I agree that conflict checks should catch this,
> > but do they ? (they don't from my understanding of the code)
> 
> Then we should fix that in the conflict check code, probably. I
> haven't looked at it enough.

Ok, I just felt it was important to check that :)

> By the way- is there a reason we even use the filelist provided with
> the package? Any package can lie about it, and since we go though the
> archive once anyway we might as well just use our own generated
> filelist. We can also gather information about each of the files at
> this time (dir, file, symlink, etc.).
> 

I think we could do that. Also that's what Andrew suggested here :
http://www.archlinux.org/pipermail/pacman-dev/2007-July/008867.html

except we maybe don't even need to extract everything to /tmp first.

> > About cases 9 and 11, I think it depends on the nature of the symlink :
> > * if the symlink points to a directory, then skip extraction
> > * if not, then conflict
> 
> If you look at the case 9 code, then you'll see that this is what
> happens, or at least tries to. I'm not sure how effective returning 1
> is by extract_single_file, however it at least spits a warning/error
> here.
> 

Indeed, I just meant that the description in the chart wasn't very clear.
And also, that in the case it doesn't point to a symlink, pacman should
first detect it as a conflict, and simply fails, before it's too late.

> For case 11, we don't care about the nature of the symlink- we want to
> skip extracting the symlink. See the explanation in dpkg code for
> details, which I have put below:
>    *   = 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.
> 
> I think this is valid. Perhaps I should put a note like this in the code.
> 

Ah ok, I guess that makes sense.

> In addition, I just realized 10 needs a bit more code to get it to
> work properly, because we would have to unlink an entire directory
> tree. However, this is quite dangerous as well in that it could
> interfere with other packages files. Perhaps we should never allow a
> file to overwrite a directory, even in a force situation? Comments
> welcome.
> 

Indeed, I think we should never allow that.
But unlink and libarchive probably don't as well, they won't remove a whole
directory afaik.
So without --force , pacman should really just fail because of a conflict.
With --force, it doesn't really matter, because unlink and libarchive
should just fail.

> New code comments:
> diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c
> index 2c1aa2c..9019c29 100644
> --- a/lib/libalpm/add.c
> +++ b/lib/libalpm/add.c
> @@ -392,9 +392,14 @@ static int extract_single_file(struct archive *archive,
>          *  D            |   10  |  11 |  12
>          *
>          *  1,2,3- just extract, no magic necessary. lstat will fail here.
> -        *  4,5,6,7,8,10- conflict checks should have caught this.
> overwrite/backup.
> -        *  9- follow the symlink, hopefully it is a directory
> -        *  11,12- skip extraction, dir already exists
> +        *  4,5,6,7,8- conflict checks should have caught this. either overwrite
> +        *      or backup the file.

So as said above, 6 needs to be checked so that is true.

> +        *  9- follow the symlink, hopefully it is a directory, check it.
> +        *  10- file replacing directory. a simple unlink probably
> won't work here.
> +        *      TODO handle this case, it is a *very* dangerous one.

Just like 6, this should also fail with a conflict. And if --force flag
is used, unlink and libarchive shouldn't do anything as dangerous as
deleting a whole directory (but maybe double check that).

> +        *  11- don't extract symlink- a dir exists here. we don't want links to
> +        *      links, etc.

Hm, now that I'm looking at this again, I think 10 and 11 should be the same
(file or symlink replacing a directory).

> +        *  12- skip extraction, dir already exists.
>          */
>         struct stat lsbuf;
>         if(lstat(filename, &lsbuf) != 0) {
>