Add worker_readPtr and worker_writePtr and use where feasible#1264
Add worker_readPtr and worker_writePtr and use where feasible#1264sporksmith merged 3 commits intoshadow:devfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1264 +/- ##
==========================================
- Coverage 55.75% 55.55% -0.20%
==========================================
Files 141 141
Lines 20462 20602 +140
Branches 5028 5059 +31
==========================================
+ Hits 11408 11446 +38
- Misses 5945 6035 +90
- Partials 3109 3121 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
2c5fe8a to
cacccd4
Compare
stevenengler
left a comment
There was a problem hiding this comment.
Requesting changes just for the discarding packet issue. I think it would be better to either discard all packets or none.
| const void* mapped = memorymanager_getReadablePtr(proc->memoryManager, thread, src, n); | ||
| memcpy(dst, mapped, n); | ||
| return 0; |
There was a problem hiding this comment.
Would it be better to do the copying logic within the memory manager instead (with a memorymanager_readPtr() and memorymanager_writePtr())? This way the Rust memory manager interface would be safe.
There was a problem hiding this comment.
I think the main APIs to use in the syscallhandlers going forward will be worker_*, and we can build safe(r) Rust interfaces on top of those. The "typed" memorymanager interfaces that are currently sketched out but unused will probably end up being removed, though we can reuse some of the API ideas for the new APIs.
In particular readPtr and writePtr makes less sense for the MemoryManager, since its main purpose is to remap pointers - it can let the caller deal with copies if they're needed.
| const void* worker_getReadablePtr(PluginVirtualPtr src, size_t n) { | ||
| Worker* worker = _worker_getPrivate(); | ||
| return process_getReadablePtr(worker->active.process, worker->active.thread, src, n); | ||
| } | ||
|
|
||
| void* worker_getWriteablePtr(PluginVirtualPtr dst, size_t n) { | ||
| Worker* worker = _worker_getPrivate(); | ||
| return process_getWriteablePtr(worker->active.process, worker->active.thread, dst, n); | ||
| } No newline at end of file |
There was a problem hiding this comment.
Should there be a worker_getMutablePtr() here as well?
| gssize bytesCopied = packet_copyPayload( | ||
| nextPacket, 0, (PluginVirtualPtr){.val = buffer.val + offset}, copyLength); | ||
| if (bytesCopied < 0) { | ||
| // Error writing to PluginVirtualPtr | ||
| return bytesCopied; | ||
| } | ||
| totalCopied += bytesCopied; | ||
| remaining -= bytesCopied; | ||
| offset += bytesCopied; | ||
|
|
||
| Packet* packet = socket_removeFromInputBuffer((Socket*)tcp); |
There was a problem hiding this comment.
This code will keep the current packet, but discards the packets from previous while loop iterations. Maybe it would be better to if it's the first packet, keep it and return the error, but if it's not the first packet, skip/keep the current packet and return the number of bytes copied up until that point?
There was a problem hiding this comment.
Oh good point.
Otoh I worry that not returning an error here could end up hiding a bug. e.g. in the worst case the plugin has a read loop for which the first "packet size" of the buffer is valid memory, but the rest isn't, and instead of returning an error we cause it to only process one packet of data at a time.
Added a warning for now.
There was a problem hiding this comment.
I think it's good to have a message here, but having a TCP stream that loses data might be better as an error() rather than a warning() since your simulation might complete successfully without it being immediately obvious that there were issues. For example applications that detect this error (such as tor using TLS) would close the connection and I think re-open a new one later, and you might not notice this since tor wouldn't crash or exit early.
There was a problem hiding this comment.
I do think the warning should be an error() as mentioned above, or at least critical(), but I'll leave this up to you if it should be changed. Otherwise looks good!
Edit: I changed my mind. Any sane program should be closing the connection on an unexpected read error anyways, so I think it's good as-is.
Yeah, that's pretty much my thinking. I was also tempted to use |
These APIs are potentially more efficient than process_getReadablePluginPtr and process_getWriteablePluginPtr when the data must be copied to or from some other Shadow-owned buffer (such as a packet). The latter can fall back to thread_* when the MemoryManager is disabled or doesn't handle the given memory region. For thread_ptrace, thread_getXPluginPtr involves an extra allocation and copy to a buffer owned by ThreadPtrace, while thread_readPtr and thread_writePtr doesn't. These APIs also lend themselves to safe, clean Rust APIs. We should be able to build generically typed interfaces for reading and writing primitives, without having to deal with the lifetime issues we'd have to contend with if returning some kind of borrowed pointer.
To get the benefits of process_readPtr and process_writePtr, we need to pass virtual pointers deeper into the code. Having to also pass Process and Thread pointers in all such cases would be cumbersome. Instead we can extend the Worker to track the active thread and provide succinct wrappers for these APIs.
This mostly affects reading and writing network sockets, avoiding an allocation and extra copy inside *_getReadablePtr and *_getWriteablePtr when the MemoryManager is disabled or hasn't mapped the applicable memory.
These APIs are potentially more efficient than process_getReadablePluginPtr and process_getWriteablePluginPtr when the data must be copied to or from some other Shadow-owned buffer (such as a packet). The latter can fall back to thread_* when the MemoryManager is disabled or doesn't handle the given memory region.
For thread_ptrace, thread_getXPluginPtr involves an extra allocation and copy to a buffer owned by ThreadPtrace, while thread_readPtr and thread_writePtr doesn't.
These APIs also lend themselves to safe, clean Rust APIs. We should be able to build generically typed interfaces for reading and writing primitives, without having to deal with the lifetime issues we'd have to contend with if returning some kind of borrowed pointer.