On Sun, Aug 7, 2016 at 8:17 PM, Joseph Burt <caseorum@gmail.com> wrote:
On Sun, Aug 7, 2016 at 5:02 AM, Allan McRae <allan@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"?
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.
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.