[llvm] r297624 - Fix crash when multiple raw_fd_ostreams to stdout are created.

Tue Apr 11 08:10:20 PDT 2017

Obviously one can't prevent it in all cases, because there will always be
things like symlinks or ".." paths or other things, but it does seem
reasonable to check whether the -MF output filename is literally the same
string as the -o filename. That should catch most common user errors, and
it would be sufficient to avoid the double-close problem, obviating the
need for the dup call in the patch here.

Dan

On Wed, Apr 5, 2017 at 5:55 AM, Rafael Espíndola <rafael.espindola at gmail.com
> wrote:

> But a similar symptom shows up if a file is open twice, no? If we are
> going to err on two streams writing to '-', we should also err on two
> streams writing to 'foo.txt', no?
>
> How would you detect that?
>
> Cheers,
> Rafael
>
>
> On 4 April 2017 at 19:22, Dan Gohman <sunfish at mozilla.com> wrote:
> > I just saw this patch; sorry for the delay.
> >
> > The best thing to do when given "clang -S -o - -MD -MF - test.cpp" is to
> > issue a diagnostic to the user. In general, it's not safe to have two
> > different buffered output streams using the same output file, because
> > there's a potential for them to interleave their output in surprising
> ways.
> > There's nothing ensuring that the buffer in one raw_ostream is flushed
> > before a write to the other. raw_ostream doesn't even to line-buffering,
> so
> > data from the two different buffered streams could be interleaved even
> > within lines.
> >
> > Consequently, having raw_ostream dup(2) a file descriptor for the
> purpose of
> > allowing multiple buffered output streams to share an output file, seems
> > like fixing a symptom rather than fixing the underlying bug.
> >
> > Dan
> >
> >
> > On Mon, Mar 13, 2017 at 7:45 AM, Rafael Espindola via llvm-commits
> > <llvm-commits at lists.llvm.org> wrote:
> >>
> >> Author: rafael
> >> Date: Mon Mar 13 09:45:06 2017
> >> New Revision: 297624
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=297624&view=rev
> >> Log:
> >> Fix crash when multiple raw_fd_ostreams to stdout are created.
> >>
> >> If raw_fd_ostream is constructed with the path of "-", it claims
> >> ownership of the stdout file descriptor. This means that it closes
> >> stdout when it is destroyed. If there are multiple users of
> >> raw_fd_ostream wrapped around stdout, then a crash can occur because
> >> of operations on a closed stream.
> >>
> >> An example of this would be running something like "clang -S -o - -MD
> >> -MF - test.cpp". Alternatively, using outs() (which creates a local
> >> version of raw_fd_stream to stdout) anywhere combined with such a
> >> stream usage would cause the crash.
> >>
> >> The fix duplicates the stdout file descriptor when used within
> >> raw_fd_ostream, so that only that particular descriptor is closed when
> >> the stream is destroyed.
> >>
> >> Patch by James Henderson!
> >>
> >> Added:
> >>     llvm/trunk/test/Other/writing-to-stdout.ll
> >> Modified:
> >>     llvm/trunk/lib/Support/raw_ostream.cpp
> >>     llvm/trunk/unittests/Support/raw_ostream_test.cpp
> >>
> >> Modified: llvm/trunk/lib/Support/raw_ostream.cpp
> >> URL:
> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Support/raw_ostream.cpp?rev=297624&r1=297623&r2=297624&view=diff
> >>
> >> ============================================================
> ==================
> >> --- llvm/trunk/lib/Support/raw_ostream.cpp (original)
> >> +++ llvm/trunk/lib/Support/raw_ostream.cpp Mon Mar 13 09:45:06 2017
> >> @@ -473,7 +473,7 @@ static int getFD(StringRef Filename, std
> >>      // possible.
> >>      if (!(Flags & sys::fs::F_Text))
> >>        sys::ChangeStdoutToBinary();
> >> -    return STDOUT_FILENO;
> >> +    return dup(STDOUT_FILENO);
> >>    }
> >>
> >>    int FD;
> >>
> >> Added: llvm/trunk/test/Other/writing-to-stdout.ll
> >> URL:
> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/
> writing-to-stdout.ll?rev=297624&view=auto
> >>
> >> ============================================================
> ==================
> >> --- llvm/trunk/test/Other/writing-to-stdout.ll (added)
> >> +++ llvm/trunk/test/Other/writing-to-stdout.ll Mon Mar 13 09:45:06 2017
> >> @@ -0,0 +1,14 @@
> >> +; Often LLVM tools use "-" to indicate that output should be written to
> >> stdout
> >> +; instead of a file. This behaviour is implemented by the
> raw_fd_ostream
> >> class.
> >> +; This test verifies that when doing so multiple times we don't try to
> >> access a
> >> +; closed STDOUT_FILENO. The exact options used in this test are
> >> unimportant, as
> >> +; long as they write to stdout using raw_fd_ostream.
> >> +; RUN: llc %s -o=- -pass-remarks-output=- -filetype=asm | FileCheck %s
> >> +; foobar should appear as a function somewhere in the assembly file.
> >> +; CHECK: foobar
> >> +; !Analysis appears at the start of pass-remarks-output.
> >> +; CHECK: !Analysis
> >> +
> >> +define void @foobar() {
> >> +  ret void
> >> +}
> >>
> >> Modified: llvm/trunk/unittests/Support/raw_ostream_test.cpp
> >> URL:
> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/
> Support/raw_ostream_test.cpp?rev=297624&r1=297623&r2=297624&view=diff
> >>
> >> ============================================================
> ==================
> >> --- llvm/trunk/unittests/Support/raw_ostream_test.cpp (original)
> >> +++ llvm/trunk/unittests/Support/raw_ostream_test.cpp Mon Mar 13
> 09:45:06
> >> 2017
> >> @@ -9,6 +9,7 @@
> >>
> >>  #include "gtest/gtest.h"
> >>  #include "llvm/ADT/SmallString.h"
> >> +#include "llvm/Support/FileSystem.h"
> >>  #include "llvm/Support/Format.h"
> >>  #include "llvm/Support/raw_ostream.h"
> >>
> >> @@ -330,4 +331,11 @@ TEST(raw_ostreamTest, FormattedHexBytes)
> >>              "0007: 68 69 6a 6b 6c        |hijkl|",
> >>              format_bytes_with_ascii_str(B.take_front(12), 0, 7, 1));
> >>  }
> >> +
> >> +TEST(raw_fd_ostreamTest, multiple_raw_fd_ostream_to_stdout) {
> >> +  std::error_code EC;
> >> +
> >> +  { raw_fd_ostream("-", EC, sys::fs::OpenFlags::F_None); }
> >> +  { raw_fd_ostream("-", EC, sys::fs::OpenFlags::F_None); }
> >> +}
> >>  }
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170411/0fce57f7/attachment.html>