<div dir="ltr">Oops, thank you!<div>r334315 should fix this.<br></div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Jun 8, 2018 at 9:45 PM Kostya Serebryany <<a href="mailto:kcc@google.com">kcc@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Looks like this broke the clang-fuzzer: <div><a href="https://oss-fuzz-build-logs.storage.googleapis.com/index.html" target="_blank">https://oss-fuzz-build-logs.storage.googleapis.com/index.html</a><br><div><br></div><div><pre style="white-space:pre-wrap;color:rgb(0,0,0);background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial">Step #4: /src/llvm/tools/clang/tools/extra/clangd/fuzzer/ClangdFuzzer.cpp:31:17: error: no viable conversion from 'std::istringstream' (aka 'basic_istringstream<char>') to 'std::FILE *' (aka '_IO_FILE *')
Step #4: LSPServer.run(In);
Step #4: ^~
Step #4: /src/llvm/tools/clang/tools/extra/clangd/fuzzer/../ClangdLSPServer.h:46:23: note: passing argument to parameter 'In' here
Step #4: bool run(std::FILE *In,
Step #4: ^
Step #4: 1 error generated.
Step #4: ninja: build stopped: subcommand failed.</pre><br></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Jun 5, 2018 at 2:38 AM Sam McCall via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: sammccall<br>
Date: Tue Jun 5 02:34:46 2018<br>
New Revision: 333993<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=333993&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=333993&view=rev</a><br>
Log:<br>
[clangd] Rewrite JSON dispatcher loop using C IO (FILE*) instead of std::istream.<br>
<br>
Summary:<br>
The EINTR loop around getline was added to fix an issue with mac gdb, but seems<br>
to loop infinitely in rare cases on linux where the parent editor exits (most<br>
reports with VSCode).<br>
I can't work out how to fix this in a portable way with std::istream, but the<br>
C APIs have clearer contracts and LLVM has a RetryAfterSignal function for use<br>
with them which seems battle-tested.<br>
<br>
While here, clean up some inconsistency around \n in log messages (now<br>
add it only after JSON payloads), and reduce the scope of the<br>
long-message handling which was only really added to fight fuzzers.<br>
<br>
Reviewers: malaperle, ilya-biryukov<br>
<br>
Subscribers: klimek, ioeric, jkorous, cfe-commits<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D47643" rel="noreferrer" target="_blank">https://reviews.llvm.org/D47643</a><br>
<br>
Modified:<br>
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp<br>
clang-tools-extra/trunk/clangd/ClangdLSPServer.h<br>
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp<br>
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h<br>
clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp<br>
clang-tools-extra/trunk/test/clangd/too_large.test<br>
<br>
Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=333993&r1=333992&r2=333993&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=333993&r1=333992&r2=333993&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)<br>
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Tue Jun 5 02:34:46 2018<br>
@@ -396,7 +396,7 @@ ClangdLSPServer::ClangdLSPServer(JSONOut<br>
SupportedSymbolKinds(defaultSymbolKinds()),<br>
Server(CDB, FSProvider, /*DiagConsumer=*/*this, Opts) {}<br>
<br>
-bool ClangdLSPServer::run(std::istream &In, JSONStreamStyle InputStyle) {<br>
+bool ClangdLSPServer::run(std::FILE *In, JSONStreamStyle InputStyle) {<br>
assert(!IsDone && "Run was called before");<br>
<br>
// Set up JSONRPCDispatcher.<br>
<br>
Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.h?rev=333993&r1=333992&r2=333993&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.h?rev=333993&r1=333992&r2=333993&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.h (original)<br>
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h Tue Jun 5 02:34:46 2018<br>
@@ -42,8 +42,8 @@ public:<br>
/// class constructor. This method must not be executed more than once for<br>
/// each instance of ClangdLSPServer.<br>
///<br>
- /// \return Wether we received a 'shutdown' request before an 'exit' request<br>
- bool run(std::istream &In,<br>
+ /// \return Whether we received a 'shutdown' request before an 'exit' request.<br>
+ bool run(std::FILE *In,<br>
JSONStreamStyle InputStyle = JSONStreamStyle::Standard);<br>
<br>
private:<br>
<br>
Modified: clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp?rev=333993&r1=333992&r2=333993&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp?rev=333993&r1=333992&r2=333993&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp (original)<br>
+++ clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp Tue Jun 5 02:34:46 2018<br>
@@ -14,6 +14,7 @@<br>
#include "llvm/ADT/SmallString.h"<br>
#include "llvm/ADT/StringExtras.h"<br>
#include "llvm/Support/Chrono.h"<br>
+#include "llvm/Support/Errno.h"<br>
#include "llvm/Support/SourceMgr.h"<br>
#include <istream><br>
<br>
@@ -66,7 +67,7 @@ void JSONOutput::writeMessage(const json<br>
Outs << "Content-Length: " << S.size() << "\r\n\r\n" << S;<br>
Outs.flush();<br>
}<br>
- log(llvm::Twine("--> ") + S);<br>
+ log(llvm::Twine("--> ") + S + "\n");<br>
}<br>
<br>
void JSONOutput::log(const Twine &Message) {<br>
@@ -180,27 +181,43 @@ bool JSONRPCDispatcher::call(const json:<br>
return true;<br>
}<br>
<br>
-static llvm::Optional<std::string> readStandardMessage(std::istream &In,<br>
+// Tries to read a line up to and including \n.<br>
+// If failing, feof() or ferror() will be set.<br>
+static bool readLine(std::FILE *In, std::string &Out) {<br>
+ static constexpr int BufSize = 1024;<br>
+ size_t Size = 0;<br>
+ Out.clear();<br>
+ for (;;) {<br>
+ Out.resize(Size + BufSize);<br>
+ // Handle EINTR which is sent when a debugger attaches on some platforms.<br>
+ if (!llvm::sys::RetryAfterSignal(nullptr, ::fgets, &Out[Size], BufSize, In))<br>
+ return false;<br>
+ clearerr(In);<br>
+ // If the line contained null bytes, anything after it (including \n) will<br>
+ // be ignored. Fortunately this is not a legal header or JSON.<br>
+ size_t Read = std::strlen(&Out[Size]);<br>
+ if (Read > 0 && Out[Size + Read - 1] == '\n') {<br>
+ Out.resize(Size + Read);<br>
+ return true;<br>
+ }<br>
+ Size += Read;<br>
+ }<br>
+}<br>
+<br>
+// Returns None when:<br>
+// - ferror() or feof() are set.<br>
+// - Content-Length is missing or empty (protocol error)<br>
+static llvm::Optional<std::string> readStandardMessage(std::FILE *In,<br>
JSONOutput &Out) {<br>
// A Language Server Protocol message starts with a set of HTTP headers,<br>
// delimited by \r\n, and terminated by an empty line (\r\n).<br>
unsigned long long ContentLength = 0;<br>
- while (In.good()) {<br>
- std::string Line;<br>
- std::getline(In, Line);<br>
- if (!In.good() && errno == EINTR) {<br>
- In.clear();<br>
- continue;<br>
- }<br>
+ std::string Line;<br>
+ while (true) {<br>
+ if (feof(In) || ferror(In) || !readLine(In, Line))<br>
+ return llvm::None;<br>
<br>
Out.mirrorInput(Line);<br>
- // Mirror '\n' that gets consumed by std::getline, but is not included in<br>
- // the resulting Line.<br>
- // Note that '\r' is part of Line, so we don't need to mirror it<br>
- // separately.<br>
- if (!In.eof())<br>
- Out.mirrorInput("\n");<br>
-<br>
llvm::StringRef LineRef(Line);<br>
<br>
// We allow comments in headers. Technically this isn't part<br>
@@ -208,19 +225,13 @@ static llvm::Optional<std::string> readS<br>
if (LineRef.startswith("#"))<br>
continue;<br>
<br>
- // Content-Type is a specified header, but does nothing.<br>
- // Content-Length is a mandatory header. It specifies the length of the<br>
- // following JSON.<br>
- // It is unspecified what sequence headers must be supplied in, so we<br>
- // allow any sequence.<br>
- // The end of headers is signified by an empty line.<br>
+ // Content-Length is a mandatory header, and the only one we handle.<br>
if (LineRef.consume_front("Content-Length: ")) {<br>
if (ContentLength != 0) {<br>
log("Warning: Duplicate Content-Length header received. "<br>
"The previous value for this message (" +<br>
- llvm::Twine(ContentLength) + ") was ignored.\n");<br>
+ llvm::Twine(ContentLength) + ") was ignored.");<br>
}<br>
-<br>
llvm::getAsUnsignedInteger(LineRef.trim(), 0, ContentLength);<br>
continue;<br>
} else if (!LineRef.trim().empty()) {<br>
@@ -233,46 +244,45 @@ static llvm::Optional<std::string> readS<br>
}<br>
}<br>
<br>
- // Guard against large messages. This is usually a bug in the client code<br>
- // and we don't want to crash downstream because of it.<br>
+ // The fuzzer likes crashing us by sending "Content-Length: 9999999999999999"<br>
if (ContentLength > 1 << 30) { // 1024M<br>
- In.ignore(ContentLength);<br>
- log("Skipped overly large message of " + Twine(ContentLength) +<br>
- " bytes.\n");<br>
+ log("Refusing to read message with long Content-Length: " +<br>
+ Twine(ContentLength) + ". Expect protocol errors.");<br>
+ return llvm::None;<br>
+ }<br>
+ if (ContentLength == 0) {<br>
+ log("Warning: Missing Content-Length header, or zero-length message.");<br>
return llvm::None;<br>
}<br>
<br>
- if (ContentLength > 0) {<br>
- std::string JSON(ContentLength, '\0');<br>
- In.read(&JSON[0], ContentLength);<br>
- Out.mirrorInput(StringRef(JSON.data(), In.gcount()));<br>
-<br>
- // If the stream is aborted before we read ContentLength bytes, In<br>
- // will have eofbit and failbit set.<br>
- if (!In) {<br>
- log("Input was aborted. Read only " + llvm::Twine(In.gcount()) +<br>
- " bytes of expected " + llvm::Twine(ContentLength) + ".\n");<br>
+ std::string JSON(ContentLength, '\0');<br>
+ for (size_t Pos = 0, Read; Pos < ContentLength; Pos += Read) {<br>
+ // Handle EINTR which is sent when a debugger attaches on some platforms.<br>
+ Read = llvm::sys::RetryAfterSignal(0u, ::fread, &JSON[Pos], 1,<br>
+ ContentLength - Pos, In);<br>
+ Out.mirrorInput(StringRef(&JSON[Pos], Read));<br>
+ if (Read == 0) {<br>
+ log("Input was aborted. Read only " + llvm::Twine(Pos) +<br>
+ " bytes of expected " + llvm::Twine(ContentLength) + ".");<br>
return llvm::None;<br>
}<br>
- return std::move(JSON);<br>
- } else {<br>
- log("Warning: Missing Content-Length header, or message has zero "<br>
- "length.\n");<br>
- return llvm::None;<br>
+ clearerr(In); // If we're done, the error was transient. If we're not done,<br>
+ // either it was transient or we'll see it again on retry.<br>
+ Pos += Read;<br>
}<br>
+ return std::move(JSON);<br>
}<br>
<br>
// For lit tests we support a simplified syntax:<br>
// - messages are delimited by '---' on a line by itself<br>
// - lines starting with # are ignored.<br>
// This is a testing path, so favor simplicity over performance here.<br>
-static llvm::Optional<std::string> readDelimitedMessage(std::istream &In,<br>
+// When returning None, feof() or ferror() will be set.<br>
+static llvm::Optional<std::string> readDelimitedMessage(std::FILE *In,<br>
JSONOutput &Out) {<br>
std::string JSON;<br>
std::string Line;<br>
- while (std::getline(In, Line)) {<br>
- Line.push_back('\n'); // getline() consumed the newline.<br>
-<br>
+ while (readLine(In, Line)) {<br>
auto LineRef = llvm::StringRef(Line).trim();<br>
if (LineRef.startswith("#")) // comment<br>
continue;<br>
@@ -284,39 +294,47 @@ static llvm::Optional<std::string> readD<br>
JSON += Line;<br>
}<br>
<br>
- if (In.bad()) {<br>
+ if (ferror(In)) {<br>
log("Input error while reading message!");<br>
return llvm::None;<br>
- } else {<br>
+ } else { // Including EOF<br>
Out.mirrorInput(<br>
llvm::formatv("Content-Length: {0}\r\n\r\n{1}", JSON.size(), JSON));<br>
return std::move(JSON);<br>
}<br>
}<br>
<br>
-void clangd::runLanguageServerLoop(std::istream &In, JSONOutput &Out,<br>
+// The use of C-style std::FILE* IO deserves some explanation.<br>
+// Previously, std::istream was used. When a debugger attached on MacOS, the<br>
+// process received EINTR, the stream went bad, and clangd exited.<br>
+// A retry-on-EINTR loop around reads solved this problem, but caused clangd to<br>
+// sometimes hang rather than exit on other OSes. The interaction between<br>
+// istreams and signals isn't well-specified, so it's hard to get this right.<br>
+// The C APIs seem to be clearer in this respect.<br>
+void clangd::runLanguageServerLoop(std::FILE *In, JSONOutput &Out,<br>
JSONStreamStyle InputStyle,<br>
JSONRPCDispatcher &Dispatcher,<br>
bool &IsDone) {<br>
auto &ReadMessage =<br>
(InputStyle == Delimited) ? readDelimitedMessage : readStandardMessage;<br>
- while (In.good()) {<br>
+ while (!IsDone && !feof(In)) {<br>
+ if (ferror(In)) {<br>
+ log("IO error: " + llvm::sys::StrError());<br>
+ return;<br>
+ }<br>
if (auto JSON = ReadMessage(In, Out)) {<br>
if (auto Doc = json::parse(*JSON)) {<br>
// Log the formatted message.<br>
log(llvm::formatv(Out.Pretty ? "<-- {0:2}\n" : "<-- {0}\n", *Doc));<br>
// Finally, execute the action for this JSON message.<br>
if (!Dispatcher.call(*Doc, Out))<br>
- log("JSON dispatch failed!\n");<br>
+ log("JSON dispatch failed!");<br>
} else {<br>
// Parse error. Log the raw message.<br>
log(llvm::formatv("<-- {0}\n" , *JSON));<br>
log(llvm::Twine("JSON parse error: ") +<br>
- llvm::toString(Doc.takeError()) + "\n");<br>
+ llvm::toString(Doc.takeError()));<br>
}<br>
}<br>
- // If we're done, exit the loop.<br>
- if (IsDone)<br>
- break;<br>
}<br>
}<br>
<br>
Modified: clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h?rev=333993&r1=333992&r2=333993&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h?rev=333993&r1=333992&r2=333993&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h (original)<br>
+++ clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h Tue Jun 5 02:34:46 2018<br>
@@ -102,7 +102,9 @@ enum JSONStreamStyle {<br>
/// if it is.<br>
/// Input stream(\p In) must be opened in binary mode to avoid preliminary<br>
/// replacements of \r\n with \n.<br>
-void runLanguageServerLoop(std::istream &In, JSONOutput &Out,<br>
+/// We use C-style FILE* for reading as std::istream has unclear interaction<br>
+/// with signals, which are sent by debuggers on some OSs.<br>
+void runLanguageServerLoop(std::FILE *In, JSONOutput &Out,<br>
JSONStreamStyle InputStyle,<br>
JSONRPCDispatcher &Dispatcher, bool &IsDone);<br>
<br>
<br>
Modified: clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp?rev=333993&r1=333992&r2=333993&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp?rev=333993&r1=333992&r2=333993&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp (original)<br>
+++ clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp Tue Jun 5 02:34:46 2018<br>
@@ -238,5 +238,5 @@ int main(int argc, char *argv[]) {<br>
llvm::set_thread_name("clangd.main");<br>
// Change stdin to binary to not lose \r\n on windows.<br>
llvm::sys::ChangeStdinToBinary();<br>
- return LSPServer.run(std::cin, InputStyle) ? 0 : NoShutdownRequestErrorCode;<br>
+ return LSPServer.run(stdin, InputStyle) ? 0 : NoShutdownRequestErrorCode;<br>
}<br>
<br>
Modified: clang-tools-extra/trunk/test/clangd/too_large.test<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/too_large.test?rev=333993&r1=333992&r2=333993&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/too_large.test?rev=333993&r1=333992&r2=333993&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/test/clangd/too_large.test (original)<br>
+++ clang-tools-extra/trunk/test/clangd/too_large.test Tue Jun 5 02:34:46 2018<br>
@@ -4,4 +4,4 @@<br>
#<br>
Content-Length: 2147483648<br>
<br>
-# STDERR: Skipped overly large message<br>
+# STDERR: Refusing to read message<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>
</blockquote></div>