[pacman-dev] [PATCH] can_remove_file: do not follow symlinks

[Andrew Gregory]

On 09/25/16 at 01:43pm, Joseph Burt wrote:
> On Sun, Aug 7, 2016 at 8:17 PM, Joseph Burt <caseorum at gmail.com> wrote:
> > On Sun, Aug 7, 2016 at 5:02 AM, Allan McRae <allan at archlinux.org> wrote:
> >> It would be better to fix _alpm_access and fix all occurrences of this
> >> rather than replace this one instance.
> >>
> > You're right of course, this is just the smallest change that would
> > correct the bug. Since _alpm_access can't tell from the W_OK mode
> > whether we mean to open for write (follow symlinks) or unlink (don't),
> > it's not enough. Thoughts on new util functions like
> > "_alpm_can_unlink"?

Looking at our other uses of _alpm_access, this appears to actually be
the only one where we would want to operate directly on the symlink.

> >> Note AT_SYMLINK_NOFOLLOW is not available on OSX.  I believe pacman
> >> still builds there currently, but I can't remember if we decided not to
> >> support it...
> >>
> > Ok, so that means using lstat instead of faccessat.

I'm not overly concerned with supporting OSX where it deviates from
POSIX, but, in this case, AT_SYMLINK_NOFOLLOW is a non-POSIX
extension for faccessat.

> > There's also a TOCTOU bug here just for checking at all. Access or
> > lstat can't guarantee that open or unlink will succeed, so why bother?
> > Those failing should be dealt with properly anyway. The question is,
> > if a file fails to unlink, after logging it, should you remove the
> > package from the db, leaving orphaned files? Or leave it, but now
> > possibly missing files? Or keep FDs open for every file in a package
> > until all unlinks succeed, and if one fails, try to undo by copying
> > back the already unlnked files?
> > This is a design decision...
> 
> Allan, any thoughts on what you'd like here? I'm currently working
> around this bug by removing the can_remove_file check entirely.

Half-finished operations are never a good thing, so we do our best to
make sure that once the operation starts it will be able to run to
completion.  Unfortunately, to the best of my knowledge, there is no
way for us to perform these checks in a way that guarantees that the
results are still valid by the time we go to perform the actual
operation or reliably undo failed operations.  The best we can do is
check as much as possible prior to beginning the operation, assume
that other processes aren't going to be fiddling with pacman-owned
files during the operation, and handle errors as gracefully as
possible.

I agree that this is the wrong place to perform this check.  This
should be performed during the prepare phase so that we can prevent
the doomed operation from ever starting.

apg