diff --git a/.github/workflows/build-test-ci.yml b/.github/workflows/build-test-ci.yml deleted file mode 100644 index 2196d55f..00000000 --- a/.github/workflows/build-test-ci.yml +++ /dev/null @@ -1,73 +0,0 @@ -# GitHub actions workflow. -# https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions - -name: Build+Test CI - -on: - push: - branches: [main] - - schedule: - # The GH mirroring from Google GoB does not trigger push actions. - # Fire it every other day to provide some coverage. This will run ~8 AM PT. - - cron: '39 3 */2 * *' - - # Allow for manual triggers from the web. - workflow_dispatch: - -jobs: - autotools: - strategy: - fail-fast: false - matrix: - include: - - os: ubuntu-latest - cc: gcc - cxx: g++ - - os: ubuntu-latest - cc: clang - cxx: clang++ - - os: macos-latest - cc: clang - cxx: clang++ - runs-on: ${{ matrix.os }} - env: - CC: ${{ matrix.cc }} - CXX: ${{ matrix.cxx }} - - steps: - - name: System settings - run: | - set -x - $CC --version - $CXX --version - getconf _NPROCESSORS_ONLN - getconf _NPROCESSORS_CONF - true - - - name: Checkout depot_tools - run: git clone --depth=1 https://chromium.googlesource.com/chromium/tools/depot_tools.git ../depot_tools - - - name: Checkout breakpad - run: | - set -xe - PATH+=:$PWD/../depot_tools - gclient config --unmanaged --name=src https://github.com/${{ github.repository }} - gclient sync --no-history --nohooks - - # First build & test in-tree. - - run: ./configure --disable-silent-rules - working-directory: src - - run: make -j$(getconf _NPROCESSORS_CONF) - working-directory: src - - run: make -j$(getconf _NPROCESSORS_CONF) check VERBOSE=1 - working-directory: src - - run: make -j$(getconf _NPROCESSORS_CONF) distclean - working-directory: src - - # Then build & test out-of-tree. - - run: mkdir -p src/build/native - - run: ../../configure --disable-silent-rules - working-directory: src/build/native - - run: make -j$(getconf _NPROCESSORS_CONF) distcheck VERBOSE=1 - working-directory: src/build/native diff --git a/.github/workflows/close-pull-request.yml b/.github/workflows/close-pull-request.yml deleted file mode 100644 index da11c4a6..00000000 --- a/.github/workflows/close-pull-request.yml +++ /dev/null @@ -1,22 +0,0 @@ -# GitHub actions workflow. -# https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions - -# https://github.com/superbrothers/close-pull-request -name: Close Pull Request - -on: - pull_request_target: - types: [opened] - -jobs: - run: - runs-on: ubuntu-latest - steps: - - uses: superbrothers/close-pull-request@v3 - with: - comment: > - Thanks for your contribution! - Unfortunately, we don't use GitHub pull requests to manage code - contributions to this repository. - Instead, please see [README.md](../blob/HEAD/README.md) which - provides full instructions on how to get involved. diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml deleted file mode 100644 index 06f8e276..00000000 --- a/.github/workflows/coverity.yml +++ /dev/null @@ -1,43 +0,0 @@ -# GitHub actions workflow. -# https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions - -# https://scan.coverity.com/projects/google-breakpad -name: Coverity Scan - -on: - push: - branches: [main] - - schedule: - # The GH mirroring from Google GoB does not trigger push actions. - # Fire it once a week to provide some coverage. - - cron: '39 2 * * WED' - - # Allow for manual triggers from the web. - workflow_dispatch: - -jobs: - coverity: - runs-on: ubuntu-latest - env: - CC: clang - CXX: clang++ - steps: - - name: Checkout depot_tools - run: git clone --depth=1 https://chromium.googlesource.com/chromium/tools/depot_tools.git ../depot_tools - - - name: Checkout breakpad - run: | - set -xe - PATH+=:$PWD/../depot_tools - gclient config --unmanaged --name=src https://github.com/${{ github.repository }} - gclient sync --no-history --nohooks - - - run: ./configure --disable-silent-rules - working-directory: src - - - uses: vapier/coverity-scan-action@v1 - with: - command: make -C src -O -j$(getconf _NPROCESSORS_CONF) - email: ${{ secrets.COVERITY_SCAN_EMAIL }} - token: ${{ secrets.COVERITY_SCAN_TOKEN }} diff --git a/src/client/linux/microdump_writer/microdump_writer.cc b/src/client/linux/microdump_writer/microdump_writer.cc index 27c245a7..34a6eb63 100644 --- a/src/client/linux/microdump_writer/microdump_writer.cc +++ b/src/client/linux/microdump_writer/microdump_writer.cc @@ -475,8 +475,10 @@ class MicrodumpWriter { // Copy as many bytes of |identifier| as will fit into a MDGUID MDGUID module_identifier = {0}; - memcpy(&module_identifier, &identifier_bytes[0], - std::min(sizeof(MDGUID), identifier_bytes.size())); + if (!identifier_bytes.empty()) { + memcpy(&module_identifier, &identifier_bytes[0], + std::min(sizeof(MDGUID), identifier_bytes.size())); + } char file_name[NAME_MAX]; char file_path[NAME_MAX]; diff --git a/src/common/linux/file_id.cc b/src/common/linux/file_id.cc index 237d1545..36665c44 100644 --- a/src/common/linux/file_id.cc +++ b/src/common/linux/file_id.cc @@ -128,8 +128,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, wasteful_vector& identifier) { - identifier.resize(kMDGUIDSize); - void* text_section; size_t text_size; if ((!FindElfSection(elf_mapped_base, ".text", SHT_PROGBITS, @@ -137,11 +135,15 @@ static bool HashElfTextSection(const void* elf_mapped_base, !FindElfSection(elf_mapped_base, "text", SHT_PROGBITS, (const void**)&text_section, &text_size)) || text_size == 0) { + // Leave |identifier| untouched. A caller that sees bytes here would + // write them as a build id, turning "cannot identify this module" into + // a valid-looking all-zero one. return false; } // Only provide |kMDGUIDSize| bytes to keep identifiers produced by this // function backwards-compatible. + identifier.resize(kMDGUIDSize); my_memset(&identifier[0], 0, kMDGUIDSize); const uint8_t* ptr = reinterpret_cast(text_section); const uint8_t* ptr_end = ptr + std::min(text_size, static_cast(4096)); diff --git a/src/common/linux/file_id_unittest.cc b/src/common/linux/file_id_unittest.cc index 5b0c3fef..ad38146c 100644 --- a/src/common/linux/file_id_unittest.cc +++ b/src/common/linux/file_id_unittest.cc @@ -455,3 +455,19 @@ TYPED_TEST(FileIDTest, ConvertIdentifierToString) { ASSERT_EQ(kExpected, FileID::ConvertIdentifierToString(identifier)); } + +// A failed identification must leave the identifier EMPTY, so FillRawModule +// writes no CV record at all. Leaving bytes behind turns "cannot identify +// this module" into a valid-looking all-zero build id, which reads downstream +// as a missing symbol upload rather than a broken minidump. +TEST(FileIDTest, FailedIdentificationLeavesNoBytes) { + // Not an ELF: no build-id note to find, and no .text section to hash. + uint8_t not_an_elf[4096] = {}; + + PageAllocator allocator; + auto_wasteful_vector identifier(&allocator); + + EXPECT_FALSE( + FileID::ElfFileIdentifierFromMappedFile(not_an_elf, identifier)); + EXPECT_TRUE(identifier.empty()); +} diff --git a/src/common/memory_allocator.h b/src/common/memory_allocator.h index f2904f35..ec1bee7e 100644 --- a/src/common/memory_allocator.h +++ b/src/common/memory_allocator.h @@ -39,6 +39,7 @@ #include #include +#include #include #include diff --git a/src/common/memory_allocator_unittest.cc b/src/common/memory_allocator_unittest.cc index 08e0f8b8..1bdb8d7f 100644 --- a/src/common/memory_allocator_unittest.cc +++ b/src/common/memory_allocator_unittest.cc @@ -32,6 +32,7 @@ #include +#include #include #include "breakpad_googletest_includes.h" @@ -135,14 +136,21 @@ TEST(WastefulVectorTest, Simple) { EXPECT_EQ(0U, allocator_.pages_allocated()); wasteful_vector v(&allocator_); + size_t total_used = 0; for (unsigned i = 0; i < 256; ++i) { + const size_t prev_capacity = v.capacity(); v.push_back(i); + const size_t new_capacity = v.capacity(); + if (new_capacity > prev_capacity) { + total_used += new_capacity * sizeof(unsigned); + } ASSERT_EQ(i, v.back()); ASSERT_EQ(&v.back(), &v[i]); } ASSERT_FALSE(v.empty()); ASSERT_EQ(v.size(), 256u); - EXPECT_EQ(1U, allocator_.pages_allocated()); + EXPECT_LE(allocator_.pages_allocated(), + total_used / 4096 + 1); for (unsigned i = 0; i < 256; ++i) ASSERT_EQ(v[i], i); } diff --git a/src/processor/simple_serializer.h b/src/processor/simple_serializer.h index bd6cc84c..3681fc70 100644 --- a/src/processor/simple_serializer.h +++ b/src/processor/simple_serializer.h @@ -38,6 +38,7 @@ #define PROCESSOR_SIMPLE_SERIALIZER_H__ #include +#include #include "google_breakpad/common/breakpad_types.h" @@ -54,8 +55,9 @@ template class SimpleSerializer { // Write 'item' to memory location 'dest', and return to the "end" address of // data written, i.e., the address after the final byte written. static char* Write(const Type& item, char* dest) { - new (dest) Type(item); - return dest + SizeOf(item); + const size_t size = SizeOf(item); + memcpy(dest, &item, size); + return dest + size; } }; diff --git a/src/tools/linux/md2core/minidump-2-core.cc b/src/tools/linux/md2core/minidump-2-core.cc index be4be741..59ee589f 100644 --- a/src/tools/linux/md2core/minidump-2-core.cc +++ b/src/tools/linux/md2core/minidump-2-core.cc @@ -1287,16 +1287,18 @@ AugmentMappings(const Options& options, CrashedProcess* crashinfo, } } + const size_t aligned_filename_size = + PageAllocator::AlignUp(filename.size(), 8); if (std::distance(iter, crashinfo->link_map.end()) == 1) { link_map.l_next = 0; } else { link_map.l_next = (struct link_map*)(start_addr + data.size() + sizeof(link_map) + - PageAllocator::AlignUp(filename.size(), 8)); + aligned_filename_size); } data.append((char*)&link_map, sizeof(link_map)); data.append(filename); - data.append(8 - (filename.size() & 7), 0); + data.append(aligned_filename_size - filename.size(), 0); } AddDataToMapping(crashinfo, data, start_addr); diff --git a/src/tools/mac/upload_system_symbols/archive/extract.go b/src/tools/mac/upload_system_symbols/archive/extract.go index 965e154e..f8568b3b 100644 --- a/src/tools/mac/upload_system_symbols/archive/extract.go +++ b/src/tools/mac/upload_system_symbols/archive/extract.go @@ -312,6 +312,10 @@ func (e *installAssistantExtractor) hasCryptexes(plistPath string) (bool, error) } } +// imagePatchManifestPath is where an installer's asset archive holds the manifest +// describing its image patches, keyed by each patch's file name. +const imagePatchManifestPath = "AssetData/image_patches.plist" + // extractCachesFromZips extracts zips that contain dyld shared caches, and extracts the dyld shared caches from them. // The specifics depend on whether this installer uses cryptexes or payload files. func (e *installAssistantExtractor) extractCachesFromZips(zips []string, destination string, hasCryptexes bool) error { @@ -324,10 +328,17 @@ func (e *installAssistantExtractor) extractCachesFromZips(zips []string, destina return fmt.Errorf("couldn't extract caches from %v: %v", containerPath, err) } } else { + // The manifest goes somewhere other than `containerPath`, which + // extractCachesFromCryptexes reads expecting nothing but image patches. + manifestDir := path.Join(e.scratchDir, "image_patch_manifest") + if err := e.unarchiveZipsMatching(zips, manifestDir, imagePatchManifestPath); err != nil { + return err + } if err := e.unarchiveZipsMatching(zips, containerPath, "AssetData/payloadv2/image_patches/cryptex-system-*"); err != nil { return err } - if err := e.extractCachesFromCryptexes(containerPath, destination); err != nil { + manifest := path.Join(manifestDir, path.Base(imagePatchManifestPath)) + if err := e.extractCachesFromCryptexes(containerPath, manifest, destination); err != nil { return fmt.Errorf("couldn't extract caches from %v: %v", containerPath, err) } } @@ -428,8 +439,9 @@ func (e *installAssistantExtractor) copySharedCaches(from, to string) error { } // extractCachesFromCryptexes extracts disk images from any cryptexes at `cryptexesPath`, mounts them, -// and extracts any dyld shared caches to `destination`. -func (e *installAssistantExtractor) extractCachesFromCryptexes(cryptexesPath string, destination string) error { +// and extracts any dyld shared caches to `destination`. `manifest` is the image patch manifest +// describing those cryptexes. +func (e *installAssistantExtractor) extractCachesFromCryptexes(cryptexesPath string, manifest string, destination string) error { scratchDir, err := os.MkdirTemp(e.scratchDir, "cryptex_dmg") if err != nil { return err @@ -446,12 +458,32 @@ func (e *installAssistantExtractor) extractCachesFromCryptexes(cryptexesPath str e.vlog("Mounting %s at %s\n", cryptex, dmgPath) e.mountDMG(dmgPath, cryptexMountpoint) if err := e.copySharedCaches(cryptexMountpoint, destination); err != nil { + // A delta image patches another image rather than containing one, so it + // can't necessarily be turned into a disk image to take caches from. For now, + // ignore the failure and proceed to the next image cache extraction. + if e.isDeltaImage(manifest, f.Name()) { + e.vlog("Ignoring %v: %v. It patches another image rather than replacing it.\n", + cryptex, err) + continue + } return err } } return nil } +// isDeltaImage returns true if the image contains "ImagePatchBaselineTag" in the plist file, +// indicating that the image is a delta one, built on top of a baseline image. +func (e *installAssistantExtractor) isDeltaImage(manifestPath string, imageName string) bool { + print_cmd := fmt.Sprintf("print :%s:ImagePatchBaselineTag", imageName) + baseline, err := exec.Command("/usr/libexec/PlistBuddy", "-c", print_cmd, manifestPath).Output() + if err != nil { + return false + } + e.vlog("%v patches %v\n", imageName, strings.TrimSpace(string(baseline))) + return true +} + // extractCryptexDMG extracts the cryptex at `cryptexPath` to `dmgPath` using libParallelCompression. func (e *installAssistantExtractor) extractCryptexDMG(cryptexPath, dmgPath string) error { inp := C.CString("") diff --git a/src/tools/mac/upload_system_symbols/archive/extract_test.go b/src/tools/mac/upload_system_symbols/archive/extract_test.go new file mode 100644 index 00000000..1409f10f --- /dev/null +++ b/src/tools/mac/upload_system_symbols/archive/extract_test.go @@ -0,0 +1,73 @@ +package archive + +import ( + "os" + "path" + "testing" +) + +// imagePatchManifest is the image patch manifest from a macOS 27 installer, whose +// cryptex-system-arm64e.x1 image patches cryptex-system-arm64e rather than containing an +// image of its own. +const imagePatchManifest = ` + + + + cryptex-app + + ImagePatchTag + cryptex-app + + cryptex-system-arm64e + + ImagePatchTag + cryptex-system-arm64e + + cryptex-system-arm64e.x1 + + ImagePatchBaselineTag + cryptex-system-arm64e + ImagePatchTag + cryptex-system-arm64e.x1 + + cryptex-system-rosetta + + ImagePatchTag + cryptex-system-rosetta + + + +` + +func TestIsDeltaImage(t *testing.T) { + dir := t.TempDir() + manifestPath := path.Join(dir, "image_patches.plist") + if err := os.WriteFile(manifestPath, []byte(imagePatchManifest), 0644); err != nil { + t.Fatalf("couldn't write %s: %v", manifestPath, err) + } + e := &installAssistantExtractor{Extractor: &Extractor{}} + + cases := []struct { + imageName string + want bool + }{ + {"cryptex-system-arm64e", false}, + {"cryptex-system-arm64e.x1", true}, + {"cryptex-system-rosetta", false}, + {"cryptex-app", false}, + {"not-in-the-manifest", false}, + } + for _, c := range cases { + t.Run(c.imageName, func(t *testing.T) { + if got := e.isDeltaImage(manifestPath, c.imageName); got != c.want { + t.Errorf("isDeltaImage(%s) = %v, want %v", c.imageName, got, c.want) + } + }) + } + + t.Run("no manifest", func(t *testing.T) { + if e.isDeltaImage(path.Join(dir, "does_not_exist.plist"), "cryptex-system-arm64e.x1") { + t.Error("isDeltaImage with no manifest = true, want false") + } + }) +}