From 4cc869a7c99227749ff98d4449420d393958c53a Mon Sep 17 00:00:00 2001 From: Maxim Kalaev Date: Mon, 1 Jul 2013 21:33:12 +0300 Subject: Adding error checking on fwrite/fflush in deps_log --- src/build.cc | 16 +++++++++++----- src/build.h | 5 ++++- src/deps_log.cc | 36 ++++++++++++++++++++++++------------ 3 files changed, 39 insertions(+), 18 deletions(-) diff --git a/src/build.cc b/src/build.cc index 5cf9d27..a2f430e 100644 --- a/src/build.cc +++ b/src/build.cc @@ -641,7 +641,10 @@ bool Builder::Build(string* err) { } --pending_commands; - FinishCommand(&result); + if (!FinishCommand(&result, err)) { + status_->BuildFinished(); + return false; + } if (!result.success()) { if (failures_allowed) @@ -704,7 +707,7 @@ bool Builder::StartEdge(Edge* edge, string* err) { return true; } -void Builder::FinishCommand(CommandRunner::Result* result) { +bool Builder::FinishCommand(CommandRunner::Result* result, string* err) { METRIC_RECORD("FinishCommand"); Edge* edge = result->edge; @@ -733,7 +736,7 @@ void Builder::FinishCommand(CommandRunner::Result* result) { // The rest of this function only applies to successful commands. if (!result->success()) - return; + return true; // Restat the edge outputs, if necessary. TimeStamp restat_mtime = 0; @@ -791,9 +794,12 @@ void Builder::FinishCommand(CommandRunner::Result* result) { assert(edge->outputs_.size() == 1 && "should have been rejected by parser"); Node* out = edge->outputs_[0]; TimeStamp deps_mtime = disk_interface_->Stat(out->path()); - scan_.deps_log()->RecordDeps(out, deps_mtime, deps_nodes); + if (!scan_.deps_log()->RecordDeps(out, deps_mtime, deps_nodes)) { + *err = string("Error writing to deps log: ") + strerror(errno); + return false; + } } - + return true; } bool Builder::ExtractDeps(CommandRunner::Result* result, diff --git a/src/build.h b/src/build.h index 2715c0c..33df7d0 100644 --- a/src/build.h +++ b/src/build.h @@ -163,7 +163,10 @@ struct Builder { bool Build(string* err); bool StartEdge(Edge* edge, string* err); - void FinishCommand(CommandRunner::Result* result); + + /// Update status ninja logs following a command termination. + /// @return false if the build can not proceed further due to a fatal error. + bool FinishCommand(CommandRunner::Result* result, string* err); /// Used for tests. void SetBuildLog(BuildLog* log) { diff --git a/src/deps_log.cc b/src/deps_log.cc index ce9bf06..b172d4b 100644 --- a/src/deps_log.cc +++ b/src/deps_log.cc @@ -70,8 +70,10 @@ bool DepsLog::OpenForWrite(const string& path, string* err) { return false; } } - fflush(file_); - + if (fflush(file_) != 0) { + *err = strerror(errno); + return false; + } return true; } @@ -88,12 +90,14 @@ bool DepsLog::RecordDeps(Node* node, TimeStamp mtime, // Assign ids to all nodes that are missing one. if (node->id() < 0) { - RecordId(node); + if (!RecordId(node)) + return false; made_change = true; } for (int i = 0; i < node_count; ++i) { if (nodes[i]->id() < 0) { - RecordId(nodes[i]); + if (!RecordId(nodes[i])) + return false; made_change = true; } } @@ -122,16 +126,21 @@ bool DepsLog::RecordDeps(Node* node, TimeStamp mtime, // Update on-disk representation. uint16_t size = 4 * (1 + 1 + (uint16_t)node_count); size |= 0x8000; // Deps record: set high bit. - fwrite(&size, 2, 1, file_); + if (fwrite(&size, 2, 1, file_) < 1) + return false; int id = node->id(); - fwrite(&id, 4, 1, file_); + if (fwrite(&id, 4, 1, file_) < 1) + return false; int timestamp = mtime; - fwrite(×tamp, 4, 1, file_); + if (fwrite(×tamp, 4, 1, file_) < 1) + return false; for (int i = 0; i < node_count; ++i) { id = nodes[i]->id(); - fwrite(&id, 4, 1, file_); + if (fwrite(&id, 4, 1, file_) < 1) + return false; } - fflush(file_); + if (fflush(file_) != 0) + return false; // Update in-memory representation. Deps* deps = new Deps(mtime, node_count); @@ -324,9 +333,12 @@ bool DepsLog::UpdateDeps(int out_id, Deps* deps) { bool DepsLog::RecordId(Node* node) { uint16_t size = (uint16_t)node->path().size(); - fwrite(&size, 2, 1, file_); - fwrite(node->path().data(), node->path().size(), 1, file_); - fflush(file_); + if (fwrite(&size, 2, 1, file_) < 1) + return false; + if (fwrite(node->path().data(), node->path().size(), 1, file_) < 1) + return false; // assuming node->path().size() > 0 + if (fflush(file_) != 0) + return false; node->set_id(nodes_.size()); nodes_.push_back(node); -- cgit v0.12 From 70f18e1138c9af638afae95b8a521c257e42ccce Mon Sep 17 00:00:00 2001 From: Maxim Kalaev Date: Tue, 2 Jul 2013 00:09:43 +0300 Subject: Adding checks for record overflow in deps_log --- src/deps_log.cc | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/deps_log.cc b/src/deps_log.cc index b172d4b..0b19e39 100644 --- a/src/deps_log.cc +++ b/src/deps_log.cc @@ -37,6 +37,9 @@ const int kCurrentVersion = 1; // buffer after every record to make sure records aren't written partially. const int kMaxBufferSize = 1 << 15; +// Record size is currently limited to 15 bit +const size_t kMaxRecordSize = (1 << 15) - 1; + DepsLog::~DepsLog() { Close(); } @@ -124,9 +127,14 @@ bool DepsLog::RecordDeps(Node* node, TimeStamp mtime, return true; // Update on-disk representation. - uint16_t size = 4 * (1 + 1 + (uint16_t)node_count); + size_t size = 4 * (1 + 1 + (uint16_t)node_count); + if (size > kMaxRecordSize) { + errno = ERANGE; + return false; + } size |= 0x8000; // Deps record: set high bit. - if (fwrite(&size, 2, 1, file_) < 1) + uint16_t size16 = (uint16_t)size; + if (fwrite(&size16, 2, 1, file_) < 1) return false; int id = node->id(); if (fwrite(&id, 4, 1, file_) < 1) @@ -332,11 +340,18 @@ bool DepsLog::UpdateDeps(int out_id, Deps* deps) { } bool DepsLog::RecordId(Node* node) { - uint16_t size = (uint16_t)node->path().size(); - if (fwrite(&size, 2, 1, file_) < 1) + size_t size = node->path().size(); + if (size > kMaxRecordSize) { + errno = ERANGE; return false; - if (fwrite(node->path().data(), node->path().size(), 1, file_) < 1) - return false; // assuming node->path().size() > 0 + } + uint16_t size16 = (uint16_t)size; + if (fwrite(&size16, 2, 1, file_) < 1) + return false; + if (fwrite(node->path().data(), node->path().size(), 1, file_) < 1) { + assert(node->path().size() > 0); + return false; + } if (fflush(file_) != 0) return false; -- cgit v0.12