Alias settings_client internals to see it in a memory dump. #26654
Conversation
goodov
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@bridiver @iefremov @atuchin-m inviting some C++ experts to validate the approach I'm using here to have the additional data in a memory dump. I'm pretty sure the undefined behavior @cdesouza-chromium mentions is not applicable here, because no access to the copied memory is performed, but maybe I'm missing something. |
For some context let me add my comment from the original PR here as well: After scouring everywhere for this, I finally found some concrete answers about the object model and how StackObjectCopy may be violating it. Unfortunately we can't have this type, and I think that makes sense why chromium doesn't offer one. The issue here is the C++ Object Model. We are invoking UB both when we reinterpret_cast the byte array, and when we copy the bytes to the array too[1].
Of course, if the objects were trivially copyable, we could just copy them to the stack, and we wouldn't need this StackObjectCopy in the first place. However, memcpying non-trivially-copiable objects ends up subverting the language's memory model, and invoking Undefined Behaviour. This wouldn't invoke undefined behaviour if T was constrained to std::is_trivially_copyable[2], but I suspect StackObjectCopy wouldn't be of use with that constraint. [1] https://en.cppreference.com/w/cpp/types/is_trivially_copyable Now for some added context. The access itself is not not the problem, although access would also be undefined behaviour, but rather the copy itself is undefined, and that makes the reinterpret cast undefined too. Undefined behaviour is banned as a foregone conclusion by the guidelines. It is what the standard calls illegal. It is really annoying that C++ makes this undefined, because of the rigid object lifetime model. What could happen then if we let this one in? It is unknown, this could work for now, and then due to code changes around, the compiler could get entirely confused and start emitting wrong code. This is the problem with undefined behaviour, it makes the program invalid, and lets the compiler makes wrong assumptions about things. I particularly am not fond of being a nuisance here about undefined behaviour, but this is life with C++ and I hate being the annoying voice to bring that up. |
There was a problem hiding this comment.
Okay, so I have had some further discussions about this and questions involving crashpad. I think the issue is fairly complex when we are considering crash dumps, etc, because it is already too low level, and @goodov does have a point about that, and the fact the value never gets read.
Is it undefined to copy the data? Maybe not, because no read is taking place. The thing that could potentially be UB is the reinterpret_cast at this point, but again, we don't read that pointer either. Will compilers mess this one? Meh, doesn't seem it would ever happen.
I don't want to be pedantic about undefined behaviour and end up blocking people's work for something that only theoretically could break, and that ins't even certain to be UB.
So LGTM.
I have a few recommendations though:
- It is not clear that serialising some virtual abstract class will produce useful debugging data, because the serialisation is not guaranteed to be well formed. We are slicing the type, and even
sizeof(T)is unreliable at this point, so it is better to double check if these debugging helpers are indeed helping. - The better approach here would be for types like
blink::WebContentSettingsClientto support string serialisation for debugging. Plenty of types do that[1], and there's a case to be madeStackObjectCopyis the wrong tool for the job and we should be getting debug strings in some of these types. This would involve submitting work to upstream though, which may not be convenient.
[1] https://source.chromium.org/chromium/chromium/src/+/main:base/values.h;l=1125
|
I'm not sure about UB, but from university years i recall that you can't really assume that copying if there is a large enough hierarchy of (virtual) multiple inheritance that T belongs to, your |
| private: | ||
| constexpr static size_t kSize = sizeof(T); | ||
|
|
||
| alignas(T) uint8_t buffer_[kSize]; |
There was a problem hiding this comment.
Would that be enough to extract and store just vtable pointer?
| // | ||
| // Can't use `byte_span_from_ref` because `T` might be an abstract class. | ||
| auto original_object_memory = UNSAFE_BUFFERS( | ||
| make_span(reinterpret_cast<const uint8_t*>(original), kSize)); |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Potentially unsafe C++ construct detected
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/unsafe-cpp-constructs.yaml
Cc @stoletheminerals @thypon @cdesouza-chromium
There was a problem hiding this comment.
I don't think you need make_span there. It can only be span I suspect.
| // | ||
| // Can't use `byte_span_from_ref` because `T` might be an abstract class. | ||
| auto original_object_memory = UNSAFE_BUFFERS( | ||
| make_span(reinterpret_cast<const uint8_t*>(original), kSize)); |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Using reinterpret_cast against some data types may lead to undefined behaviour. In general, when needing to do these conversions, check how Chromium upstream does them. Most of the times a reinterpret_cast is wrong and there's no guarantee the compiler will generate the code that you thought it would.
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/reinterpret_cast.yaml
Cc @stoletheminerals @thypon @cdesouza-chromium
|
Released in v1.75.44 |
Look into the client to see what the actual type was when we got the list. I have a slight suspicion that we get the non-overridden version of
WorkerContentSettingsClient, because any other code path should return a non-emptyShieldsSettingsPtrobject.Related brave/brave-browser#41889
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: