[PATCH] D93881: [llvm-objcopy] preserve file ownership when overwritten

Thu Feb 11 17:17:47 PST 2021

jcai19 added a comment.

In D93881#2558654 <https://reviews.llvm.org/D93881#2558654>, @MaskRay wrote:

> Overall, LGTM. I am fine with fchown, but the description needs a lot of changes to proceed.

Thanks!

> For one, the fix advice in https://reviews.llvm.org/D93881#2483542 needs to be taken, `objcopy xxx -o yyy` does not work.

Oh i've updated my commit message locally a while ago, I did not realize this has not been updated in Phabricator. Will definitely update it.

> You can mention that as of binutils 2.36, GNU strip calls chown(2) for `sudo strip a` and `sudo strip a -o a`, but not sudo strip a -o b or sudo strip a -o ./a.
>
>> The discrepancy prevents Chrome OS from migrating to llvm-objcopy and llvm-strip as Portage (the build system) builds binaries following recommended steps documented at the man page of GNU strip as follows,
>
> This part should be rephrased. I think whatever llvm-objcopy/llvm-strip resolution is, Portage may need to migrate away from in-place strip as well @mgorny @robertu94, like https://git.archlinux.org/pacman.git/commit/?id=88d054093c1c99a697d95b26bd9aad5bc4d8e170
> To prevent pessimistic double write behavior with binutils 2.37.
>
>> With llvm-strip and llvm-objcopy, the last two steps change ownership of binaries to root and cause them to become inaccessible to the intended user.
>
> The question is about when root strips that is intended to have different owner/group, after atomic rename, should root restore owner/group?
> I have asked many folks for their opinions. The result is contentious. It can be argued either way. In particular, I got objection like:
> "the question is not 'how to justify fchown' but 'why justify fchown'" "if someone wants to sudo strip, let them do so and deal with the consequences"
>
> I finally swing to fchown because the implicit intention to keep the file accessible by original owner/group.
> The argument is stronger than "because doing this patch makes us more compatible with GNU, so we do it." While generally it makes sense to keep compatibility when I can do that for free,
> the binutils behavior is quite problematic. Do we want to take its new behavior? D96308 <https://reviews.llvm.org/D96308> I have thought very hard and finally think "no".
>
> This does mean unfortunate platform differences and slightly inconsistent behaviors, but we are going to take it.

Will address the rest.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93881/new/

https://reviews.llvm.org/D93881