Commit 898dc6b9 authored by Peter Wu's avatar Peter Wu Committed by Brad King
Browse files

SystemTools: Fix GetLineFromStream to avoid libc++ bug on OS X 10.7

LLVM libc++ as included with Mac OS X 10.7 suffers from an issue where
the trailing character is discarded when the delimiter (LF) is not found
within the given buffer size (1024).  The returned length is also 1024
rather than 1023.

This issue results in truncated reads as observed with CMake 3.11.0 on
Mac OS X 10.7 and `cmake -E cmake_link_script link.txt`.

Solve this by replacing `istream::getline` by `std::getline` which does
not trigger the buffering issue.  There is one edge case that I decided
to leave up to the callers though: a file containing `\0` previously
resulted in line truncation, but is now included in the result.

Tested with Mac OS X 10.7 and 10.11:

    -DCMAKE_OSX_DEPLOYMENT_TARGET=10.7 -DCMAKE_CXX_FLAGS=-stdlib=libc++

and `./kwsysTestsCxx testSystemTools`.

Issue: cmake/cmake#15039
parent 5f757898
...@@ -4032,23 +4032,16 @@ std::string SystemTools::MakeCidentifier(const std::string& s) ...@@ -4032,23 +4032,16 @@ std::string SystemTools::MakeCidentifier(const std::string& s)
return str; return str;
} }
// Due to a buggy stream library on the HP and another on Mac OS X, we // Convenience function around std::getline which removes a trailing carriage
// need this very carefully written version of getline. Returns true // return and can truncate the buffer as needed. Returns true
// if any data were read before the end-of-file was reached. // if any data were read before the end-of-file was reached.
bool SystemTools::GetLineFromStream(std::istream& is, std::string& line, bool SystemTools::GetLineFromStream(std::istream& is, std::string& line,
bool* has_newline /* = 0 */, bool* has_newline /* = 0 */,
long sizeLimit /* = -1 */) long sizeLimit /* = -1 */)
{ {
const int bufferSize = 1024;
char buffer[bufferSize];
bool haveData = false;
bool haveNewline = false;
// Start with an empty line. // Start with an empty line.
line = ""; line = "";
long leftToRead = sizeLimit;
// Early short circuit return if stream is no good. Just return // Early short circuit return if stream is no good. Just return
// false and the empty line. (Probably means caller tried to // false and the empty line. (Probably means caller tried to
// create a file stream with a non-existent file name...) // create a file stream with a non-existent file name...)
...@@ -4060,44 +4053,23 @@ bool SystemTools::GetLineFromStream(std::istream& is, std::string& line, ...@@ -4060,44 +4053,23 @@ bool SystemTools::GetLineFromStream(std::istream& is, std::string& line,
return false; return false;
} }
// If no characters are read from the stream, the end of file has std::getline(is, line);
// been reached. Clear the fail bit just before reading. bool haveData = !line.empty() || !is.eof();
while (!haveNewline && leftToRead != 0 && if (!line.empty()) {
(static_cast<void>(is.clear(is.rdstate() & ~std::ios::failbit)),
static_cast<void>(is.getline(buffer, bufferSize)),
is.gcount() > 0)) {
// We have read at least one byte.
haveData = true;
// If newline character was read the gcount includes the character
// but the buffer does not: the end of line has been reached.
size_t length = strlen(buffer);
if (length < static_cast<size_t>(is.gcount())) {
haveNewline = true;
}
// Avoid storing a carriage return character. // Avoid storing a carriage return character.
if (length > 0 && buffer[length - 1] == '\r') { if (*line.rbegin() == '\r') {
buffer[length - 1] = 0; line.resize(line.size() - 1);
} }
// if we read too much then truncate the buffer // if we read too much then truncate the buffer
if (leftToRead > 0) { if (sizeLimit >= 0 && line.size() >= static_cast<size_t>(sizeLimit)) {
if (static_cast<long>(length) > leftToRead) { line.resize(sizeLimit);
buffer[leftToRead] = 0;
leftToRead = 0;
} else {
leftToRead -= static_cast<long>(length);
}
} }
// Append the data read to the line.
line.append(buffer);
} }
// Return the results. // Return the results.
if (has_newline) { if (has_newline) {
*has_newline = haveNewline; *has_newline = !is.eof();
} }
return haveData; return haveData;
} }
......
...@@ -523,9 +523,8 @@ public: ...@@ -523,9 +523,8 @@ public:
static bool GetShortPath(const std::string& path, std::string& result); static bool GetShortPath(const std::string& path, std::string& result);
/** /**
* Read line from file. Make sure to get everything. Due to a buggy stream * Read line from file. Make sure to read a full line and truncates it if
* library on the HP and another on Mac OS X, we need this very carefully * requested via sizeLimit. Returns true if any data were read before the
* written version of getline. Returns true if any data were read before the
* end-of-file was reached. If the has_newline argument is specified, it will * end-of-file was reached. If the has_newline argument is specified, it will
* be true when the line read had a newline character. * be true when the line read had a newline character.
*/ */
......
...@@ -912,6 +912,78 @@ static bool CheckGetLineFromStream() ...@@ -912,6 +912,78 @@ static bool CheckGetLineFromStream()
return ret; return ret;
} }
static bool CheckGetLineFromStreamLongLine()
{
const std::string fileWithLongLine("longlines.txt");
std::string firstLine, secondLine;
// First line: large buffer, containing a carriage return for some reason.
firstLine.assign(2050, ' ');
firstLine += "\rfirst";
secondLine.assign(2050, 'y');
secondLine += "second";
// Create file with long lines.
{
kwsys::ofstream out(fileWithLongLine.c_str(), std::ios::binary);
if (!out) {
std::cerr << "Problem opening for write: " << fileWithLongLine
<< std::endl;
return false;
}
out << firstLine << "\r\n\n" << secondLine << "\n";
}
kwsys::ifstream file(fileWithLongLine.c_str(), std::ios::binary);
if (!file) {
std::cerr << "Problem opening: " << fileWithLongLine << std::endl;
return false;
}
std::string line;
bool has_newline = false;
bool result;
// Read first line.
result = kwsys::SystemTools::GetLineFromStream(file, line, &has_newline, -1);
if (!result || line != firstLine) {
std::cerr << "First line does not match, expected " << firstLine.size()
<< " characters, got " << line.size() << std::endl;
return false;
}
if (!has_newline) {
std::cerr << "Expected new line to be read from first line" << std::endl;
return false;
}
// Read empty line.
has_newline = false;
result = kwsys::SystemTools::GetLineFromStream(file, line, &has_newline, -1);
if (!result || !line.empty()) {
std::cerr << "Expected successful read with an empty line, got "
<< line.size() << " characters" << std::endl;
return false;
}
if (!has_newline) {
std::cerr << "Expected new line to be read for an empty line" << std::endl;
return false;
}
// Read second line.
has_newline = false;
result = kwsys::SystemTools::GetLineFromStream(file, line, &has_newline, -1);
if (!result || line != secondLine) {
std::cerr << "Second line does not match, expected " << secondLine.size()
<< " characters, got " << line.size() << std::endl;
return false;
}
if (!has_newline) {
std::cerr << "Expected new line to be read from second line" << std::endl;
return false;
}
return true;
}
int testSystemTools(int, char* []) int testSystemTools(int, char* [])
{ {
bool res = true; bool res = true;
...@@ -951,6 +1023,8 @@ int testSystemTools(int, char* []) ...@@ -951,6 +1023,8 @@ int testSystemTools(int, char* [])
res &= CheckGetLineFromStream(); res &= CheckGetLineFromStream();
res &= CheckGetLineFromStreamLongLine();
res &= CheckGetFilenameName(); res &= CheckGetFilenameName();
return res ? 0 : 1; return res ? 0 : 1;
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment