Add support for fetching and submitting transactions over Tor #193
Conversation
| /// The two `TorRuntime`s will share internal state and configuration, but their streams | ||
| /// will never share circuits with one another. |
There was a problem hiding this comment.
Missed opportunity for a "don't cross the streams!" joke ;-)
| /// - `tor_runtime` must be non-null and point to a struct having the layout of | ||
| /// [`TorRuntime`]. |
There was a problem hiding this comment.
Is that sufficient? I don't think it would be ok to memcpy a TorRuntime and pass a pointer to the copy; I think it has to be a pointer returned by an API function with return type *mut TorRuntime (that has not subsequently been freed).
Same comment for all uses of the "point to a struct having the layout of" wording.
There was a problem hiding this comment.
The safety reqirements here are those necessary for the unsafe { tor_runtime.as_mut() } below to be safe. In particular, if non-null it must be convertible to a reference.
AFAICT it would be perfectly fine to memcpy a TorRuntime as long as you then discard the old one, which in Rust would be equivalent to a move. If you memcpy and then attempt to use both, that would be equivalent to impl Copy for TorRuntime which is disallowed because Arc does not implement Copy (because any copy cannot be purely bitwise due to also needing to update reference counters).
But that has no bearing on this method: as long as it can construct a valid &mut TorRuntime then it will work as long as the underlying TorRuntime has the correct layout and you don't try to use both copies simultaneously (as per the safety requirements). The problem you envisage specifically arises if you:
- Obtain a
*mut TorRuntime memcpyit in some way to obtain a second*mut TorRuntime.- Call
zcashlc_free_tor_runtimewith both, which will cause a double-free (or more likely other problems because thememcpyed one was not allocated with the Rust allocator).
So I think the only change I'd make here is independent of this PR: document on all zcashlc_free_* methods where their arguments must have come from.
There was a problem hiding this comment.
AFAICT it would be perfectly fine to memcpy a TorRuntime as long as you then discard the old one, which in Rust would be equivalent to a move.
Agreed: the copy operation itself is not UB; it's only using the copy that is unsafe. But I don't agree that you can free or discard the original and safely use the copy (including freeing the copy). That's not safe in general for opaque types. You can only safely use it via the pointer you were originally given.
Reason 1: If the structure (directly or indirectly) contains pointers into itself, those pointers would be invalidated by effectively moving it.
Reason 2: It would be UB to use with_exposed_provenance[_mut] to reconstruct a pointer into such a copied type, after only exposing a pointer into the original. (Regardless of whether the current code in the Swift SDK does this, it might need to do it in future, as the corresponding Android PR does.)
There was a problem hiding this comment.
Contrast with https://github.com/Electric-Coin-Company/zcash-light-client-ffi/pull/196/files#r1966356159 where the "must point to a struct having the layout of" wording is sufficient because we know that the struct is "plain old data". Here, the TorRuntime struct contains a tor::Client which we don't know anything about (without peering into its implementation for a given version of the tor crate).
| // SAFETY: We ensure unwind safety by: | ||
| // - using `*mut TorRuntime` and respecting mutability rules on the Swift side, to | ||
| // avoid observing the effects of a panic in another thread. | ||
| // - discarding the `TorRuntime` whenever we get an error that is due to a panic. |
There was a problem hiding this comment.
The safety documentation doesn't say that the caller has to discard the TorRuntime pointer on error.
Same comment for all similar unwind safety justifications.
There was a problem hiding this comment.
This safety documentation comes from the PR in which I added exchange rate fetching (#142). Unfortunately I can't find where I determined that this was the way to ensure unwind safety for *mut T. AFAICT the Swift caller is not discarding the TorRuntime in existing usage when getting an error that is due to a panic; I don't recall whether I meant here that tor_runtime is not used outside of catch_panic, or whether I did indeed intend for us to treat it that way in Swift and we're not doing so. The fact that I referenced Swift in the first point but not the second means I might have meant the former?
In any case, what we're doing here is the same as what we're doing everywhere else, so I've opened #194 for reviewing what we currently do and ensuring it is correct, or fixing it if not.
There was a problem hiding this comment.
AFAICT the Swift caller is not discarding the TorRuntime in existing usage when getting an error that is due to a panic [...]
Indeed, and I think that means it is not panic-safe. [Moved the rest of this comment to #194 so that we're not having the conversation in two places, but note that one of the possible solutions —the one that is less complicated to implement and more typesafe— requires changing the API.]
| let res = catch_panic(|| { | ||
| let tor_runtime = | ||
| unsafe { tor_runtime.as_mut() }.ok_or_else(|| anyhow!("A Tor runtime is required"))?; | ||
|
|
||
| let isolated_client = tor_runtime.isolated_client(); | ||
|
|
||
| Ok(Box::into_raw(Box::new(isolated_client))) | ||
| }); |
No description provided.