[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:140
+    fs::file_status Stat;
+    std::error_code EC = fs::status(Path, Stat);
+    if (!EC)
----------------
rupprecht wrote:
> rupprecht wrote:
> > WDYT about doing a stat on the newly-generated tempfile so we can skip the redundant fchown if possible? (i.e. the common case of running as a non-root user)
> IIUC, this has to be `fstat` on the input file in order to work securely. Otherwise, there is a race between reading the file (for copying the object from), and deciding what to fchown the new file to.
> WDYT about doing a stat on the newly-generated tempfile so we can skip the redundant fchown if possible? (i.e. the common case of running as a non-root user)

I'm not sure I followed here.  How can we set the ownership of the tempfile to the ownership of the output file without inquiring it? And how do we skip fchown in that case?

> (i.e. the common case of running as a non-root user)
Right, I will limit this to only affect root user, i.e. when Stat.getUser() is 0. This should also avoid some of the issues with "sudo -u" issue mentioned by @MaskRay.

> IIUC, this has to be fstat on the input file in order to work securely. Otherwise, there is a race between reading the file (for copying the object from), and deciding what to fchown the new file to.

I think fs::status by default calls stat (https://llvm.org/doxygen/Unix_2Path_8inc_source.html#l00739), which should have the same effect with fast according to man page: "fstat() is identical to stat(), except that the file about which information is to be retrieved is specified by the file descriptor fd."


================
Comment at: llvm/lib/Support/FileOutputBuffer.cpp:208
     else
-      return createOnDiskBuffer(Path, Size, Mode);
+      return createOnDiskBuffer(Path, Size, Mode, Flags & F_keep_ownership);
   default:
----------------
rupprecht wrote:
> Personally, I was a little confused by `Flags & F_keep_ownership`, and I think it would be simpler to pass in the `unsigned Flags` param itself instead of `bool KeepOwnership`, and deferring the `Flags & F_keep_ownership` check until later.
We could definitely delay the and operation into createOnDiskBuffer, but it is not a member function so we would have to use something like Flags & FileOutputBuffer::F_keep_ownership. I feel with the current implementation we could avoid spilling implementation details of FileOutputBuffer into createOnDiskBuffer. WDTY?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93881