Repository navigation
Merging upstream main from commit 6598c9c3 - #33
Merged
Merged
Conversation
std::max_align_t is defined in header <cstddef> which is pulled in transitively via <memory>/<vector> on some libstdc++ versions, but that's not guaranteed. On GCC 11 that transitive include doesn't happen, so std::max_align_t is unresolved. This commit explicitly includes <cstddef> to resolve the issue. Bug: 538420330 Change-Id: I02805d316be9606e28d77ba592ac26865797c552 Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/8140782 Reviewed-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org>
In SimpleSerializer<Type>::Write, placement new was used to construct objects at the destination memory address. When serializing data into unaligned byte buffers, dest is not guaranteed to satisfy alignof(Type), causing undefined behavior and runtime errors under AddressSanitizer or UndefinedBehaviorSanitizer (e.g. misaligned_pointer_use on unsigned long). Replace placement new with memcpy so that bytes are copied safely regardless of pointer alignment. Bug: 540850595 Change-Id: I7e0137ee92935665d7447a4490bbaaab462d0f58 Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/8231125 Reviewed-by: Joshua Peraza <jperaza@chromium.org> Reviewed-by: Ivan Penkov <ivanpe@google.com>
Keep AlignUp for l_next (no over-padding when filename is 8-aligned) and derive padding from the same aligned size so they always match. Change-Id: Idb47659115bc168fd3728cd97573219f8f8f42b8 Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/8259656 Reviewed-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Mark Mentovai <mark@chromium.org>
HashElfTextSection is the fallback used when an ELF has no build id
note: hash the first page of .text. It resized the output buffer before
the check that can fail, so when .text could not be found either it
returned false with 16 zero bytes left in the identifier buffer.
The caller discards the return value and tests the buffer instead:
dumper_->ElfFileIdentifierForMapping(mapping, member, mapping_id,
identifier_bytes);
...
if (!identifier_bytes.empty()) {
wasteful_vector derives from std::vector, so resize value-initializes
and the bytes are deterministically zero. A CV record was therefore
written with an all-zero build id whenever identification failed.
That is why the symptom read as a missing symbol upload rather than a
broken dump: an all-zero build id looks like a build id nobody uploaded
symbols for, while no build id at all points at the writer. Move the
resize below the check so a failed identification leaves the buffer
empty and no CV record is written.
MicrodumpWriter indexed the identifier unconditionally, which is
undefined on an empty vector even though the copy length would be zero.
Guard it.
Testing
Built libbreakpad_client.a, core2md, dump_syms and minidump_stackwalk.
The autotools suite passes on x86-64 Linux: 48 of 48 test programs.
FailedIdentificationLeavesNoBytes in file_id_unittest.cc covers it:
identification of a buffer with no build id note and no .text must
return false and leave the identifier empty. It fails without this
commit, with a 16-byte identifier of zeros.
Change-Id: Ie2d787320c459b0dbfadab8828020f15574682d5
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/8398976
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
upload system symbols is failing on the latest MacOS27 installer, while extracting dyld cache from `cryptex_cryptex-system-arm64e.x1` with the error `couldn't find System/Library/dyld`. `.x1` image is a delta image on top of a baseline image, so it seems like it does not contain dyld cache. Bug: 562142544 Change-Id: I1fa249832009c321a69d782411d1d877faaecf26 Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/8415769 Reviewed-by: Mark Mentovai <mark@chromium.org>
WastefulVectorTest.Simple pushes 256 elements into a wasteful_vector backed by a PageAllocator and previously asserted that exactly 1 page (4096 bytes) was allocated. Because PageAllocator does not free old allocations when std::vector reallocates, the total memory allocated depends on the std::vector implementation's growth factor and capacity decisions (for example, when using a lower growth factor or size-returning allocation). Track the total bytes used across capacity increases and verify that allocator_.pages_allocated() is at most total_used / 4096 + 1. Also include <cstddef> for size_t. Change-Id: I0642a3edd2747a72fae1fff61b9191b0945c650f Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/8465926 Reviewed-by: Primiano Tucci <primiano@chromium.org> Reviewed-by: Ivan Penkov <ivanpe@google.com>
|
Thanks for your contribution! Unfortunately, we don't use GitHub pull requests to manage code contributions to this repository. Instead, please see README.md which provides full instructions on how to get involved. |
Member
Author
|
LOL |
We definitely don't want the close-pull-request action. The build-test-ci and coverity workflows make heavy usage of gclient, we may not do that. If we want a CI, we will do it ourselves our own way.
Member
Author
|
I actually cherry-picked the commit from: So the annoying PR-closing GitHub action will be removed when merging this. |
Member
|
LGTM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The few commits that had been merged upstream since I started working on merging the upstream
main:Should be merged by hand to keep both original authors and committers.