This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][NFC] Move ownership keeping code into restoreStatOnFile()
ClosedPublic

Authored by avl on Mar 12 2021, 7:43 AM.

Details

Summary

The D93881 added functionality which preserve ownership for output file
if llvm-objcopy is called under /root. That code was added into the place
where output file is created. The llvm-objcopy already has a function which
sets/restores rights/permissions for the output file.
That is the restoreStatOnFile() function. This patch moves code
(preserving ownershipping) into the restoreStatOnFile() function.

Diff Detail

Event Timeline

avl created this revision.Mar 12 2021, 7:43 AM
avl requested review of this revision.Mar 12 2021, 7:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2021, 7:43 AM

I'm not sure if this would be a big issue but restoreStatOnFile also runs on the split dwo file, and will keep its ownership with this change. I don't think GNU assembler does that currently. Also you could delete the code added in https://reviews.llvm.org/D98511 for passing user and group ID since they are no longer needed, assuming the new behavior is what we want.

avl added a comment.Mar 12 2021, 11:36 AM

I'm not sure if this would be a big issue but restoreStatOnFile also runs on the split dwo file, and will keep its ownership with this change. I don't think GNU assembler does that currently.

I though it would be good to keep ownership for all modes. If it is important to not doing it for split dwo file case then I will add corresponding check.

Also you could delete the code added in https://reviews.llvm.org/D98511 for passing user and group ID since they are no longer needed, assuming the new behavior is what we want.

I suppose this code:

explicit FileBuffer(StringRef FileName, bool Keep, unsigned UID, unsigned GID)
     : Buffer(FileName), KeepOwnership(Keep), UserID(UID), GroupID(GID) {}

Ok, will delete it. thanks.

avl updated this revision to Diff 330361.Mar 12 2021, 1:38 PM

addressed comments(removed ownership keeping code from FileOutputBuffer).

In D98511#2623027, @avl wrote:

I'm not sure if this would be a big issue but restoreStatOnFile also runs on the split dwo file, and will keep its ownership with this change. I don't think GNU assembler does that currently.

I though it would be good to keep ownership for all modes. If it is important to not doing it for split dwo file case then I will add corresponding check.

I'm fine with the new behavior, if I'm understanding correctly and it means that dwo files will be treated more like/the same as how .o files have been treated by both GCC and Clang for some time now. (GCC's behavior is probably an artifact of running llvm-objcopy to make the dwo file)

MaskRay accepted this revision.Mar 16 2021, 12:05 PM

The fchown behavior requires root (CAP_CHOWN) so it is not tested. I have tried several commands and the behavior is kept.

'cp' -f aa.o a.o; chmod 3777 a.o; ls -l a.o; sudo /tmp/RelA/bin/llvm-strip -p a.o; ls -l a.o
'cp' -f aa.o a.o; chmod 3707 a.o; ls -l a.o; sudo /tmp/RelA/bin/llvm-objcopy -p a.o b.o; ls -l b.o
umask 0
'cp' -f aa.o a.o; chmod 3707 a.o; ls -l a.o; sudo /tmp/RelA/bin/llvm-objcopy -p a.o b.o; ls -l b.o

Treating .dwo similar to .o is good.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
301

/root => root.

302

The two conditions can be folded.

This revision is now accepted and ready to land.Mar 16 2021, 12:05 PM
MaskRay retitled this revision from [llvm-objcopy][NFC] Move ownership keeping code into the restoreStatOnFile(). to [llvm-objcopy][NFC] Move ownership keeping code into restoreStatOnFile().Mar 16 2021, 12:05 PM
avl added a comment.Mar 17 2021, 3:09 AM

Thank you for the review.

@avl Did you test this change? It has broken setuid permission preservation with sudo.

Repro case:

$ chmod +s main main
$ -rwsr-sr-x 1 manojgupta primarygroup 5936 Apr 11 17:46 main
$ sudo bin/llvm-strip main
$ ls -l main
-rwxr-xr-x 1 manojgupta primarygroup 5936 Apr 11 17:46 main

Note that setuid bit was removed.

This is causing breakage in chrome os builds :
https://bugs.chromium.org/p/chromium/issues/detail?id=1197970

Please fix or revert.

avl added a comment.Apr 12 2021, 2:30 AM

yep, will fix or revert.

@cjdb please pick the fix once available to chrome os.

avl added a comment.Apr 12 2021, 10:39 AM

@manojgupta @cjdb Sorry for the inconvenience. Please check, whether the problem is really fixed on your side(The commit with the fix is https://reviews.llvm.org/rGee8a5e4bc2c986b8e6c07e81fb58dc1e5a5c2d17)

Thanks for the quick turn around, I can verify that https://reviews.llvm.org/rGee8a5e4bc2c986b8e6c07e81fb58dc1e5a5c2d17 fixes the issue.

@cjdb please cherry pick to chrome os.