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

[Jian Cai via Phabricator via llvm-commits]

jcai19 marked an inline comment as done.
jcai19 added inline comments.


================
Comment at: llvm/lib/Support/FileOutputBuffer.cpp:131
   Expected<fs::TempFile> FileOrErr =
       fs::TempFile::create(Path + ".tmp%%%%%%%", Mode);
   if (!FileOrErr)
----------------
rupprecht wrote:
> jhenderson wrote:
> > Is there a way we could improve this API to create the temporary file with the right UID and GID in the first place? That seems like a better idea to me than having to do something post creation.
> > 
> > A thought to consider (I don't know what the desirable behaviour is here): what happens if a user were to Ctrl-C (or the program otherwise terminated unexpectedly) after the TempFile creation, before the ownership was updated? Would the temp file be in a desirable state?
> Calling `setfsuid` was suggested at one point, but that isn't a portable solution. Something like that would be the ideal fix IMO.
Thanks @rupprecht for the clarification. @jhenderson I agree that would be the best and it was in fact what I had in mind when I started, but I could not find suitable functions to use.


================
Comment at: llvm/lib/Support/Unix/Path.inc:1216
+  auto Chown = [&]() { return ::fchown(FD, Owner, Group); };
+  // Retry if fchown call fails due to interruption
+  if ((sys::RetryAfterSignal(-1, Chown)) < 0)
----------------
jhenderson wrote:
> Nit: missing trailing full stop at end of comment. It would be helpful to explain the why behind the retry in this comment. It isn't obvious to those less experienced with this sort of code.
I called RetryAfterSignal simply to follow the way the other functions in this file were written. Unfortunately I do not know exactly when this will be helpful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93881