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

[Dan Gohman via llvm-commits]

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/20170404/38dadea6/attachment.html>