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

[Dan McGee]

On 7/15/07, Xavier <shiningxc at gmail.com> wrote:
> 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) {
> >

I made a few changes after thinking about the above comments to my
patch, and it is now on my working branch. More explicit handling of
case 10 is done.

-Dan