Skip to content

Commit

Permalink
Merge pull request #2534 from digit-google/fix-build-log-leak
Browse files Browse the repository at this point in the history 
Fix build log leak
  • Loading branch information
@jhasse
jhasse authored Dec 10, 2024
2 parents 127f2d3 + 3ecea33 commit b78335e
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 67 deletions.
77 changes: 37 additions & 40 deletions src/build_log.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@
#define strtoll _strtoi64
#endif

using namespace std;

// Implementation details:
// Each run's log appends to the log file.
// To load, we run through all log entries in series, throwing away
Expand All @@ -63,24 +61,21 @@ uint64_t BuildLog::LogEntry::HashCommand(StringPiece command) {
return rapidhash(command.str_, command.len_);
}

BuildLog::LogEntry::LogEntry(const string& output)
: output(output) {}
BuildLog::LogEntry::LogEntry(std::string output) : output(std::move(output)) {}

BuildLog::LogEntry::LogEntry(const string& output, uint64_t command_hash,
int start_time, int end_time, TimeStamp mtime)
: output(output), command_hash(command_hash),
start_time(start_time), end_time(end_time), mtime(mtime)
{}
BuildLog::LogEntry::LogEntry(const std::string& output, uint64_t command_hash,
int start_time, int end_time, TimeStamp mtime)
: output(output), command_hash(command_hash), start_time(start_time),
end_time(end_time), mtime(mtime) {}

BuildLog::BuildLog()
: log_file_(NULL), needs_recompaction_(false) {}
BuildLog::BuildLog() = default;

BuildLog::~BuildLog() {
Close();
}

