[llvm-objcopy] preserve file ownership when overwritten by root

[jcai19]

Summary

As of binutils 2.36, GNU strip calls chown(2) for "sudo strip foo" and
"sudo strip foo -o foo", but no "sudo strip foo -o bar" or "sudo strip
foo -o ./foo". In other words, while "sudo strip foo -o bar" creates a
new file bar with root access, "sudo strip foo" will keep the owner and
group of foo unchanged. Currently llvm-objcopy and llvm-strip behave
differently, always changing the owner and group to root. The
discrepancy prevents Chrome OS from migrating to llvm-objcopy and
llvm-strip as they change file ownership and cause intended users/groups
to lose access when invoked by sudo with the following sequence
(recommended in man page of GNU strip).

1.<Link the executable as normal.>
2.<Copy "foo" to "foo.full">
3.<Run "strip --strip-debug foo">
4.<Run "objcopy --add-gnu-debuglink=foo.full foo">

This patch makes llvm-objcopy and llvm-strip follow GNU's behavior.

Link: crbug.com/1108880

[jcai19]

Update test cases.

[jcai19]

Fix test cases to make them pass on macOS.

[jhenderson]

If this brings us closer to GNU behaviour, that's fine, but I do find it weird that a build system would be doing anything involving sudo to run llvm-objcopy/llvm-strip.

llvm/include/llvm/Support/FileSystem.h

1163–1166

llvm/lib/Support/Unix/Path.inc

1214

This function doesn't look like it's been clang-formatted properly.

1219

I'm not particularly familiar with these lower-level fucntions. What's the RetryAfterSignal about? Should it be commented?

llvm/test/tools/llvm-objcopy/preserve-file-ownership.test

1 ↗

