[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:
> jcai19 wrote:
> > 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."
> The suggestion to use `fstat` is here: https://sourceware.org/bugzilla/show_bug.cgi?id=26945#c2. Although as mentioned, it may not be exploitable.
Thanks for the link, but if I understand you correctly, it seems to me that we only have path of the output file, so I have to call stat instead, which is the same as fstat except it takes the path instead of the file descriptor. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93881