Skip to content

Commit

Permalink
build_log.cc: Remove memory leak in Recompact() method.
Browse files Browse the repository at this point in the history
The erase() call to remove `dead_outputs` was leaking the
LogEntry value that the map pointed to.

This patch changes the type of `Entries` to store
`std::unique_ptr<LogEntry>` values, instead of raw pointers
to get rid of the problem, and adjust call sites accordingly.

Also use Entries::emplace() instead of Entries::insert() to
create the new value in-place when inserting new ones into
the map.
  • Loading branch information
digit-google committed Nov 25, 2024
1 parent 1fdfe32 commit e7ebebc
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 18 deletions.
34 changes: 18 additions & 16 deletions src/build_log.cc
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,11 @@ bool BuildLog::RecordCommand(Edge* edge, int start_time, int end_time,
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 @@ -286,10 +287,11 @@ LoadStatus BuildLog::Load(const std::string& path, std::string* err) {
LogEntry* entry;
Entries::iterator i = entries_.find(output);
if (i != entries_.end()) {
entry = i->second;
entry = i->second.get();
} else {
entry = new LogEntry(std::move(output));
entries_.insert(Entries::value_type(entry->output, entry));
// 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 @@ -325,7 +327,7 @@ LoadStatus BuildLog::Load(const std::string& path, std::string* err) {
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 Down Expand Up @@ -354,21 +356,21 @@ bool BuildLog::Recompact(const std::string& path, const BuildLogUser& user,
}

std::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);
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 @@ -403,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
6 changes: 4 additions & 2 deletions src/build_log.h
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 @@ -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 Down

0 comments on commit e7ebebc

Please sign in to comment.