Fix some shadow variables, including one in file_id.cc that causes all files to generate the same hash. Add a test to make sure this doesn't happen again.

Review URL: http://breakpad.appspot.com/316002

git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@875 4c0a9323-5329-0410-9bdc-e9ce6186880e

6 files changed, 127 insertions, 30 deletions

diff --git a/src/client/linux/minidump_writer/minidump_writer.cc b/src/client/linux/minidump_writer/minidump_writer.cc
index 77ff7ea2..b860e430 100644
--- a/src/client/linux/minidump_writer/minidump_writer.cc
+++ b/src/client/linux/minidump_writer/minidump_writer.cc
@@ -671,8 +671,8 @@ class MinidumpWriter {
         // don't bother trying to write it.
         bool ip_is_mapped = false;
         MDMemoryDescriptor ip_memory_d;
-        for (unsigned i = 0; i < dumper_.mappings().size(); ++i) {
-          const MappingInfo& mapping = *dumper_.mappings()[i];
+        for (unsigned j = 0; j < dumper_.mappings().size(); ++j) {
+          const MappingInfo& mapping = *dumper_.mappings()[j];
           if (ip >= mapping.start_addr &&
               ip < mapping.start_addr + mapping.size) {
             ip_is_mapped = true;
diff --git a/src/common/linux/file_id.cc b/src/common/linux/file_id.cc
index acd448a9..13f4f3ad 100644
--- a/src/common/linux/file_id.cc
+++ b/src/common/linux/file_id.cc
@@ -107,10 +107,10 @@ static void FindElfClassSection(const char *elf_base,
   const Shdr* section = NULL;
   for (int i = 0; i < elf_header->e_shnum; ++i) {
     if (sections[i].sh_type == section_type) {
-      const char* section_name = (char*)(elf_base +
-                                         string_section->sh_offset +
-                                         sections[i].sh_name);
-      if (!my_strncmp(section_name, section_name, name_len)) {
+      const char* current_section_name = (char*)(elf_base +
+                                                 string_section->sh_offset +
+                                                 sections[i].sh_name);
+      if (!my_strncmp(current_section_name, section_name, name_len)) {
         section = &sections[i];
         break;
       }
@@ -166,8 +166,7 @@ static bool FindElfSection(const void *elf_mapped_base,
 
 template<typename ElfClass>
 static bool ElfClassBuildIDNoteIdentifier(const void *section,
-                                          uint8_t identifier[kMDGUIDSize])
-{
+                                          uint8_t identifier[kMDGUIDSize]) {
   typedef typename ElfClass::Nhdr Nhdr;
 
   const Nhdr* note_header = reinterpret_cast<const Nhdr*>(section);
@@ -190,8 +189,7 @@ static bool ElfClassBuildIDNoteIdentifier(const void *section,
 // Attempt to locate a .note.gnu.build-id section in an ELF binary
 // and copy as many bytes of it as will fit into |identifier|.
 static bool FindElfBuildIDNote(const void *elf_mapped_base,
-                               uint8_t identifier[kMDGUIDSize])
-{
+                               uint8_t identifier[kMDGUIDSize]) {
   void* note_section;
   int note_size, elfclass;
   if (!FindElfSection(elf_mapped_base, ".note.gnu.build-id", SHT_NOTE,
@@ -213,7 +211,6 @@ static bool FindElfBuildIDNote(const void *elf_mapped_base,
 // a simple hash by XORing the first page worth of bytes into |identifier|.
 static bool HashElfTextSection(const void *elf_mapped_base,
                                uint8_t identifier[kMDGUIDSize]) {
-
   void* text_section;
   int text_size;
   if (!FindElfSection(elf_mapped_base, ".text", SHT_PROGBITS,
@@ -235,8 +232,7 @@ static bool HashElfTextSection(const void *elf_mapped_base,
 
 // static
 bool FileID::ElfFileIdentifierFromMappedFile(void* base,
-                                             uint8_t identifier[kMDGUIDSize])
-{
+                                             uint8_t identifier[kMDGUIDSize]) {
   // Look for a build id note first.
   if (FindElfBuildIDNote(base, identifier))
     return true;
diff --git a/src/common/linux/file_id_unittest.cc b/src/common/linux/file_id_unittest.cc
index 24208983..7479a966 100644
--- a/src/common/linux/file_id_unittest.cc
+++ b/src/common/linux/file_id_unittest.cc
@@ -43,6 +43,18 @@ using google_breakpad::synth_elf::ELF;
 using google_breakpad::test_assembler::kLittleEndian;
 using google_breakpad::test_assembler::Section;
 
+namespace {
+
+// Simply calling Section::Append(size, byte) produces a uninteresting pattern
+// that tends to get hashed to 0000...0000. This populates the section with
+// data to produce better hashes.
+void PopulateSection(Section* section, int size, int prime_number) {
+  for (int i = 0; i < size; i++)
+    section->Append(1, (i % prime_number) % 256);
+}
+
+}  // namespace
+
 TEST(FileIDStripTest, StripSelf) {
   // Calculate the File ID of this binary using
   // FileID::ElfFileIdentifier, then make a copy of this binary,
@@ -181,3 +193,92 @@ TEST_F(FileIDTest, BuildID) {
                                     sizeof(identifier_string));
   EXPECT_STREQ(expected_identifier_string, identifier_string);
 }
+
+// Test to make sure two files with different text sections produce
+// different hashes when not using a build id.
+TEST_F(FileIDTest, UniqueHashes32) {
+  char identifier_string_1[] =
+    "00000000-0000-0000-0000-000000000000";
+  char identifier_string_2[] =
+    "00000000-0000-0000-0000-000000000000";
+  uint8_t identifier_1[sizeof(MDGUID)];
+  uint8_t identifier_2[sizeof(MDGUID)];
+
+  {
+    ELF elf1(EM_386, ELFCLASS32, kLittleEndian);
+    Section foo_1(kLittleEndian);
+    PopulateSection(&foo_1, 32, 5);
+    elf1.AddSection(".foo", foo_1, SHT_PROGBITS);
+    Section text_1(kLittleEndian);
+    PopulateSection(&text_1, 4096, 17);
+    elf1.AddSection(".text", text_1, SHT_PROGBITS);
+    elf1.Finish();
+    GetElfContents(elf1);
+  }
+
+  EXPECT_TRUE(FileID::ElfFileIdentifierFromMappedFile(elfdata, identifier_1));
+  FileID::ConvertIdentifierToString(identifier_1, identifier_string_1,
+                                    sizeof(identifier_string_1));
+
+  {
+    ELF elf2(EM_386, ELFCLASS32, kLittleEndian);
+    Section text_2(kLittleEndian);
+    Section foo_2(kLittleEndian);
+    PopulateSection(&foo_2, 32, 5);
+    elf2.AddSection(".foo", foo_2, SHT_PROGBITS);
+    PopulateSection(&text_2, 4096, 31);
+    elf2.AddSection(".text", text_2, SHT_PROGBITS);
+    elf2.Finish();
+    GetElfContents(elf2);
+  }
+
+  EXPECT_TRUE(FileID::ElfFileIdentifierFromMappedFile(elfdata, identifier_2));
+  FileID::ConvertIdentifierToString(identifier_2, identifier_string_2,
+                                    sizeof(identifier_string_2));
+
+  EXPECT_STRNE(identifier_string_1, identifier_string_2);
+}
+
+// Same as UniqueHashes32, for x86-64.
+TEST_F(FileIDTest, UniqueHashes64) {
+  char identifier_string_1[] =
+    "00000000-0000-0000-0000-000000000000";
+  char identifier_string_2[] =
+    "00000000-0000-0000-0000-000000000000";
+  uint8_t identifier_1[sizeof(MDGUID)];
+  uint8_t identifier_2[sizeof(MDGUID)];
+
+  {
+    ELF elf1(EM_X86_64, ELFCLASS64, kLittleEndian);
+    Section foo_1(kLittleEndian);
+    PopulateSection(&foo_1, 32, 5);
+    elf1.AddSection(".foo", foo_1, SHT_PROGBITS);
+    Section text_1(kLittleEndian);
+    PopulateSection(&text_1, 4096, 17);
+    elf1.AddSection(".text", text_1, SHT_PROGBITS);
+    elf1.Finish();
+    GetElfContents(elf1);
+  }
+
+  EXPECT_TRUE(FileID::ElfFileIdentifierFromMappedFile(elfdata, identifier_1));
+  FileID::ConvertIdentifierToString(identifier_1, identifier_string_1,
+                                    sizeof(identifier_string_1));
+
+  {
+    ELF elf2(EM_X86_64, ELFCLASS64, kLittleEndian);
+    Section text_2(kLittleEndian);
+    Section foo_2(kLittleEndian);
+    PopulateSection(&foo_2, 32, 5);
+    elf2.AddSection(".foo", foo_2, SHT_PROGBITS);
+    PopulateSection(&text_2, 4096, 31);
+    elf2.AddSection(".text", text_2, SHT_PROGBITS);
+    elf2.Finish();
+    GetElfContents(elf2);
+  }
+
+  EXPECT_TRUE(FileID::ElfFileIdentifierFromMappedFile(elfdata, identifier_2));
+  FileID::ConvertIdentifierToString(identifier_2, identifier_string_2,
+                                    sizeof(identifier_string_2));
+
+  EXPECT_STRNE(identifier_string_1, identifier_string_2);
+}
diff --git a/src/common/stabs_reader.cc b/src/common/stabs_reader.cc
index 7dd2eecd..4897361e 100644
--- a/src/common/stabs_reader.cc
+++ b/src/common/stabs_reader.cc
@@ -130,7 +130,7 @@ bool StabsReader::ProcessCompilationUnit() {
   // There may be an N_SO entry whose name ends with a slash,
   // indicating the directory in which the compilation occurred.
   // The build directory defaults to NULL.
-  const char *build_directory = NULL;  
+  const char *build_directory = NULL;
   {
     const char *name = SymbolString();
     if (name[0] && name[strlen(name) - 1] == '/') {
@@ -138,7 +138,7 @@ bool StabsReader::ProcessCompilationUnit() {
       ++iterator_;
     }
   }
-      
+
   // We expect to see an N_SO entry with a filename next, indicating
   // the start of the compilation unit.
   {
@@ -212,7 +212,7 @@ bool StabsReader::ProcessCompilationUnit() {
   queued_lines_.clear();
 
   return true;
-}          
+}
 
 bool StabsReader::ProcessFunction() {
   assert(!iterator_->at_end && iterator_->type == N_FUN);
@@ -237,7 +237,7 @@ bool StabsReader::ProcessFunction() {
       return false;
   }
   queued_lines_.clear();
-  
+
   while (!iterator_->at_end) {
     if (iterator_->type == N_SO || iterator_->type == N_FUN)
       break;
@@ -266,8 +266,8 @@ bool StabsReader::ProcessFunction() {
   if (!iterator_->at_end) {
     assert(iterator_->type == N_SO || iterator_->type == N_FUN);
     if (iterator_->type == N_FUN) {
-      const char *name = SymbolString();
-      if (name[0] == '\0') {
+      const char *symbol_name = SymbolString();
+      if (symbol_name[0] == '\0') {
         // An N_FUN entry with no name is a terminator for this function;
         // its value is the function's size.
         ending_address = function_address + iterator_->value;
diff --git a/src/processor/source_line_resolver_base.cc b/src/processor/source_line_resolver_base.cc
index b8362dcf..49088c89 100644
--- a/src/processor/source_line_resolver_base.cc
+++ b/src/processor/source_line_resolver_base.cc
@@ -88,7 +88,7 @@ bool SourceLineResolverBase::ReadSymbolFile(char **symbol_data,
   int error_code = stat(map_file.c_str(), &buf);
   if (error_code == -1) {
     string error_string;
-    int error_code = ErrnoString(&error_string);
+    error_code = ErrnoString(&error_string);
     BPLOG(ERROR) << "Could not open " << map_file <<
         ", error " << error_code << ": " << error_string;
     return false;
@@ -110,7 +110,7 @@ bool SourceLineResolverBase::ReadSymbolFile(char **symbol_data,
   FILE *f = fopen(map_file.c_str(), "rt");
   if (!f) {
     string error_string;
-    int error_code = ErrnoString(&error_string);
+    error_code = ErrnoString(&error_string);
     BPLOG(ERROR) << "Could not open " << map_file <<
         ", error " << error_code << ": " << error_string;
     delete [] (*symbol_data);
@@ -126,7 +126,7 @@ bool SourceLineResolverBase::ReadSymbolFile(char **symbol_data,
 
   if (items_read != file_size) {
     string error_string;
-    int error_code = ErrnoString(&error_string);
+    error_code = ErrnoString(&error_string);
     BPLOG(ERROR) << "Could not slurp " << map_file <<
         ", error " << error_code << ": " << error_string;
     delete [] (*symbol_data);
@@ -240,11 +240,11 @@ void SourceLineResolverBase::UnloadModule(const CodeModule *code_module) {
   if (!code_module)
     return;
 
-  ModuleMap::iterator iter = modules_->find(code_module->code_file());
-  if (iter != modules_->end()) {
-    Module *symbol_module = iter->second;
+  ModuleMap::iterator mod_iter = modules_->find(code_module->code_file());
+  if (mod_iter != modules_->end()) {
+    Module *symbol_module = mod_iter->second;
     delete symbol_module;
-    modules_->erase(iter);
+    modules_->erase(mod_iter);
   }
 
   if (ShouldDeleteMemoryBufferAfterLoadModule()) {
diff --git a/src/tools/linux/md2core/minidump-2-core.cc b/src/tools/linux/md2core/minidump-2-core.cc
index f08da768..efec22e4 100644
--- a/src/tools/linux/md2core/minidump-2-core.cc
+++ b/src/tools/linux/md2core/minidump-2-core.cc
@@ -442,7 +442,7 @@ ParseSystemInfo(CrashedProcess* crashinfo, MMappedRange range,
 #else
 #error "This code has not been ported to your platform yet"
 #endif
-  if (!strstr(full_file.GetString(sysinfo->csd_version_rva).c_str(), "Linux")){
+  if (!strstr(full_file.GetString(sysinfo->csd_version_rva).c_str(), "Linux")) {
     fprintf(stderr, "This minidump was not generated by Linux.\n");
     _exit(1);
   }
@@ -654,9 +654,9 @@ ParseCmdLine(CrashedProcess* crashinfo, MMappedRange range) {
 
       len = range.length() > args_len ? args_len : range.length();
       memcpy(crashinfo->prps.pr_psargs, cmdline, len);
-      for (unsigned i = 0; i < len; ++i) {
-        if (crashinfo->prps.pr_psargs[i] == 0)
-          crashinfo->prps.pr_psargs[i] = ' ';
+      for (unsigned j = 0; j < len; ++j) {
+        if (crashinfo->prps.pr_psargs[j] == 0)
+          crashinfo->prps.pr_psargs[j] = ' ';
       }
       break;
     }