[M] Change in dart/sdk[main]: [vm/service]: Add _readNativeMemory RPC and OS::SafeReadMemory for sa...
Ben Konyi (Gerrit)
Ben Konyi voted and added 6 comments![]( "Open in Gerrit")
Votes added by Ben Konyi
| Commit-Queue | +1 |
6 comments
uint8_t* buffer = reinterpret_cast<uint8_t*>(malloc(size));I think we can use `CAllocUniquePtr<uint8_t>(malloc(size))` here to handle cleanup automatically.
char* hex = reinterpret_cast<char*>(malloc(size * 2 + 1));Same here, but with `CStringUniquePtr(malloc(size * 2 + 1))`.
response.AddProperty("error", "address not readable");I think we should define another error code here and use `response.PrintError(...)` instead.
The error codes are in `json_stream.h` and we can just add to the set of experimental RPC codes for now.
new StringParameter("address", true),Since we're using `"address"` and `"size"` in multiple places, I'd define constants for each and use them in place of the raw string literals. That way tooling can help autocomplete references to these parameter names and the compiler will catch any typos.
We'll also want to do the same with the response values.
static void ReadNativeMemory(Thread* thread, JSONStream* js) {Can you document the parameters and the formats of the possible response types?
Related details
- Daco Harkes
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 5 comments![]( "Open in Gerrit")
static bool SafeReadMemory(uintptr_t address, uint8_t* buffer, intptr_t size);We should add a `char** error` parameter to return any error. That way we can return the error provided by the underlying OS call.
static bool SafeReadMemory(uintptr_t address, uint8_t* buffer, intptr_t size);Nit: size_in_bytes as param name.
ssize_t bytes_read = pread64(fd, buffer, size, static_cast<off64_t>(address));if bytes_read is -1
`strerror(errno)` to get a readable error message to return. (Or `strerror_r`)
if (address > kMaxUserAddr) {We could possibly rely on the -1 return value to see an error and use `errno` to get the error from the OS. (It will probably just return "bad address").
EXPECT_SUBSTRING("\"bytes\":\"0102030405060708\"", handler.msg());🔥
Related details
- Ben Konyi
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nourhan Hasan (Gerrit)
Nourhan Hasan added 1 comment![]( "Open in Gerrit")
new StringParameter("address", true),Since we're using `"address"` and `"size"` in multiple places, I'd define constants for each and use them in place of the raw string literals. That way tooling can help autocomplete references to these parameter names and the compiler will catch any typos.
We'll also want to do the same with the response values.
Regarding the response, do you mean something similar that is defined at the top of runtime/vm/service.cc?
```
static constexpr const char* kNativeMemoryAddressParam = "address";
static constexpr const char* kNativeMemorySizeParam = "size";
static constexpr const char* kNativeMemoryBytesKey = "bytes";
static constexpr const char* kNativeMemoryErrorKey = "error";
```
Related details
- Ben Konyi
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nourhan Hasan (Gerrit)
Nourhan Hasan voted and added 8 comments![]( "Open in Gerrit")
Votes added by Nourhan Hasan
| Auto-Submit | +1 |
8 comments
static bool SafeReadMemory(uintptr_t address, uint8_t* buffer, intptr_t size);Nit: size_in_bytes as param name.
Done
static bool SafeReadMemory(uintptr_t address, uint8_t* buffer, intptr_t size);We should add a `char** error` parameter to return any error. That way we can return the error provided by the underlying OS call.
Done
ssize_t bytes_read = pread64(fd, buffer, size, static_cast<off64_t>(address));if bytes_read is -1
`strerror(errno)` to get a readable error message to return. (Or `strerror_r`)
Done
We could possibly rely on the -1 return value to see an error and use `errno` to get the error from the OS. (It will probably just return "bad address").
Done
uint8_t* buffer = reinterpret_cast<uint8_t*>(malloc(size));I think we can use `CAllocUniquePtr<uint8_t>(malloc(size))` here to handle cleanup automatically.
Done
char* hex = reinterpret_cast<char*>(malloc(size * 2 + 1));Same here, but with `CStringUniquePtr(malloc(size * 2 + 1))`.
Done
I think we should define another error code here and use `response.PrintError(...)` instead.
The error codes are in `json_stream.h` and we can just add to the set of experimental RPC codes for now.
Done
EXPECT_SUBSTRING("\"bytes\":\"0102030405060708\"", handler.msg());Nourhan Hasan🔥
Done
Related details
- Ben Konyi
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nourhan Hasan (Gerrit)
Nourhan Hasan added 1 comment![]( "Open in Gerrit")
static void ReadNativeMemory(Thread* thread, JSONStream* js) {Can you document the parameters and the formats of the possible response types?
Is there a specific file to do this? I think the `runtime/vm/service/service.md` file is for Public RPC documentation
Or should I do that as a comment on the RPC right now?
Daco Harkes (Gerrit)
Daco Harkes added 1 comment![]( "Open in Gerrit")
Lets make sure we add all relevant bots so that it doesn't turn red on the CI after landing:
`Cq-Include-Trybots: dart/try:vm-asan-linux-release-x64-try,vm-asan-mac-release-arm64-try,vm-asan-win-release-x64-try,vm-dyn-linux-debug-x64-try,vm-dyn-mac-debug-arm64-try,vm-ffi-qemu-linux-release-arm-try,vm-ffi-qemu-linux-release-riscv64-try,vm-linux-debug-arm64-try,vm-linux-debug-ia32-try,vm-linux-debug-simriscv32-try,vm-linux-debug-simriscv64-try,vm-linux-debug-x64-try,vm-linux-debug-x64c-try,vm-linux-release-simarm-try,vm-mac-debug-arm64-try,vm-mac-debug-x64-try,vm-msan-linux-release-x64-try,vm-reload-linux-debug-x64-try,vm-tsan-linux-release-x64-try,vm-tsan-mac-release-arm64-try,vm-ubsan-linux-release-x64-try,vm-ubsan-mac-release-arm64-try,vm-ubsan-win-release-x64-try,vm-win-debug-arm64-try,vm-win-debug-x64-try,vm-win-debug-x64c-try`
(found via `dart tools/find_builders.dart vm/cc/Service_Code`)
Related details
- Ben Konyi
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nourhan Hasan (Gerrit)
Nourhan Hasan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nourhan Hasan
| Auto-Submit | +1 |
Lets make sure we add all relevant bots so that it doesn't turn red on the CI after landing:
`Cq-Include-Trybots: dart/try:vm-asan-linux-release-x64-try,vm-asan-mac-release-arm64-try,vm-asan-win-release-x64-try,vm-dyn-linux-debug-x64-try,vm-dyn-mac-debug-arm64-try,vm-ffi-qemu-linux-release-arm-try,vm-ffi-qemu-linux-release-riscv64-try,vm-linux-debug-arm64-try,vm-linux-debug-ia32-try,vm-linux-debug-simriscv32-try,vm-linux-debug-simriscv64-try,vm-linux-debug-x64-try,vm-linux-debug-x64c-try,vm-linux-release-simarm-try,vm-mac-debug-arm64-try,vm-mac-debug-x64-try,vm-msan-linux-release-x64-try,vm-reload-linux-debug-x64-try,vm-tsan-linux-release-x64-try,vm-tsan-mac-release-arm64-try,vm-ubsan-linux-release-x64-try,vm-ubsan-mac-release-arm64-try,vm-ubsan-win-release-x64-try,vm-win-debug-arm64-try,vm-win-debug-x64-try,vm-win-debug-x64c-try`
(found via `dart tools/find_builders.dart vm/cc/Service_Code`)
- Ben Konyi
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Ben Konyi added 2 comments![]( "Open in Gerrit")
new StringParameter("address", true),Nourhan HasanSince we're using `"address"` and `"size"` in multiple places, I'd define constants for each and use them in place of the raw string literals. That way tooling can help autocomplete references to these parameter names and the compiler will catch any typos.
We'll also want to do the same with the response values.
Regarding the response, do you mean something similar that is defined at the top of runtime/vm/service.cc?
```
static constexpr const char* kNativeMemoryAddressParam = "address";
static constexpr const char* kNativeMemorySizeParam = "size";
static constexpr const char* kNativeMemoryBytesKey = "bytes";
static constexpr const char* kNativeMemoryErrorKey = "error";
```
Yeah, basically! They don't need to be at the top of the file though (you can define them near the functions you've added).
static void ReadNativeMemory(Thread* thread, JSONStream* js) {Nourhan HasanCan you document the parameters and the formats of the possible response types?
Is there a specific file to do this? I think the `runtime/vm/service/service.md` file is for Public RPC documentation
Or should I do that as a comment on the RPC right now?
- Daco Harkes
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 4 comments![]( "Open in Gerrit")
Utils::SNPrint(hex.get() + i * 2, 3, "%02x", buffer.get()[i]);I think it's better to use hex encoding than two chars per byte.
uint8_t buffer[8] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};Add bytes above 0x9 e.g. 0xF
EXPECT_SUBSTRING("\"bytes\":\"0102030405060708\"", handler.msg());I think we should use hex instead for encoding the bytes in JSON. (Unless @bko...@google.com has a different idea.)
(Reasoning: ints is too verbose. And base64 is harder to inspect and work with.)
Also add a test with a pretty big size. E.g. allocate like a MB and fill it in a for loop. And test that reading an MB through the JSON protocol works as expected.
(Probably an MB is the largest we're interested in. Most likely we'll read <1 KB most of the time. E.g. even if you an array of super large structs we'd probably have some kind of pagination UI to browse through it. And then we could do a JSON call when we flip the pages.)
Related details
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nourhan Hasan (Gerrit)
Nourhan Hasan added 1 comment![]( "Open in Gerrit")
Utils::SNPrint(hex.get() + i * 2, 3, "%02x", buffer.get()[i]);I think it's better to use hex encoding than two chars per byte.
Could you please clarify? I believe the current code is already using hex encoding.
Is there a different encoding approach you had in mind?
Related details
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nourhan Hasan (Gerrit)
Nourhan Hasan voted and added 4 comments![]( "Open in Gerrit")
Votes added by Nourhan Hasan
| Auto-Submit | +1 |
4 comments
new StringParameter("address", true),Nourhan HasanSince we're using `"address"` and `"size"` in multiple places, I'd define constants for each and use them in place of the raw string literals. That way tooling can help autocomplete references to these parameter names and the compiler will catch any typos.
We'll also want to do the same with the response values.
Ben KonyiRegarding the response, do you mean something similar that is defined at the top of runtime/vm/service.cc?
```
static constexpr const char* kNativeMemoryAddressParam = "address";
static constexpr const char* kNativeMemorySizeParam = "size";
static constexpr const char* kNativeMemoryBytesKey = "bytes";
static constexpr const char* kNativeMemoryErrorKey = "error";
```
Yeah, basically! They don't need to be at the top of the file though (you can define them near the functions you've added).
Done
static void ReadNativeMemory(Thread* thread, JSONStream* js) {Nourhan HasanCan you document the parameters and the formats of the possible response types?
Ben KonyiIs there a specific file to do this? I think the `runtime/vm/service/service.md` file is for Public RPC documentation
Or should I do that as a comment on the RPC right now?
I'd just add it as documentation here.
Done
uint8_t buffer[8] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};Add bytes above 0x9 e.g. 0xF
Done
Also add a test with a pretty big size. E.g. allocate like a MB and fill it in a for loop. And test that reading an MB through the JSON protocol works as expected.
(Probably an MB is the largest we're interested in. Most likely we'll read <1 KB most of the time. E.g. even if you an array of super large structs we'd probably have some kind of pagination UI to browse through it. And then we could do a JSON call when we flip the pages.)
- Ben Konyi
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 1 comment![]( "Open in Gerrit")
Utils::SNPrint(hex.get() + i * 2, 3, "%02x", buffer.get()[i]);Nourhan HasanI think it's better to use hex encoding than two chars per byte.
Could you please clarify? I believe the current code is already using hex encoding.
Is there a different encoding approach you had in mind?
Ah yes, I meant why do we use `02`? why the zeros in between? The buffer is `uint8_t` so one hex char should be enough to represent a byte.
Related details
- Ben Konyi
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Ben Konyi added 1 comment![]( "Open in Gerrit")
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nourhan Hasan (Gerrit)
Nourhan Hasan added 1 comment![]( "Open in Gerrit")
Utils::SNPrint(hex.get() + i * 2, 3, "%02x", buffer.get()[i]);Nourhan HasanI think it's better to use hex encoding than two chars per byte.
Daco HarkesCould you please clarify? I believe the current code is already using hex encoding.
Is there a different encoding approach you had in mind?
Ah yes, I meant why do we use `02`? why the zeros in between? The buffer is `uint8_t` so one hex char should be enough to represent a byte.
I don't completely agree with you. Actually, a `uint8_t` holds 8 bits, which needs 2 hex digits to represent , one hex digit only covers 4 bits. Without the zero, the byte `0x0A` would print as `a` instead of `0a`, making the output ambiguous.
I also found that this is the standard way of hex encoding, such as in SHA-256 hashes.
Related details
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes voted and added 5 comments![]( "Open in Gerrit")
Votes added by Daco Harkes
| Code-Review | +1 |
5 comments
Utils::SNPrint(hex.get() + i * 2, 3, "%02x", buffer.get()[i]);Nourhan HasanI think it's better to use hex encoding than two chars per byte.
Daco HarkesCould you please clarify? I believe the current code is already using hex encoding.
Is there a different encoding approach you had in mind?
Nourhan HasanAh yes, I meant why do we use `02`? why the zeros in between? The buffer is `uint8_t` so one hex char should be enough to represent a byte.
I don't completely agree with you. Actually, a `uint8_t` holds 8 bits, which needs 2 hex digits to represent , one hex digit only covers 4 bits. Without the zero, the byte `0x0A` would print as `a` instead of `0a`, making the output ambiguous.
I also found that this is the standard way of hex encoding, such as in SHA-256 hashes.
Right. Apparently I missed my coffee. 😂
// "bytes": "0102030405060708"address starts with 0x but bytes does not. Maybe we should be consistent.
if (!GetIntegerId(size_str, &size) || size <= 0 || size > 1 * MB) {Is there a reason we don't support more than 1 MB?
If the system calls support it, and the JSON encoding can handle it, I don't see a reason to limit the number.
EXPECT_SUBSTRING("\"bytes\":\"0102030405060708\"", handler.msg());I think we should use hex instead for encoding the bytes in JSON. (Unless @bko...@google.com has a different idea.)
(Reasoning: ints is too verbose. And base64 is harder to inspect and work with.)
Done
EXPECT_SUBSTRING("\"bytes\":", handler.msg());Maybe expect at least one hex value in the "bytes" field as well.
Related details
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nourhan Hasan (Gerrit)
Nourhan Hasan added 2 comments![]( "Open in Gerrit")
// "bytes": "0102030405060708"address starts with 0x but bytes does not. Maybe we should be consistent.
The actual response is this way:
`response.AddPropertyF(kNativeMemoryAddressParam, "0x%" Px "", address);`
Do you want me to change that as well?
if (!GetIntegerId(size_str, &size) || size <= 0 || size > 1 * MB) {Is there a reason we don't support more than 1 MB?
If the system calls support it, and the JSON encoding can handle it, I don't see a reason to limit the number.
I did this as a precaution, to prevent excessive memory load and usage on the VM, and for security purposes, because the memory might be occupied by large amounts of data by malicious clients.
Do you have a different opinion? Or would you like me to raise or remove the limit?
Related details
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 2 comments![]( "Open in Gerrit")
// "bytes": "0102030405060708"Nourhan Hasanaddress starts with 0x but bytes does not. Maybe we should be consistent.
The actual response is this way:
`response.AddPropertyF(kNativeMemoryAddressParam, "0x%" Px "", address);`
Do you want me to change that as well?
`response.AddProperty(kNativeMemoryBytesKey, "0x%s", hex.get());` ?
But then reading the string becomes more annoying because we would need to skip the `0x` always when indexing into the string.
So maybe being inconsistent is fine. Both are hex, but address is short and will be read at once. while the bytes will likely be parsed/read in a loop or indexing into the string.
if (!GetIntegerId(size_str, &size) || size <= 0 || size > 1 * MB) {Nourhan HasanIs there a reason we don't support more than 1 MB?
If the system calls support it, and the JSON encoding can handle it, I don't see a reason to limit the number.
I did this as a precaution, to prevent excessive memory load and usage on the VM, and for security purposes, because the memory might be occupied by large amounts of data by malicious clients.
Do you have a different opinion? Or would you like me to raise or remove the limit?
Hm, we can keep it. We can always remove it later if we want or need to.
Related details
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 1 comment![]( "Open in Gerrit")
This LGTM once Daco is happy with it!
We need your +1 as well Ben to submit!
Related details
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Ben Konyi voted![]( "Open in Gerrit")
| Code-Review | +1 |
| Commit-Queue | +2 |
Related details
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes submitted the change![]( "Open in Gerrit")
Change information
[vm/service]: Add _readNativeMemory RPC and OS::SafeReadMemory for safe native memory inspection
- Declare OS::SafeReadMemory in os.h
- Implement using pread64(/proc/self/mem) on Linux and Android
- Add _readNativeMemory VM Service RPC with pre-checks for null
and address overflow
TEST=runtime/vm/service_test.cc
- M runtime/tests/vm/vm.status
- M runtime/vm/json_stream.h
- M runtime/vm/os.h
- M runtime/vm/os_android.cc
- M runtime/vm/os_linux.cc
- M runtime/vm/service.cc
- M runtime/vm/service_test.cc
Code-Review: +1 by Daco Harkes, +1 by Ben Konyi
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov added 3 comments![]( "Open in Gerrit")
pread64(fd, buffer, size_in_bytes, static_cast<off64_t>(address));Should this handle `EINTR`? If not - why not?
*error = strerror(errno);If `close` also fails it will clobber `errno`.
Related details
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |