adds the security cookie check hook transform.

syzygy/instrument/transforms/security_cookie_check_hook_transform.cc

+// Copyright 2017 Google Inc. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+
+#include "syzygy/instrument/transforms/security_cookie_check_hook_transform.h"
+
+#include "syzygy/pe/pe_utils.h"
+
+namespace instrument {
+namespace transforms {
+
+using block_graph::Displacement;
+using block_graph::Operand;
+
+const char SecurityCookieCheckHookTransform::kTransformName[] =
+    "SecurityCookieCheckHookTransform";
+
+bool SecurityCookieCheckHookTransform::TransformBlockGraph(
+  const TransformPolicyInterface* policy,
+  BlockGraph* block_graph,
+  BlockGraph::Block* header_block

[chrisha, 2017/05/09 19:18:39]

Indent parameters another +2 (4 from start of line)

+) {

[chrisha, 2017/05/09 19:18:39]

Move to the previous line.

+  BlockGraph::Block *__report_gsfailure = nullptr;

[chrisha, 2017/05/09 19:18:40]

Move * to left (BlockGraph::Block* __report_gsfailure)

Also, double underscore symbols are typically reserved names. May as well just
call this 'report_gsfailure'.

+  const BlockGraph::BlockMap &blocks = block_graph->blocks();

[chrisha, 2017/05/09 19:18:39]

Move & to left.

+  for (const auto &block : blocks) {

[chrisha, 2017/05/09 19:18:39]

Move & to left (auto& block)

+    std::string name(block.second.name());
+    if (name == "__report_gsfailure") {

[chrisha, 2017/05/09 19:18:39]

Make an anonymous namespaced static global at the top of this file with this
string value rather than repeating it everywhere:

constexpr char kReportGsFailure[] = "__report_gsfailure";

+      __report_gsfailure = block_graph->GetBlockById(block.first);

[chrisha, 2017/05/09 19:18:39]

You've already got the block, no need to go through the block_graph to ask for
it again.

report_gsfailure = &block.second;

+      break;
+    }
+  }
+
+  if (__report_gsfailure == nullptr) {
+    LOG(ERROR) << "Could not find __report_gsfailure.";
+    return false;
+  }
+
+  LOG(INFO) << "Found a __report_gsfailure implementation, hooking it now..";

[chrisha, 2017/05/09 19:18:40]

Remove extra period.

+  BlockGraph::Section* section_text = block_graph->FindOrAddSection(
+    pe::kCodeSectionName,
+    pe::kCodeCharacteristics

[chrisha, 2017/05/09 19:18:39]

Both of these parameters fit on one line, indented +4 from the previous line (so
+6 overall).

+  );

[chrisha, 2017/05/09 19:18:39]

Move to end of previous line.

+
+  // All of the below is needed to build the instrumentation via the assembler

[chrisha, 2017/05/09 19:18:39]

Missing period on comment.

+  BasicBlockSubGraph bbsg;
+  BasicBlockSubGraph::BlockDescription* block_desc = bbsg.AddBlockDescription(
+    "__my_report_gsfailure",
+    nullptr,
+    BlockGraph::CODE_BLOCK,
+    section_text->id(),
+    1,
+    0

[chrisha, 2017/05/09 19:18:39]

These likely all fit on one line.

+  );

[chrisha, 2017/05/09 19:18:39]

Move to end of previous line.

+
+  BasicCodeBlock* bb = bbsg.AddBasicCodeBlock("__my_report_gsfailure");

[chrisha, 2017/05/09 19:18:39]

Prefix with __syzygy_ maybe? Slightly more consistent with other transforms.

+  block_desc->basic_block_order.push_back(bb);
+  BasicBlockAssembler assm(bb->instructions().begin(), &bb->instructions());
+  assm.mov(
+    Operand(Displacement(0xdeadbeef)),
+    assm::eax

[chrisha, 2017/05/09 19:18:39]

Indent another 2 spaces.

+  );

[chrisha, 2017/05/09 19:18:39]

Move to previous line.

+
+  // Condense into a block

[chrisha, 2017/05/09 19:18:39]

Missing period on comment.

+  BlockBuilder block_builder(block_graph);
+  if (!block_builder.Merge(&bbsg)) {
+    LOG(ERROR) << "Failed to build __my_report_gsfailure block.";

[chrisha, 2017/05/09 19:18:39]

Create another string constant with this name and use it here as well?

+    return false;
+  }
+
+  // Exactly one new block should have been created

[chrisha, 2017/05/09 19:18:39]

This should be a DCHECK (kind of like an assertion) rather than a runtime
validation. No need for the comment as its self-documenting:

DCHECK_EQ(1u, block_builder.new_blocks().size());

+  if (block_builder.new_blocks().size() != 1) {
+    LOG(ERROR) << "Only one block should have been built.";
+    return false;
+  }
+
+  if (__report_gsfailure->references().size() != 1) {

[chrisha, 2017/05/09 19:18:39]

Ditto with this as a DCHECK assertion.

+    VLOG(1) << "Only a single reference is expected.";
+  }
+
+  // Transfer the referrers to the new block, and delete the old one
+  BlockGraph::Block* __my_report_gsfailure = block_builder.new_blocks().front();
+  __report_gsfailure->TransferReferrers(
+    0,
+    __my_report_gsfailure,
+    BlockGraph::Block::kTransferInternalReferences

[chrisha, 2017/05/09 19:18:39]

Indent all of these another 2 spaces, try to fit as many parameters on a line as
possible.

+  );

[chrisha, 2017/05/09 19:18:39]

Move to end of previous line.

+
+  __report_gsfailure->RemoveAllReferences();
+  if (!block_graph->RemoveBlock(__report_gsfailure)) {
+    LOG(ERROR) << "Removing __report_gsfailure failed.";
+    return false;
+  }
+
+  return true;
+}
+
+} // namespace transforms
+} // namespace instrument