On 7/15/07, 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) {
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