Page MenuHomePhabricator

Introduce `RemoteAddressSpaceView` and `VMReadContext`.
Needs ReviewPublic

Authored by delcypher on Wed, Jan 2, 10:35 AM.

Details

Summary

RemoteAddressSpaceView is the remote memory counter part to
LocalAddressSpaceView that implements the AddressSpaceView
interface.

The RemoteAddressSpaceView implementation is a simple wrapper around a
VMReadContext object. A VMReadContext object implements the platform
specific code to

  • Read memory from another process.
  • Cache those reads and manage the cache lifetime.
  • Manage the writability of the cached memory.

Two implementations are provided:

  • A Darwin only implementation that allows in-process and out-of-process

reads. The reads are cached by a Darwin specific framework
(CoreSymbolication) and thus the VMReadContext implementation does not
implement a cache.

  • A non-Darwin implementation that only allows in-process reads

(basically a no-op). This exists only for testing purposes.

Tests for this functionality will be added in later patches.

rdar://problem/45284065

Event Timeline

delcypher created this revision.Wed, Jan 2, 10:35 AM
Herald added subscribers: Restricted Project, mgorny, kubamracek. · View Herald TranscriptWed, Jan 2, 10:35 AM
delcypher updated this revision to Diff 179889.Wed, Jan 2, 10:40 AM

Fix some comments.

Would it make sense to also extend this to Linux using process_vm_readv/process_vm_writev?

Would it make sense to also extend this to Linux using process_vm_readv/process_vm_writev?

Perhaps, but certainly not in this patch because I consider that out of scope. On Darwin we already have tools that will be using the new functionality, on Linux the tools don't exist yet. So implementing VMReadContext for Darwin is much more useful at this time.
The interface should be generic enough that we could add Linux support in the future.

vitalybuka added inline comments.Tue, Jan 8, 4:10 PM
lib/sanitizer_common/sanitizer_remote_address_space_view.h
28

Why all these stuff needs to be static?

delcypher added inline comments.Wed, Jan 9, 1:22 AM
lib/sanitizer_common/sanitizer_remote_address_space_view.h
28

@vitalybuka Because the allocators don't receive an instance of RemoteAddressSpaceView, they receive the type as a template parameter (e.g. SizeClassAllocator64). This design choice means that everything in an implementation of the AddressSpaceView interface (i.e. LocalAddressSpaceView and RemoteAddressSpaceView) needs to be static.

To implement RemoteAddressSpaceView we have to store some state. All of this state and the read operations are encapsulated in the VMReadContext object. We cannot store any of this state inside the allocators because that could change the data-layout.

This design is not thread-safe of course but for allocator enumeration on Darwin will only ever be doing that in a single thread anyway.

vitalybuka added inline comments.Wed, Jan 9, 12:07 PM
lib/sanitizer_common/sanitizer_remote_address_space_view.h
28

Can you add some CHECKs validate this single thread assumption?

lib/sanitizer_common/sanitizer_vm_read_context.h
28

could you please init these inline?

vitalybuka added inline comments.Wed, Jan 9, 1:11 PM
lib/sanitizer_common/sanitizer_remote_address_space_view.h
26

struct -> class

69

maybe nested: ScopedRemoteAddressSpaceViewContext -> RemoteAddressSpaceView::ScopedContest
and make RemoteAddressSpaceView::set_context private

69

class and
private:

VMReadContext vmrc_;
lib/sanitizer_common/sanitizer_vm_read_context.h
25

Just define "typedef int ProcessHandle;" and remove process_vm_read_handle_t
On other platforms it does not matter, and we will add more specific type if needed later.

32

historically sanitizer common uses Google C++ naming convention so members need to have _ suffux.

51

Please remove all stuff you are not using in upcoming patches.
When can extend API when needed

74

If you can calculate it from target_process please remove member and simplify state

94

As I understand we do not expect a lot of context callers
so let remove default "writable = true" and make calls more verbose

