FS#7652 - libalpm check for ability to remove file is wrong, pacman runs into a loop when this gets triggered

Attached to Project: Pacman
Opened by Jan de Groot (JGC) - Monday, 23 July 2007, 16:57 GMT
Last edited by Dan McGee (toofishes) - Monday, 03 December 2007, 04:22 GMT
Task Type Bug Report
Category General
Status Closed
Assigned To Aaron Griffin (phrakture)
Dan McGee (toofishes)
Architecture All
Severity Low
Priority Normal
Reported Version 3.0.5
Due in Version 3.1.0
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

in libalpm/remove.c, there's a function that checks if it's possible to delete a file. This check fails on FreeBSD, and actually, this check isn't sane:

if(access(file, W_OK) == -1) {
if(errno != EACCES && access(file, F_OK) == 0) {

First of all, deleting a file is not the same as writing a file. Deleting the file means writing the parent directory. On FreeBSD, this check returns ETXTBSY because the "text file" is "busy" (pacman or bash). This is also the documented behaviour in linux glibc, but somehow it doesn't happen there (bad glibc!).
I modified the check to ignore ETXTBSY also in the 2nd line, pacman can upgrade pacman and bash safely on FreeBSD now.

At the moment pacman runs into this error, pacman goes into a loop to clear the internal error state, pacman can only be interrupted with CTRL+\ (SIGQUIT).
This task depends upon

This task blocks these from closing
 FS#8109 - Pacman 3.1 Release Roadmap 
Closed by  Dan McGee (toofishes)
Monday, 03 December 2007, 04:22 GMT
Reason for closing:  Fixed
Additional comments about closing:  ETXTBSY check was added, closing.
Comment by Dan McGee (toofishes) - Friday, 27 July 2007, 00:12 GMT
Do you want to post your patch here just to make sure I understand it correctly?
Comment by Jan de Groot (JGC) - Friday, 27 July 2007, 06:19 GMT
This piece of patch fixes the described bug. IMHO the logic is still not right, but this fixes the "Text file busy" error I get on FreeBSD which should also be the case on linux according to documentation. To reproduce the loop I was getting, try running pacman without the two lines I put in the original bugreport, it should loop until you kill it.
   pacman-3_0_5-freebsd.patch (1 KiB)
Comment by Xavier (shining) - Thursday, 09 August 2007, 08:55 GMT
About the loop, just one little question : you're installing with -U right ? Not with -S ?
Comment by Jan de Groot (JGC) - Thursday, 09 August 2007, 09:40 GMT
The loop happens with pacman -U yes.
Comment by Xavier (shining) - Thursday, 09 August 2007, 10:12 GMT
Ok thanks, I had been looking at this loop problem a while ago, but didn't notice it was fine with -S. Hopefully this new element will help :)
http://www.archlinux.org/pipermail/pacman-dev/2007-August/009084.html
That's not the real issue this bug report is about though, so I'll stop hijacking it now.
Comment by Dan McGee (toofishes) - Friday, 24 August 2007, 00:45 GMT
OK, I've pushed your patch to my pacman GIT tree. Can you suggest a better way of fixing this? Your initial report indicates that the check isn't being done quite right in the first place.
Comment by Jan de Groot (JGC) - Friday, 24 August 2007, 07:12 GMT
The point is that we want to remove the file. To check if we can remove the file, we try to open the file in write mode, which isn't correct. This is only the case when the directory the file is in has the sticky bit set (just like /tmp). When you remove the sticky bit from a directory and give permissions to that directory, everyone can write new files or delete existing files from that directory. First deleting a file and then writing a new one is not the same as opening it in write mode like we do in this check.
Comment by Xavier (shining) - Friday, 24 August 2007, 09:03 GMT
I also recently noticed that this code might be incorrect, but for another reason :
http://www.archlinux.org/pipermail/pacman-dev/2007-August/009150.html
Comment by Dan McGee (toofishes) - Tuesday, 30 October 2007, 17:43 GMT
OK, it sounds like we need to really work on this can_remove_file function. Anyone want to step up to the plate and rewrite the current GIT version to make it cross-platform compatible and actually do the right checks?
Comment by Aaron Griffin (phrakture) - Tuesday, 30 October 2007, 21:02 GMT
There are a few other issues here too. For instance, we don't check for immutable files (FS#3564).

What we really need is to do real transactional logic and NOT make this check at all, but rollback on failure.
Comment by Xavier (shining) - Tuesday, 30 October 2007, 21:37 GMT
I was writing an answer but was interrupted. And now I see that Aaron already said what I wanted.
I also think there is no easy fix, and the best thing to do would be to rollback on failure.
Comment by Dan McGee (toofishes) - Friday, 09 November 2007, 04:25 GMT
Aaron- maybe we should open up another collector-type bug called "Implement REAL pacman transaction system", and collect these type of bugs so we know what we intend to fix with real transactions?
Comment by Aaron Griffin (phrakture) - Friday, 09 November 2007, 05:46 GMT
Agreed. However, didn't you apply this patch, closing this particular bug?
Comment by Dan McGee (toofishes) - Friday, 09 November 2007, 05:51 GMT
Yep, we check for both of these codes now.
Comment by Xavier (shining) - Friday, 09 November 2007, 07:56 GMT
The title is : libalpm check for ability to remove file is wrong, pacman runs into a loop when this gets triggered
The loop was fixed, and the check was sligthly modified so that it also works on freebsd, but according to JGC, that check is still wrong.
But if you want to close this bug report, I can't prevent you from doing it anyway :)
Comment by Jan de Groot (JGC) - Friday, 09 November 2007, 08:20 GMT
I can't test at this moment as I don't have a FreeBSD box at this moment and the git version of pacman requires a lot more patching than the last release does (all the #ifdef __SomeOS__ things have been removed from git last time I checked).
Comment by Aaron Griffin (phrakture) - Friday, 09 November 2007, 08:45 GMT
The check being wrong is a whole different issue. We should open a new report related to real transaction support (it is 100% impossible to make that check correct, immutable files, etc etc - there can always be problems and issues there).

Jan: I think Eliott has been working with Dan to get pacman working on FreeBSD and OSX. Last I heard, all expected pactests worked fine.

Jan, because you opened it, I'll wait for your word to close this bug
Comment by Dan McGee (toofishes) - Friday, 09 November 2007, 15:10 GMT
Jan- compiling pacman-git on FreeBSD works fine- all those #ifdefs were completely unnecessary. Compiling works fine as long as you have a recent version of libarchive (2.X series) and libfetch.

See here (you really only need the libdownload->libfetch patch):
http://code.toofishes.net/gitweb.cgi?p=pacman.git;a=shortlog;h=freebsd
Comment by Jan de Groot (JGC) - Friday, 09 November 2007, 18:08 GMT
if this ETXTBSY check is added this bug can be closed, as it works fine with the current logic.

Loading...