(On Diff #314795)

This test won't work on Windows, I expect. Also, please add a top-level comment to the test explaining what the behaviour is that this test is testing.

2 ↗

(On Diff #314795)

Won't this require root access to run? Pretty sure that not everyone who runs the test is going to have it...

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

334

[jcai19]

Address some comments.

[jcai19]

llvm/lib/Support/Unix/Path.inc

1214

Good catch! git clang-format HEAD~1 somehow did not notice it. I have updated it.

1219

What's the RetryAfterSignal about?

I think it basically keeps calling the same function again if EINTR (interrupted function call) happens https://llvm.org/doxygen/Errno_8h_source.html#l00033.

Should it be commented?

I just added some comments in the latest patch. FYI this function is called several times in the same file and has not been commented anywhere else, so let me know if you think we should just remove it to keep consistent.

llvm/test/tools/llvm-objcopy/preserve-file-ownership.test

2 ↗

(On Diff #314795)

That's a good point. I could remove sudo from the commands but this change won't get tested. Any suggestion on how we could better test this?

[MaskRay]

I don't think we want to introduce a filename based chmod API. Unless it is essential (very unlikely), it is easy for developers to make time-of-check time-of-use (TOCTOU) race condition allowing bypass of protection mechanism due to symlink attacks.

binutils 2.36 will fix the issue https://sourceware.org/bugzilla/show_bug.cgi?id=26945

Regarding whether llvm-objcopy should adopt the GNU objcopy smart_rename behavior, I am a bit hesitant, somewhat inclining to no.

It seems that very few packages actually require the behavior - most packages should use something like install -o ... -g ... -m ... to copy files, the owner/group is not too useful.

[manojgupta]

In D93881#2483184, @MaskRay wrote:

I don't think we want to introduce a filename based chmod API. Unless it is essential (very unlikely), it is easy for developers to make time-of-check time-of-use (TOCTOU) race condition allowing bypass of protection mechanism due to symlink attacks.

binutils 2.36 will fix the issue https://sourceware.org/bugzilla/show_bug.cgi?id=26945

Can you please clarify what is being fixed, the use of internal renaming APIs or the current behavior?

Regarding whether llvm-objcopy should adopt the GNU objcopy smart_rename behavior, I am a bit hesitant, somewhat inclining to no.

It seems that very few packages actually require the behavior - most packages should use something like install -o ... -g ... -m ... to copy files, the owner/group is not too useful.

We are hitting this problem in Portage (used in Gentoo Linux and Chrome OS) which is both a build system and a package manager. One of the steps after building a package is to install the files with the required permissions. And another action is to strip the files and add the debuginfo link etc. These steps execute as root privilege in portage as they modify the file system and we do not have control over it.

Because of the deviation from GNU objcopy/strip, stripping or just adding debuginfo link causes the "security sensitive" binaries to lose the intended permissions. We already had to revert twice switching to llvm objcopy/strip for this reason in Chrome OS before we figured out the problem. Therefore, I'd argue that this change is needed for GNU compatibility.
It is also what most tools already do when dealing with existing files e.g. "sudo vi foo.c" or "echo foo |& sudo tee a.c" does not change file ownership or group.

[nickdesaulniers]

It sounds like portage should be using something like setuidgid to drop privileges for parts of the build.

[manojgupta]

In D93881#2483309, @nickdesaulniers wrote:

It sounds like portage should be using something like setuidgid to drop privileges for parts of the build.

I do not think that matters. If a file has owner other than default [1] , stripping or adding debuginfo link to this file will still require root privileges.

[1] https://github.com/gentoo/gentoo/search?q=fowners&type=code

[MaskRay]

GNU objcopy and strip preserve the ownership of a file when it's overwritten, e.g. while "sudo objcopy foo -o bar" creates a new file bar with root access, "sudo objcopy foo" will keep the owner and group of foo unchanged. Currently llvm-objcopy and llvm-strip behave differently, always changing the ownership to root.

objcopy foo -o bar => objcopy foo bar

I am unsure that we want to adopt the GNU behavior because

(1) there were security vulnerabilities
(2) the code is subtle and untestable
(3) the behavior is self inconsistent. The behaviors of sudo objcopy foo and sudo -u mail objcopy foo (if user mail has write permission to the directory) are different.
(4) I believe very few packages require the behavior. It can only be used by root (CAP_CHOWN), so it is a very minor use case.

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,

Not having a behavior has pros and cons and I understand that it may sometimes cause friction.
But from https://chromium-review.googlesource.com/q/gnu+strip , it seems that very few crbug.com bugs require the GNU fchown (was: chown) behavior.
Could you just update the build instructions for these few packages?

In D93881#2483249, @manojgupta wrote:

In D93881#2483184, @MaskRay wrote:

I don't think we want to introduce a filename based chmod API. Unless it is essential (very unlikely), it is easy for developers to make time-of-check time-of-use (TOCTOU) race condition allowing bypass of protection mechanism due to symlink attacks.

binutils 2.36 will fix the issue https://sourceware.org/bugzilla/show_bug.cgi?id=26945

Can you please clarify what is being fixed, the use of internal renaming APIs or the current behavior?

GNU objcopy before 2.36 had the symlink attach vulnerability. The patch would introduce the similar vulnerability to llvm-objcopy.

Regarding whether llvm-objcopy should adopt the GNU objcopy smart_rename behavior, I am a bit hesitant, somewhat inclining to no.

It seems that very few packages actually require the behavior - most packages should use something like install -o ... -g ... -m ... to copy files, the owner/group is not too useful.

We are hitting this problem in Portage (used in Gentoo Linux and Chrome OS) which is both a build system and a package manager. One of the steps after building a package is to install the files with the required permissions. And another action is to strip the files and add the debuginfo link etc. These steps execute as root privilege in portage as they modify the file system and we do not have control over it.

Because of the deviation from GNU objcopy/strip, stripping or just adding debuginfo link causes the "security sensitive" binaries to lose the intended permissions. We already had to revert twice switching to llvm objcopy/strip for this reason in Chrome OS before we figured out the problem. Therefore, I'd argue that this change is needed for GNU compatibility.
It is also what most tools already do when dealing with existing files e.g. "sudo vi foo.c" or "echo foo |& sudo tee a.c" does not change file ownership or group.

It's different. vi foo.c and tee a.c edits the file in place while strip/llvm-strip creates a temporary file and renames it to the original name.

[manojgupta]

In D93881#2483542, @MaskRay wrote:

GNU objcopy and strip preserve the ownership of a file when it's overwritten, e.g. while "sudo objcopy foo -o bar" creates a new file bar with root access, "sudo objcopy foo" will keep the owner and group of foo unchanged. Currently llvm-objcopy and llvm-strip behave differently, always changing the ownership to root.

objcopy foo -o bar => objcopy foo bar

I am unsure that we want to adopt the GNU behavior because

(1) there were security vulnerabilities
(2) the code is subtle and untestable
(3) the behavior is self inconsistent. The behaviors of sudo objcopy foo and sudo -u mail objcopy foo (if user mail has write permission to the directory) are different.

While I can understand the hesitation, we have a real need for this. And IIUC, even the trunk GNU tools has the existing old behavior, though they may have changed the internal implementation.

(4) I believe very few packages require the behavior. It can only be used by root (CAP_CHOWN), so it is a very minor use case.

I think I already provided examples of usage in Gentoo and Chrome OS that llvm objcopy breaks. This does not seem to be a case of minor use case to me.

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,

Not having a behavior has pros and cons and I understand that it may sometimes cause friction.
But from https://chromium-review.googlesource.com/q/gnu+strip , it seems that very few crbug.com bugs require the GNU fchown (was: chown) behavior.

Please see the query links below for a more accurate count.

Could you just update the build instructions for these few packages?

I see over 600 uses of of fowners in Gentoo and over 55 in Chrome OS (public). Not all uses are related to binary files but asking to fix the build system instead of fixing the incompatibility is a non-trivial and a no-go.

[1] https://github.com/gentoo/gentoo/search?q=fowners&type=code
[2] https://source.chromium.org/search?q=fowners&sq=&ss=chromiumos

In D93881#2483249, @manojgupta wrote:

In D93881#2483184, @MaskRay wrote:

I don't think we want to introduce a filename based chmod API. Unless it is essential (very unlikely), it is easy for developers to make time-of-check time-of-use (TOCTOU) race condition allowing bypass of protection mechanism due to symlink attacks.

binutils 2.36 will fix the issue https://sourceware.org/bugzilla/show_bug.cgi?id=26945

Can you please clarify what is being fixed, the use of internal renaming APIs or the current behavior?

GNU objcopy before 2.36 had the symlink attach vulnerability. The patch would introduce the similar vulnerability to llvm-objcopy.

Regarding whether llvm-objcopy should adopt the GNU objcopy smart_rename behavior, I am a bit hesitant, somewhat inclining to no.

It seems that very few packages actually require the behavior - most packages should use something like install -o ... -g ... -m ... to copy files, the owner/group is not too useful.

We are hitting this problem in Portage (used in Gentoo Linux and Chrome OS) which is both a build system and a package manager. One of the steps after building a package is to install the files with the required permissions. And another action is to strip the files and add the debuginfo link etc. These steps execute as root privilege in portage as they modify the file system and we do not have control over it.

Because of the deviation from GNU objcopy/strip, stripping or just adding debuginfo link causes the "security sensitive" binaries to lose the intended permissions. We already had to revert twice switching to llvm objcopy/strip for this reason in Chrome OS before we figured out the problem. Therefore, I'd argue that this change is needed for GNU compatibility.
It is also what most tools already do when dealing with existing files e.g. "sudo vi foo.c" or "echo foo |& sudo tee a.c" does not change file ownership or group.

It's different. vi foo.c and tee a.c edits the file in place while strip/llvm-strip creates a temporary file and renames it to the original name.

I am simply trying to point out the normal behavior of tools when modifying existing files by various. The internal implementation obviously will vary between tools. From user point of view, it does not matter if the tool does a copy/rename or in place modification to do so.

[MaskRay]

In D93881#2483564, @manojgupta wrote:

In D93881#2483542, @MaskRay wrote:

GNU objcopy and strip preserve the ownership of a file when it's overwritten, e.g. while "sudo objcopy foo -o bar" creates a new file bar with root access, "sudo objcopy foo" will keep the owner and group of foo unchanged. Currently llvm-objcopy and llvm-strip behave differently, always changing the ownership to root.

objcopy foo -o bar => objcopy foo bar

I am unsure that we want to adopt the GNU behavior because

(1) there were security vulnerabilities
(2) the code is subtle and untestable
(3) the behavior is self inconsistent. The behaviors of sudo objcopy foo and sudo -u mail objcopy foo (if user mail has write permission to the directory) are different.

While I can understand the hesitation, we have a real need for this. And IIUC, even the trunk GNU tools has the existing old behavior, though they may have changed the internal implementation.

(4) I believe very few packages require the behavior. It can only be used by root (CAP_CHOWN), so it is a very minor use case.

I think I already provided examples of usage in Gentoo and Chrome OS that llvm objcopy breaks. This does not seem to be a case of minor use case to me.

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,

Not having a behavior has pros and cons and I understand that it may sometimes cause friction.
But from https://chromium-review.googlesource.com/q/gnu+strip , it seems that very few crbug.com bugs require the GNU fchown (was: chown) behavior.

Please see the query links below for a more accurate count.

Could you just update the build instructions for these few packages?

I see over 600 uses of of fowners in Gentoo and over 55 in Chrome OS (public). Not all uses are related to binary files but asking to fix the build system instead of fixing the incompatibility is a non-trivial and a no-go.

[1] https://github.com/gentoo/gentoo/search?q=fowners&type=code
[2] https://source.chromium.org/search?q=fowners&sq=&ss=chromiumos

OK. If that is the case, I am happy that llvm-objcopy adopts the GNU objcopy behavior.

But I think we should do better: instead of: (1) create a temp file (2) open it by name again (3) check it is a regular file with one hard link & it is writable (S_IWUSR) (4) fstat (5) fchown,
we should just modify the destination in place (for objcopy file and strip file). If the file has been mapped readonly, on many platforms it may be unwritable. We have to accept this.

The various security checks is to prevent vulnerability.

In D93881#2483249, @manojgupta wrote:

In D93881#2483184, @MaskRay wrote:

I don't think we want to introduce a filename based chmod API. Unless it is essential (very unlikely), it is easy for developers to make time-of-check time-of-use (TOCTOU) race condition allowing bypass of protection mechanism due to symlink attacks.

binutils 2.36 will fix the issue https://sourceware.org/bugzilla/show_bug.cgi?id=26945

Can you please clarify what is being fixed, the use of internal renaming APIs or the current behavior?

GNU objcopy before 2.36 had the symlink attach vulnerability. The patch would introduce the similar vulnerability to llvm-objcopy.

Regarding whether llvm-objcopy should adopt the GNU objcopy smart_rename behavior, I am a bit hesitant, somewhat inclining to no.

It seems that very few packages actually require the behavior - most packages should use something like install -o ... -g ... -m ... to copy files, the owner/group is not too useful.

We are hitting this problem in Portage (used in Gentoo Linux and Chrome OS) which is both a build system and a package manager. One of the steps after building a package is to install the files with the required permissions. And another action is to strip the files and add the debuginfo link etc. These steps execute as root privilege in portage as they modify the file system and we do not have control over it.

Because of the deviation from GNU objcopy/strip, stripping or just adding debuginfo link causes the "security sensitive" binaries to lose the intended permissions. We already had to revert twice switching to llvm objcopy/strip for this reason in Chrome OS before we figured out the problem. Therefore, I'd argue that this change is needed for GNU compatibility.
It is also what most tools already do when dealing with existing files e.g. "sudo vi foo.c" or "echo foo |& sudo tee a.c" does not change file ownership or group.

It's different. vi foo.c and tee a.c edits the file in place while strip/llvm-strip creates a temporary file and renames it to the original name.

I am simply trying to point out the normal behavior of tools when modifying existing files by various. The internal implementation obviously will vary between tools. From user point of view, it does not matter if the tool does a copy/rename or in place modification to do so.

[manojgupta]

Could you just update the build instructions for these few packages?

I see over 600 uses of of fowners in Gentoo and over 55 in Chrome OS (public). Not all uses are related to binary files but asking to fix the build system instead of fixing the incompatibility is a non-trivial and a no-go.

[1] https://github.com/gentoo/gentoo/search?q=fowners&type=code
[2] https://source.chromium.org/search?q=fowners&sq=&ss=chromiumos

OK. If that is the case, I am happy that llvm-objcopy adopts the GNU objcopy behavior.

But I think we should do better: instead of: (1) create a temp file (2) open it by name again (3) check it is a regular file with one hard link & it is writable (S_IWUSR) (4) fstat (5) fchown,
we should just modify the destination in place (for objcopy file and strip file). If the file has been mapped readonly, on many platforms it may be unwritable. We have to accept this.

The various security checks is to prevent vulnerability.

Thanks, we will work on an improved patch with the added checks.

[rupprecht]

From a user standpoint, I agree that "objcopy foo.o" should not be changing the ownership of a file, regardless of the implementation (which, in the case of llvm-objcopy, is getting it "wrong" compared to GNU objcopy, because we use temporary file and rename it at the end, without fixing ownership). Sounds like there's agreement there.

I also wonder if we should make this logic uniform to be least surprising, e.g. "llvm-objcopy foo.o bar.o" or "llvm-strip foo.o -o bar.o" should similarly have the same ownership for foo.o and bar.o. Also, if "llvm-objcopy foo.o --split-dwo=foo.dwo" should propagate ownership of foo.o to foo.dwo. As a user, that may be the most intuitive. This seems to break from what GNU objcopy does, however.

If so, I think we'd want to move some of this elsewhere, e.g. into FileOutputBuffer (chown the temporary file immediately instead of patching it at the end), or restoreStatOnFile (so it runs on both the object file and the split dwo file), so that it doesn't apply just to this narrow use case.

[jcai19]

GNU objcopy before 2.36 had the symlink attach vulnerability. The patch would introduce the similar vulnerability to llvm-objcopy.

@MaskRay, I am not familiar with this vulnerability, so just to clarify, my code can be exploited by a symlink attack after it opens the file but before it calls fchown, i.e. (1) llvm-objcopy opens the output file (2) attackers created a symlink with the same name, (3) fchown would change the ownership of the symlink instead of the intended file. Is that correct?

But I think we should do better: instead of: (1) create a temp file (2) open it by name again (3) check it is a regular file with one hard link & it is writable (S_IWUSR) (4) fstat (5) fchown,
we should just modify the destination in place (for objcopy file and strip file). If the file has been mapped readonly, on many platforms it may be unwritable. We have to accept this.

Can we use absolute path instead of relative path to shield the code from symlink attacks? This should also avoid the above-mentioned issue on readonly files.

EDIT: while testing about readonly files, I found another discrepancy between llvm-objcopy and objcopy

$ chmod -w foo
$ objcopy foo
objcopy: unable to copy file 'foo'; reason: Permission denied

The same commands worked with llvm-objcopy.

$ chmod -w foo
$ llvm-objcopy foo
$ echo $?
0

In-place write may fix that as well. I will start working on that approach.

[jcai19]

In D93881#2483674, @rupprecht wrote:

From a user standpoint, I agree that "objcopy foo.o" should not be changing the ownership of a file, regardless of the implementation (which, in the case of llvm-objcopy, is getting it "wrong" compared to GNU objcopy, because we use temporary file and rename it at the end, without fixing ownership). Sounds like there's agreement there.

Thanks for the support!

I also wonder if we should make this logic uniform to be least surprising, e.g. "llvm-objcopy foo.o bar.o" or "llvm-strip foo.o -o bar.o" should similarly have the same ownership for foo.o and bar.o. Also, if "llvm-objcopy foo.o --split-dwo=foo.dwo" should propagate ownership of foo.o to foo.dwo. As a user, that may be the most intuitive. This seems to break from what GNU objcopy does, however.

This sounds interesting and probably is worth another thread for discussion.

If so, I think we'd want to move some of this elsewhere, e.g. into FileOutputBuffer (chown the temporary file immediately instead of patching it at the end), or restoreStatOnFile (so it runs on both the object file and the split dwo file), so that it doesn't apply just to this narrow use case.

IMO it's safer to decouple the GNU-compatible and GNU-incompatible changes in case the latter breaks existing code and we need to revert it before we find a long-term solution.

[jcai19]

But I think we should do better: instead of: (1) create a temp file (2) open it by name again (3) check it is a regular file with one hard link & it is writable (S_IWUSR) (4) fstat (5) fchown,
we should just modify the destination in place (for objcopy file and strip file). If the file has been mapped readonly, on many platforms it may be unwritable. We have to accept this.

@MaskRay I took a look at the how ELF binaries are handled and from what I understand, the code directly copies the content of the input file into temporary file (https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/Object.cpp#L1786), before replacing the output file with it. So unless we are willing to allocate memory space to save its content (which will be very costive for large binaries), it does not appear modifying the input file in place is easy. And if we ever decide to take that path, it looks like we would have to make a lot of changes as the code of interest is buried deeply in the call hierarchy, and in addition we would likely have to change how the other file formats handle such cases to be consistent. Please let me know if I misunderstood your proposal or overlooked anything, but so far it appears to me the best place to preserve the file ownership without major change to the code is in the proposed place.

[MaskRay]

In D93881#2499664, @jcai19 wrote:

But I think we should do better: instead of: (1) create a temp file (2) open it by name again (3) check it is a regular file with one hard link & it is writable (S_IWUSR) (4) fstat (5) fchown,
we should just modify the destination in place (for objcopy file and strip file). If the file has been mapped readonly, on many platforms it may be unwritable. We have to accept this.

@MaskRay I took a look at the how ELF binaries are handled and from what I understand, the code directly copies the content of the input file into temporary file (https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/Object.cpp#L1786), before replacing the output file with it. So unless we are willing to allocate memory space to save its content (which will be very costive for large binaries), it does not appear modifying the input file in place is easy. And if we ever decide to take that path, it looks like we would have to make a lot of changes as the code of interest is buried deeply in the call hierarchy, and in addition we would likely have to change how the other file formats handle such cases to be consistent. Please let me know if I misunderstood your proposal or overlooked anything, but so far it appears to me the best place to preserve the file ownership without major change to the code is in the proposed place.

If that case, I'll personally want to rethink whether the Gentoo usage is a case we want to support directly. I have mentioned that the GNU objcopy/strip smart_rename still has vulnerability and the owner preservation does not work for non-root (CAP_CHOWN) users.

chmod o+wx .
sudo chown mail a.o
sudo -u bin strip a.o  # owner becomes bin. llvm-strip has the same behavior.

You can create a wrapper script for strip used by root.

CC some Gentoo folks: @mgorny @robertu94

[jcai19]

I have mentioned that the GNU objcopy/strip smart_rename still has vulnerability and the owner preservation does not work for non-root (CAP_CHOWN) users.

Can this be solved by limiting the affected user to root only?

chmod o+wx .
sudo chmod mail a.o
sudo -u bin strip a.o  # owner becomes bin. llvm-strip has the same behavior.

I tried to verify with this example but failed. Could you share the complete list of commands? Thanks.

[robertu94]

I could be wrong (@mgorny would know better), but it was my understanding that portage (if sandbox and userpriv FEATURES in are set in make.conf or not overwritten from /usr/share/portage/config/make.globals) drops user privileges to the portage user and group using the setuid/setgid functions during the unpack, prepare, configure, compile, and test phases. During the install process fakeroot can be used (but not the default), but I don't believe that privileges can be dropped separately from fakeroot.

[jcai19]

Update the ownership of the temporary file, instead of the overwritten file.

[manojgupta]

llvm/lib/Support/FileOutputBuffer.cpp

142

Can we pass the file descriptor (File.FD) directly?

llvm/lib/Support/Unix/Path.inc

1216

Please use the existing file descriptor and call fchown. I don't think there is any need to open and close the file if file is already open.

[jcai19]

Address @manojgupta's comments.

[jhenderson]

llvm/include/llvm/Support/FileOutputBuffer.h

38

Nit: comments should end with a . (same above).

llvm/lib/Support/FileOutputBuffer.cpp

131

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?

llvm/lib/Support/Unix/Path.inc

1216

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.

[rupprecht]

llvm/lib/Support/FileOutputBuffer.cpp

131

Calling setfsuid was suggested at one point, but that isn't a portable solution. Something like that would be the ideal fix IMO.

[jcai19]

Add trailing comma to comments.

[jcai19]

llvm/lib/Support/FileOutputBuffer.cpp

131

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.

llvm/lib/Support/Unix/Path.inc

1216

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.

[jhenderson]

Thanks for the update. No more comments from me at this time, but I'd prefer others to finish looking and approve it.

[rupprecht]

llvm/lib/Support/FileOutputBuffer.cpp

140

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)

208

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.

llvm/lib/Support/Unix/Path.inc

1215

auto FChown

[rupprecht]

llvm/lib/Support/FileOutputBuffer.cpp

140

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.

[jcai19]

llvm/lib/Support/FileOutputBuffer.cpp

140

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."

208

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?

[jcai19]

llvm/lib/Support/FileOutputBuffer.cpp

208

To further clarify, I meant we could create less dependency between FileOutputBuffer and createOnDiskBuffer with the current implementation so I feel it could be marginally better. Also correction to my typo, WDTY -> WDYT.

[jcai19]

Limit the patch to root user.

[manojgupta]

llvm/lib/Support/FileOutputBuffer.cpp

142

The check for root user looks incorrect to me. The stat check for root should be done on the temp file, not the existing file.

[jcai19]

llvm/lib/Support/FileOutputBuffer.cpp

142

Yes, I realized it the moment I sent the update. Will fix it.

[manojgupta]

llvm/lib/Support/FileOutputBuffer.cpp

142

I also think that calling fstat (fd) is better than stat. So I think using the existing status function which uses file descriptor is better here: std::error_code status(int FD, file_status &Result)

[rupprecht]

llvm/lib/Support/FileOutputBuffer.cpp

140

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.

[jcai19]

Check the user of the temporary file instead of the output file to decide the user that invokes the tool.

[jcai19]

llvm/lib/Support/FileOutputBuffer.cpp

140

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.

[rupprecht]

llvm/lib/Support/FileOutputBuffer.cpp

140

I do realize that fstat is just stat but applied to a file descriptor. But the point is that relying on the stat properties to not change is a race if you use path names instead of file descriptors.

The idea is to do:

1. int input_fd = fopen("input-file.o") // llvm-objcopy copies sections etc. from this fd
2. stat input_stat = fstat(input_fd)
3. fchown(output_fd, input_stat.uid, input_stat.gid)

As opposed to:

1. int input_fd = fopen("input-file.o") // llvm-objcopy copies sections etc. from this fd
2. Attacker changes "input-file.o"
3. stat input_stat = stat("input-file.o") // stat coming from the wrong file
4. fchown(output_fd, input_stat.uid, input_stat.gid) // wrong chown applied

[manojgupta]

llvm/lib/Support/FileOutputBuffer.cpp

140

yes, I think this should be doable to avoid the call to stat completely here since objcopy already has the Stat information for the input file.
Jian, please see my comment for llvm-objcopy.cpp file.

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

275

Since original file Stat is available, please re-use this instead of calling stat again on original file later.

[jcai19]

Reuse the result of stat instead of calling it on the output file again.

llvm/lib/Support/FileOutputBuffer.cpp

140

@rupprecht @manojgupta Thank you for the clarification, now I understand what you mean :). I've updated my patch to reuse the stat from earlier, as you suggest.

[MaskRay]

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

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

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 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.

llvm/lib/Support/FileOutputBuffer.cpp

137

if requested.

[jcai19]

In 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 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.

[MaskRay]

In D93881#2558677, @jcai19 wrote:

In 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.

If you use arc diff 'HEAD^', you need --verbatim to amend the description.

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 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.

[jcai19]

If you use arc diff 'HEAD^', you need --verbatim to amend the description.

Gotcha. Thanks. I've updated the message.

[jcai19]

Update a comment.

[MaskRay]

always changing the owner and gropu to root. The

gropu -> group.

The subsequent bullet points can use 1/2/3/4 instead of 1/1/1/1.

[jcai19]

Landed at https://reviews.llvm.org/rGc2a84771bb63947695ea50b89160c02b36fb634d.

llvm/include/llvm/Support/FileOutputBuffer.h

 /// objects, the content or existence of the specified file is undefined. That
 /// is, creating an OutputBuffer for a file may immediately remove the file.
 /// If the FileOutputBuffer is committed, the target file's content will become
 /// the buffer content at the time of the commit.  If the FileOutputBuffer is
 /// not committed, the file will be deleted in the FileOutputBuffer destructor.
 class FileOutputBuffer {
 public:
   enum {
-    /// set the 'x' bit on the resulting file
+    /// Set the 'x' bit on the resulting file.
     F_executable = 1,
 
     /// Don't use mmap and instead write an in-memory buffer to a file when this
     /// buffer is closed.
     F_no_mmap = 2,
+
+    /// Preserve ownership if the file already exists.
+    F_keep_ownership = 4,
   };
 
   /// Factory method to create an OutputBuffer object which manages a read/write
   /// buffer of the specified size. When committed, the buffer will be written
   /// to the file at the specified path.
   ///
   /// When F_modify is specified and \p FilePath refers to an existing on-disk
   /// file \p Size may be set to -1, in which case the entire file is used.

llvm/include/llvm/Support/FileSystem.h

 ///
 /// @param F On input, this is the file to close.  On output, the file is
 /// set to kInvalidFile.
 ///
 /// @returns An error code if closing the file failed. Typically, an error here
 /// means that the filesystem may have failed to perform some buffered writes.
 std::error_code closeFile(file_t &F);
 
+#ifdef LLVM_ON_UNIX
+/// @brief Change ownership of a file.
+///
+/// @param Owner The owner of the file to change to.
+/// @param Group The group of the file to change to.
+/// @returns errc::success if successfully updated file ownership, otherwise an
+///          error code is returned.
+std::error_code changeFileOwnership(int FD, uint32_t Owner, uint32_t Group);
+#endif
+
 /// RAII class that facilitates file locking.
 class FileLocker {
   int FD; ///< Locked file handle.
   FileLocker(int FD) : FD(FD) {}
   friend class llvm::raw_fd_ostream;
 
 public:
   FileLocker(const FileLocker &L) = delete;

llvm/lib/Support/FileOutputBuffer.cpp

createInMemoryBuffer(StringRef Path, size_t Size, unsigned Mode) {
   MemoryBlock MB = Memory::allocateMappedMemory(
       Size, nullptr, sys::Memory::MF_READ | sys::Memory::MF_WRITE, EC);
   if (EC)
     return errorCodeToError(EC);
   return std::make_unique<InMemoryBuffer>(Path, MB, Size, Mode);
 }
 
 static Expected<std::unique_ptr<FileOutputBuffer>>
-createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode) {
+createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode,
+                   bool KeepOwnership) {
   Expected<fs::TempFile> FileOrErr =
       fs::TempFile::create(Path + ".tmp%%%%%%%", Mode);
   if (!FileOrErr)
     return FileOrErr.takeError();
   fs::TempFile File = std::move(*FileOrErr);
 
 #ifndef _WIN32
+  // Try to preserve file ownership if it exists
+  if (KeepOwnership) {
+    fs::file_status Stat;
+    std::error_code EC = fs::status(Path, Stat);
+    if (!EC)
+      fs::changeFileOwnership(File.FD, Stat.getUser(), Stat.getGroup());
+  }
+
   // On Windows, CreateFileMapping (the mmap function on Windows)
   // automatically extends the underlying file. We don't need to
   // extend the file beforehand. _chsize (ftruncate on Windows) is
   // pretty slow just like it writes specified amount of bytes,
   // so we should avoid calling that function.
   if (auto EC = fs::resize_file(File.FD, Size)) {
     consumeError(File.discard());
     return errorCodeToError(EC);
FileOutputBuffer::create(StringRef Path, size_t Size, unsigned Flags) {
   case fs::file_type::directory_file:
     return errorCodeToError(errc::is_a_directory);
   case fs::file_type::regular_file:
   case fs::file_type::file_not_found:
   case fs::file_type::status_error:
     if (Flags & F_no_mmap)
       return createInMemoryBuffer(Path, Size, Mode);
     else
-      return createOnDiskBuffer(Path, Size, Mode);
+      return createOnDiskBuffer(Path, Size, Mode, Flags & F_keep_ownership);
   default:
     return createInMemoryBuffer(Path, Size, Mode);
   }
 }

llvm/lib/Support/Unix/Path.inc

 #endif
 
 using namespace llvm;
 
 namespace llvm {
 namespace sys  {
 namespace fs {
 
 const file_t kInvalidFile = -1;
 
 #if defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) ||     \
     defined(__minix) || defined(__FreeBSD_kernel__) || defined(__linux__) ||   \
     defined(__CYGWIN__) || defined(__DragonFly__) || defined(_AIX) || defined(__GNU__)
 static int
 test_dir(char ret[PATH_MAX], const char *dir, const char *bin)
 {
   struct stat sb;
if (sys::fs::exists(curproc)) {
     }
   }
   // If we don't have procfs mounted, fall back to argv[0]
   if (getprogpath(exe_path, argv0) != NULL)
     return exe_path;
 #elif defined(__linux__) || defined(__CYGWIN__) || defined(__gnu_hurd__)
   char exe_path[PATH_MAX];
   const char *aPath = "/proc/self/exe";
   if (sys::fs::exists(aPath)) {
     // /proc is not always mounted under Linux (chroot for example).
     ssize_t len = readlink(aPath, exe_path, sizeof(exe_path));
     if (len < 0)
       return "";
 
     // Null terminate the string for realpath. readlink never null
     // terminates its output.
     len = std::min(len, ssize_t(sizeof(exe_path) - 1));
#elif defined(HAVE_DLFCN_H) && defined(HAVE_DLADDR)
   if (realpath(DLInfo.dli_fname, link_path))
     return link_path;
 #else
 #error GetMainExecutable is not implemented on this host yet.
 #endif
   return "";
 }
 
 TimePoint<> basic_file_status::getLastAccessedTime() const {
   return toTimePoint(fs_st_atime, fs_st_atime_nsec);
 }
 
 TimePoint<> basic_file_status::getLastModificationTime() const {
   return toTimePoint(fs_st_mtime, fs_st_mtime_nsec);
 }
 
 UniqueID file_status::getUniqueID() const {
   return UniqueID(fs_st_dev, fs_st_ino);
 }
 
 uint32_t file_status::getLinkCount() const {
   return fs_st_nlinks;
 }
 
 ErrorOr<space_info> disk_space(const Twine &Path) {
   struct STATVFS Vfs;
   if (::STATVFS(const_cast<char *>(Path.str().c_str()), &Vfs))
     return std::error_code(errno, std::generic_category());
   auto FrSize = STATVFS_F_FRSIZE(Vfs);
   space_info SpaceInfo;
   SpaceInfo.capacity = static_cast<uint64_t>(Vfs.f_blocks) * FrSize;
   SpaceInfo.free = static_cast<uint64_t>(Vfs.f_bfree) * FrSize;
   SpaceInfo.available = static_cast<uint64_t>(Vfs.f_bavail) * FrSize;
   return SpaceInfo;
 }
 
 std::error_code current_path(SmallVectorImpl<char> &result) {
   result.clear();
 
   const char *pwd = ::getenv("PWD");
   llvm::sys::fs::file_status PWDStatus, DotStatus;
   if (pwd && llvm::sys::path::is_absolute(pwd) &&
       !llvm::sys::fs::status(pwd, PWDStatus) &&
       !llvm::sys::fs::status(".", DotStatus) &&
       PWDStatus.getUniqueID() == DotStatus.getUniqueID()) {
     result.append(pwd, pwd + strlen(pwd));
     return std::error_code();
   }
 
   result.reserve(PATH_MAX);
 
   while (true) {
     if (::getcwd(result.data(), result.capacity()) == nullptr) {
       // See if there was a real error.
       if (errno != ENOMEM)
         return std::error_code(errno, std::generic_category());
       // Otherwise there just wasn't enough space.
       result.reserve(result.capacity() * 2);
     } else
       break;
   }
 
   result.set_size(strlen(result.data()));
   return std::error_code();
 }
 
 std::error_code set_current_path(const Twine &path) {
   SmallString<128> path_storage;
   StringRef p = path.toNullTerminatedStringRef(path_storage);
 
   if (::chdir(p.begin()) == -1)
     return std::error_code(errno, std::generic_category());
 
   return std::error_code();
 }
 
 std::error_code create_directory(const Twine &path, bool IgnoreExisting,
                                  perms Perms) {
   SmallString<128> path_storage;
   StringRef p = path.toNullTerminatedStringRef(path_storage);
 
   if (::mkdir(p.begin(), Perms) == -1) {
     if (errno != EEXIST || !IgnoreExisting)
       return std::error_code(errno, std::generic_category());
   }
 
   return std::error_code();
std::error_code real_path(const Twine &path, SmallVectorImpl<char> &dest,
   StringRef P = path.toNullTerminatedStringRef(Storage);
   char Buffer[PATH_MAX];
   if (::realpath(P.begin(), Buffer) == nullptr)
     return std::error_code(errno, std::generic_category());
   dest.append(Buffer, Buffer + strlen(Buffer));
   return std::error_code();
 }
 
+std::error_code changeFileOwnership(int FD, uint32_t Owner, uint32_t Group) {
+  auto Chown = [&]() { return ::fchown(FD, Owner, Group); };
+  // Retry if fchown call fails due to interruption.
+  if ((sys::RetryAfterSignal(-1, Chown)) < 0)
+    return std::error_code(errno, std::generic_category());
+  return std::error_code();
+}
+
 } // end namespace fs
 
 namespace path {
 
 bool home_directory(SmallVectorImpl<char> &result) {
   char *RequestedDir = getenv("HOME");
   if (!RequestedDir) {
     struct passwd *pw = getpwuid(getuid());

llvm/tools/llvm-objcopy/Buffer.h

public:
   StringRef getName() const { return Name; }
 };
 
 class FileBuffer : public Buffer {
   std::unique_ptr<FileOutputBuffer> Buf;
   // Indicates that allocate(0) was called, and commit() should create or
   // truncate a file instead of using a FileOutputBuffer.
   bool EmptyFile = false;
+  bool KeepOwnership = false;
 
 public:
   Error allocate(size_t Size) override;
   uint8_t *getBufferStart() override;
   Error commit() override;
 
   explicit FileBuffer(StringRef FileName) : Buffer(FileName) {}
+  explicit FileBuffer(StringRef FileName, bool Keep)
+      : Buffer(FileName), KeepOwnership(Keep) {}
 };
 
 class MemBuffer : public Buffer {
   std::unique_ptr<WritableMemoryBuffer> Buf;
 
 public:
   Error allocate(size_t Size) override;
   uint8_t *getBufferStart() override;

llvm/tools/llvm-objcopy/Buffer.cpp

Error FileBuffer::allocate(size_t Size) {
   // creation/truncation until commit() to avoid side effects if something
   // happens between allocate() and commit().
   if (Size == 0) {
     EmptyFile = true;
     return Error::success();
   }
 
   Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
-      FileOutputBuffer::create(getName(), Size, FileOutputBuffer::F_executable);
+      FileOutputBuffer::create(getName(), Size,
+                               KeepOwnership
+                                   ? FileOutputBuffer::F_executable |
+                                         FileOutputBuffer::F_keep_ownership
+                                   : FileOutputBuffer::F_executable);
   // FileOutputBuffer::create() returns an Error that is just a wrapper around
   // std::error_code. Wrap it in FileError to include the actual filename.
   if (!BufferOrErr)
     return createFileError(getName(), BufferOrErr.takeError());
   Buf = std::move(*BufferOrErr);
   return Error::success();
 }
 

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

#endif
 
   return Error::success();
 }
 
 /// The function executeObjcopy does the higher level dispatch based on the type
 /// of input (raw binary, archive or single object file) and takes care of the
 /// format-agnostic modifications, i.e. preserving dates.
 static Error executeObjcopy(CopyConfig &Config) {
   sys::fs::file_status Stat;
   if (Config.InputFilename != "-") {
     if (auto EC = sys::fs::status(Config.InputFilename, Stat))
       return createFileError(Config.InputFilename, EC);
   } else {
     Stat.permissions(static_cast<sys::fs::perms>(0777));
   }
 
   using ProcessRawFn = Error (*)(CopyConfig &, MemoryBuffer &, Buffer &);
Expected<OwningBinary<llvm::object::Binary>> BinaryOrErr =
         createBinary(Config.InputFilename);
     if (!BinaryOrErr)
       return createFileError(Config.InputFilename, BinaryOrErr.takeError());
 
     if (Archive *Ar = dyn_cast<Archive>(BinaryOrErr.get().getBinary())) {
       if (Error E = executeObjcopyOnArchive(Config, *Ar))
         return E;
     } else {
-      FileBuffer FB(Config.OutputFilename);
+      FileBuffer FB(Config.OutputFilename,
+                    Config.InputFilename != "-" &&
+                        Config.InputFilename == Config.OutputFilename);
       if (Error E = executeObjcopyOnBinary(Config,
                                            *BinaryOrErr.get().getBinary(), FB))
         return E;
     }
   }
 
   if (Error E =
           restoreStatOnFile(Config.OutputFilename, Stat, Config.PreserveDates))
     return E;
 
   if (!Config.SplitDWO.empty()) {
     Stat.permissions(static_cast<sys::fs::perms>(0666));
     if (Error E =
             restoreStatOnFile(Config.SplitDWO, Stat, Config.PreserveDates))
       return E;
   }
 
   return Error::success();
 }
 
 namespace {
 
 } // anonymous namespace
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   ToolName = argv[0];