lib/sanitizer_common/sanitizer_vm_read_context_common.cc
20 ↗(On Diff #179889)

Remove it if you don't use it.
I'd expected ScopedContext is the why to reset it.

lib/sanitizer_common/sanitizer_vm_read_context_mac.cc
43

Just remove it. You'll add it when needed

delcypher marked 9 inline comments as done.Fri, Jan 11, 11:00 AM
delcypher added inline comments.
lib/sanitizer_common/sanitizer_remote_address_space_view.h
26

LocalAddressSpaceView is a struct. Making RemoteAddressSpaceView be a class seems inconsistent. Why do you want to make this change?

28

Can you add some CHECKs validate this single thread assumption?

@vitalybuka

I don't see an easy way to do this because this code will be used in a context where there is no ThreadRegistry and there is no interception on pthread_create and other thread functions. I also couldn't see a non-asan specific (e.g. I found AsanThread* GetCurrentThread()) API to use because this code will be common to all sanitizers.

There are also examples of existing code that don't check the single thread assumption. E.g. from lib/asan/asan_thread.cc

ThreadRegistry &asanThreadRegistry() {
  static bool initialized;
  // Don't worry about thread_safety - this should be called when there is
  // a single thread.
...

I'm not against adding some sort of check to validate the single thread assumption but I need more guidance on how to do this using the available sanitizer internal APIs.

69

maybe nested: ScopedRemoteAddressSpaceViewContext -> RemoteAddressSpaceView::ScopedContest

and make RemoteAddressSpaceView::set_context private

That's a great idea. I'll do that.

lib/sanitizer_common/sanitizer_vm_read_context.h
25

Apart from that fact that it needs to be unsigned on Darwin. I'm not super comfortable doing that because the declaration of __sanitizer::process_vm_read_handle_t in sanitizer_internal_defs.h allows to detect if the system declaration of task_t disagrees with the Sanitizer runtimes type. This is because on Darwin we include system headers that do define the task_t type.

28

Yes. I'll do that.

32

I'll try to fix that.

51

I'll have a look through my out-of-tree implementation and see what I can remove.

94

Sure.

lib/sanitizer_common/sanitizer_vm_read_context_common.cc
20 ↗(On Diff #179889)

Reset() gets called if the read call fails. Although I've just realised that's actually inconsistent with the API comments. I'll fix that.

delcypher marked an inline comment as done.Fri, Jan 11, 12:23 PM
delcypher added inline comments.
lib/sanitizer_common/sanitizer_vm_read_context_mac.cc
43

By "it" I presume you mean that writable argument?

But if I do that, how far up the stack should I go? Taken to the extreme that means reverting what was landed in https://reviews.llvm.org/D54879 . I really don't want to do that because the distinction between writable and read-only access is likely important.

delcypher updated this revision to Diff 181566.Mon, Jan 14, 8:49 AM

Lots of clean-up.

delcypher marked 6 inline comments as done and 2 inline comments as done.Mon, Jan 14, 8:52 AM
delcypher added inline comments.
lib/sanitizer_common/sanitizer_vm_read_context.h
74

We cannot calculate it. It needs to be part of the sate.

delcypher updated this revision to Diff 182057.Wed, Jan 16, 8:13 AM
delcypher marked an inline comment as done.
  • Remote fields/methods in VMReadContext that won't be used.
  • Remove default writable=true argument.
delcypher marked 5 inline comments as done.Wed, Jan 16, 8:15 AM
delcypher added inline comments.
lib/sanitizer_common/sanitizer_vm_read_context.h
51

I've removed several fields/methods from this patch that weren't being used in my internal implementation.

delcypher marked an inline comment as done.Wed, Jan 16, 8:16 AM

@vitalybuka I've fixed what I can and I've left questions about things that aren't straightforward to fix or that I don't agree with. When you have time could you take another look?

vitalybuka added inline comments.Wed, Jan 16, 1:37 PM
lib/sanitizer_common/sanitizer_remote_address_space_view.h
26

Historically sanitizers follow google c++ style and class is more appropriate here
https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes

as well for LocalAddressSpaceView

28

what about saving GetTid() on the first call of set_context()
then check that saved id matches GetTid() for all subsequent calls to
set_context()
get_context()
Load*()

I assume this is not going to be main thread?

67

same for class

lib/sanitizer_common/sanitizer_vm_read_context.h
17

empty live between define and include

25

then just define process_vm_read_handle_t as task_t in sanitizer_internal_defs for Darwin?

30

please init target_process_ to something target_process_ as well

42

why copying is not acceptable?

74

We cannot calculate it. It needs to be part of the sate.

I read this as:

bool IsLocal() const {
  return config_ == nullptr || mach_task_self() == target_process_;
}
lib/sanitizer_common/sanitizer_vm_read_context_mac.cc
28

can you remove "config" argument and just initialize config_ to darwin specific stuff here?

29

if we keep it please remove {} for consistency

43

It still looks like better to split Read and ReadWriteble when we have different implementation for them.
So If you have/expect such patch quite soon, then fine, let's keep it. If this is just expectation that some-when in undefined future we will likely need to spit them, then I'd prefer we split when it's actually necessary.

50

why do we need config_?
from here looks like you expect a particular function
could you call it directly here?