bool BuildLog::OpenForWrite(const string& path, const BuildLogUser& user,
string* err) {
bool BuildLog::OpenForWrite(const std::string& path, const BuildLogUser& user,
std::string* err) {
if (needs_recompaction_) {
if (!Recompact(path, user, err))
return false;
Expand All @@ -94,18 +89,19 @@ bool BuildLog::OpenForWrite(const string& path, const BuildLogUser& user,

bool BuildLog::RecordCommand(Edge* edge, int start_time, int end_time,
TimeStamp mtime) {
string command = edge->EvaluateCommand(true);
std::string command = edge->EvaluateCommand(true);
uint64_t command_hash = LogEntry::HashCommand(command);
for (vector<Node*>::iterator out = edge->outputs_.begin();
for (std::vector<Node*>::iterator out = edge->outputs_.begin();
out != edge->outputs_.end(); ++out) {
const string& path = (*out)->path();
const std::string& path = (*out)->path();
Entries::iterator i = entries_.find(path);
LogEntry* log_entry;
if (i != entries_.end()) {
log_entry = i->second;
log_entry = i->second.get();
} else {
log_entry = new LogEntry(path);
entries_.insert(Entries::value_type(log_entry->output, log_entry));
// Passes ownership of |log_entry| to the map, but keeps the pointer valid.
entries_.emplace(log_entry->output, std::unique_ptr<LogEntry>(log_entry));
}
log_entry->command_hash = command_hash;
log_entry->start_time = start_time;
Expand Down Expand Up @@ -209,7 +205,7 @@ struct LineReader {
char* line_end_;
};

LoadStatus BuildLog::Load(const string& path, string* err) {
LoadStatus BuildLog::Load(const std::string& path, std::string* err) {
METRIC_RECORD(".ninja_log load");
FILE* file = fopen(path.c_str(), "r");
if (!file) {
Expand Down Expand Up @@ -283,18 +279,19 @@ LoadStatus BuildLog::Load(const string& path, string* err) {
end = static_cast<char*>(memchr(start, kFieldSeparator, line_end - start));
if (!end)
continue;
string output = string(start, end - start);
std::string output(start, end - start);

start = end + 1;
end = line_end;

LogEntry* entry;
Entries::iterator i = entries_.find(output);
if (i != entries_.end()) {
entry = i->second;
entry = i->second.get();
} else {
entry = new LogEntry(output);
entries_.insert(Entries::value_type(entry->output, entry));
entry = new LogEntry(std::move(output));
// Passes ownership of |entry| to the map, but keeps the pointer valid.
entries_.emplace(entry->output, std::unique_ptr<LogEntry>(entry));
++unique_entry_count;
}
++total_entry_count;
Expand Down Expand Up @@ -327,10 +324,10 @@ LoadStatus BuildLog::Load(const string& path, string* err) {
return LOAD_SUCCESS;
}

BuildLog::LogEntry* BuildLog::LookupByOutput(const string& path) {
BuildLog::LogEntry* BuildLog::LookupByOutput(const std::string& path) {
Entries::iterator i = entries_.find(path);
if (i != entries_.end())
return i->second;
return i->second.get();
return NULL;
}

Expand All @@ -340,12 +337,12 @@ bool BuildLog::WriteEntry(FILE* f, const LogEntry& entry) {
entry.output.c_str(), entry.command_hash) > 0;
}

bool BuildLog::Recompact(const string& path, const BuildLogUser& user,
string* err) {
bool BuildLog::Recompact(const std::string& path, const BuildLogUser& user,
std::string* err) {
METRIC_RECORD(".ninja_log recompact");

Close();
string temp_path = path + ".recompact";
std::string temp_path = path + ".recompact";
FILE* f = fopen(temp_path.c_str(), "wb");
if (!f) {
*err = strerror(errno);
Expand All @@ -358,22 +355,22 @@ bool BuildLog::Recompact(const string& path, const BuildLogUser& user,
return false;
}

vector<StringPiece> dead_outputs;
for (Entries::iterator i = entries_.begin(); i != entries_.end(); ++i) {
if (user.IsPathDead(i->first)) {
dead_outputs.push_back(i->first);
std::vector<StringPiece> dead_outputs;
for (const auto& pair : entries_) {
if (user.IsPathDead(pair.first)) {
dead_outputs.push_back(pair.first);
continue;
}

if (!WriteEntry(f, *i->second)) {
if (!WriteEntry(f, *pair.second)) {
*err = strerror(errno);
fclose(f);
return false;
}
}

for (size_t i = 0; i < dead_outputs.size(); ++i)
entries_.erase(dead_outputs[i]);
for (StringPiece output : dead_outputs)
entries_.erase(output);

fclose(f);
if (unlink(path.c_str()) < 0) {
Expand Down Expand Up @@ -408,24 +405,24 @@ bool BuildLog::Restat(const StringPiece path,
fclose(f);
return false;
}
for (Entries::iterator i = entries_.begin(); i != entries_.end(); ++i) {
for (auto& pair : entries_) {
bool skip = output_count > 0;
for (int j = 0; j < output_count; ++j) {
if (i->second->output == outputs[j]) {
if (pair.second->output == outputs[j]) {
skip = false;
break;
}
}
if (!skip) {
const TimeStamp mtime = disk_interface.Stat(i->second->output, err);
const TimeStamp mtime = disk_interface.Stat(pair.second->output, err);
if (mtime == -1) {
fclose(f);
return false;
}
i->second->mtime = mtime;
pair.second->mtime = mtime;
}

if (!WriteEntry(f, *i->second)) {
if (!WriteEntry(f, *pair.second)) {
*err = strerror(errno);
fclose(f);
return false;
Expand Down
20 changes: 11 additions & 9 deletions src/build_log.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@
#ifndef NINJA_BUILD_LOG_H_
#define NINJA_BUILD_LOG_H_

#include <string>
#include <stdio.h>

#include <memory>
#include <string>

#include "hash_map.h"
#include "load_status.h"
#include "timestamp.h"
Expand Down Expand Up @@ -57,10 +59,10 @@ struct BuildLog {

struct LogEntry {
std::string output;
uint64_t command_hash;
int start_time;
int end_time;
TimeStamp mtime;
uint64_t command_hash = 0;
int start_time = 0;
int end_time = 0;
TimeStamp mtime = 0;

static uint64_t HashCommand(StringPiece command);

Expand All @@ -71,7 +73,7 @@ struct BuildLog {
mtime == o.mtime;
}

explicit LogEntry(const std::string& output);
explicit LogEntry(std::string output);
LogEntry(const std::string& output, uint64_t command_hash,
int start_time, int end_time, TimeStamp mtime);
};
Expand All @@ -90,7 +92,7 @@ struct BuildLog {
bool Restat(StringPiece path, const DiskInterface& disk_interface,
int output_count, char** outputs, std::string* err);

typedef ExternalStringHashMap<LogEntry*>::Type Entries;
typedef ExternalStringHashMap<std::unique_ptr<LogEntry>>::Type Entries;
const Entries& entries() const { return entries_; }

private:
Expand All @@ -99,9 +101,9 @@ struct BuildLog {
bool OpenForWriteIfNeeded();

Entries entries_;
FILE* log_file_;
FILE* log_file_ = nullptr;
std::string log_file_path_;
bool needs_recompaction_;
bool needs_recompaction_ = false;
};

#endif // NINJA_BUILD_LOG_H_
35 changes: 17 additions & 18 deletions src/build_log_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@
#endif
#include <cassert>

using namespace std;

namespace {

const char kTestFilename[] = "BuildLogTest-tempfile";
Expand All @@ -50,7 +48,7 @@ TEST_F(BuildLogTest, WriteRead) {
"build mid: cat in\n");

BuildLog log1;
string err;
std::string err;
EXPECT_TRUE(log1.OpenForWrite(kTestFilename, *this, &err));
ASSERT_EQ("", err);
log1.RecordCommand(state_.edges_[0], 15, 18);
Expand All @@ -77,7 +75,7 @@ TEST_F(BuildLogTest, FirstWriteAddsSignature) {
const size_t kVersionPos = strlen(kExpectedVersion) - 2; // Points at 'X'.

BuildLog log;
string contents, err;
std::string contents, err;

EXPECT_TRUE(log.OpenForWrite(kTestFilename, *this, &err));
ASSERT_EQ("", err);
Expand Down Expand Up @@ -111,7 +109,7 @@ TEST_F(BuildLogTest, DoubleEntry) {
BuildLog::LogEntry::HashCommand("command def"));
fclose(f);

string err;
std::string err;
BuildLog log;
EXPECT_TRUE(log.Load(kTestFilename, &err));
ASSERT_EQ("", err);
Expand All @@ -128,7 +126,7 @@ TEST_F(BuildLogTest, Truncate) {

{
BuildLog log1;
string err;
std::string err;
EXPECT_TRUE(log1.OpenForWrite(kTestFilename, *this, &err));
ASSERT_EQ("", err);
log1.RecordCommand(state_.edges_[0], 15, 18);
Expand All @@ -148,7 +146,7 @@ TEST_F(BuildLogTest, Truncate) {
// crash when parsing.
for (off_t size = statbuf.st_size; size > 0; --size) {
BuildLog log2;
string err;
std::string err;
EXPECT_TRUE(log2.OpenForWrite(kTestFilename, *this, &err));
ASSERT_EQ("", err);
log2.RecordCommand(state_.edges_[0], 15, 18);
Expand All @@ -169,10 +167,10 @@ TEST_F(BuildLogTest, ObsoleteOldVersion) {
fprintf(f, "123 456 0 out command\n");
fclose(f);

string err;
std::string err;
BuildLog log;
EXPECT_TRUE(log.Load(kTestFilename, &err));
ASSERT_NE(err.find("version"), string::npos);
ASSERT_NE(err.find("version"), std::string::npos);
}

TEST_F(BuildLogTest, SpacesInOutput) {
Expand All @@ -182,7 +180,7 @@ TEST_F(BuildLogTest, SpacesInOutput) {
BuildLog::LogEntry::HashCommand("command"));
fclose(f);

string err;
std::string err;
BuildLog log;
EXPECT_TRUE(log.Load(kTestFilename, &err));
ASSERT_EQ("", err);
Expand All @@ -208,7 +206,7 @@ TEST_F(BuildLogTest, DuplicateVersionHeader) {
BuildLog::LogEntry::HashCommand("command2"));
fclose(f);

string err;
std::string err;
BuildLog log;
EXPECT_TRUE(log.Load(kTestFilename, &err));
ASSERT_EQ("", err);
Expand All @@ -229,22 +227,23 @@ TEST_F(BuildLogTest, DuplicateVersionHeader) {
}

struct TestDiskInterface : public DiskInterface {
virtual TimeStamp Stat(const string& path, string* err) const {
virtual TimeStamp Stat(const std::string& path, std::string* err) const {
return 4;
}
virtual bool WriteFile(const string& path, const string& contents) {
virtual bool WriteFile(const std::string& path, const std::string& contents) {
assert(false);
return true;
}
virtual bool MakeDir(const string& path) {
virtual bool MakeDir(const std::string& path) {
assert(false);
return false;
}
virtual Status ReadFile(const string& path, string* contents, string* err) {
virtual Status ReadFile(const std::string& path, std::string* contents,
std::string* err) {
assert(false);
return NotFound;
}
virtual int RemoveFile(const string& path) {
virtual int RemoveFile(const std::string& path) {
assert(false);
return 0;
}
Expand Down Expand Up @@ -289,7 +288,7 @@ TEST_F(BuildLogTest, VeryLongInputLine) {
BuildLog::LogEntry::HashCommand("command2"));
fclose(f);

string err;
std::string err;
BuildLog log;
EXPECT_TRUE(log.Load(kTestFilename, &err));
ASSERT_EQ("", err);
Expand Down Expand Up @@ -335,7 +334,7 @@ TEST_F(BuildLogRecompactTest, Recompact) {
"build out2: cat in\n");

BuildLog log1;
string err;
std::string err;
EXPECT_TRUE(log1.OpenForWrite(kTestFilename, *this, &err));
ASSERT_EQ("", err);
// Record the same edge several times, to trigger recompaction
Expand Down

0 comments on commit b78335e

Please sign in to comment.