Repository navigation
Support by-ref-like (ref struct) parameter types such as Span<T> and ReadOnlySpan<T> #663
Description
Activity
Scratch the above proposal, I think I can actually come up with one that works for arbitrary by-ref-like types. I'll post an updated feature proposal shortly.
I've put something together that appears to work... see the draft PR linked above (#664) for two short code examples how this would be used in practice. I'd be glad for some feedback (and whether the suggested API would actually be usable e. g. in downstream testing libraries).
/cc @thomaslevesque & @blairconrad (FakeItEasy), @dtchepak & @zvirja (NSubstitute), @jonorossi
Hey @stakx, nice job!
Using converters is an interesting approach. I think it would work just fine for mocking scenarios, where performance isn't typically a critical concern. In other scenarios, the performance impact of converting spans to arrays is something to keep in mind. I don't see a better way, though...
Something else to consider: there might not always be a sensible way of converting aref structto an object and still be able to convert back (I'm thinking ofUtf8JsonReader, for instance).
From FakeItEasy's perspective, I think the converters forSpan<T>/ReadOnlySpan<T>would be provided directly by the library, and we could expose extension points to provide additional converters.
BTW, is there a particular reason why converters don't implement an interface in your proposal?Reacted by Dominique SchuppliThanks for the feedback @thomaslevesque.
is there a particular reason why converters don't implement an interface in your proposal?
Yes, there is.
ref structtypes cannot be used as generic type arguments. So a proper interface type such asIByRefLikeConverter<TByRefLike>could never be instantiated.there might not always be a sensible way of converting a ref struct to an object and still be able to convert back
True. I don't see what we could do about that, though... do you? If I understand your concern correctly, this would make it only impossible to do round-tripping of
ref structvalues during interception. "Half-trips" should still work since theBox/Unboxmethods are only looked up & called when needed; in theory, if you don't need the conversion back fromobject(i.e. yourref structtype does not figure as a method return type or in aref/outparameter) you wouldn't even need to provide theUnboxconversion method.the performance impact of converting spans to arrays is something to keep in mind.
Indeed. Which is why the copying behavior shouldn't be the default behavior (for spans), and why I thought it would be a good idea for the converter selectors to pinpoint single method parameters, so you're free to choose to nullify most spans except where retaining the value somehow – e.g. as an array copy – is actually important. Being only able to set up a single converter globally that gets used for all by-ref-like parameters equally might be too coarse-grained.
I've also briefly considered doing weird
unsafepointer-based stuff (similar to whatSystem.Runtime.CompilerServices.Unsafedoes) instead of converters that simply convert to/fromobject, but apart from being unsure if it would've even worked, I don't think it would've resulted in a very nice, easy to understand API).Reacted by Thomas LevesqueThanks, @stakx. Slightly over my head, but in general, I think I understand the approach and the tradeoffs. Appreciate your work. Thanks for thinking of us to invite for comments.
The possible performance optimization by specifying the position of the parameter is interesting, and not something I would've thought of.
Just to make sure I understand everything, if I combine that power with a comment @thomaslevesque made earlier ("From FakeItEasy's perspective, I think the converters for
Span<T>/ReadOnlySpan<T>would be provided directly by the library"), then I expect FakeItEasy would end up providing a selector not unlike the sample one you had:// this will get invoked during proxy type generation: class ConverterSelector : IByRefLikeConverterSelector { public Type SelectConverterType(MethodInfo method, int parameterPosition, Type parameterType) { return parameterType == typeof(Span<byte>) ? typeof(CopyByteSpanConverter) : null; } }
where we'd end up ignoring the
parameterPosition.Reacted by Dominique Schuppli@blairconrad, I suppose that you'd typically configure a converter out-of-the-box that checks for both
Span<*>andReadOnlySpan<*>, and optionally lets user code opt into the conversion process for all other types. And yes, it should typically suffice to only check the type and ignore theMethodInfoand the parameter position.For mocking libraries, the selector's method body could look like this:
if (parameterType.IsGenericType) { var def = parameterType.GetGenericTypeDefinition(); var args = paramrterType.GetGenericArguments(); if (def == typeof(Span<>)) { return typeof(CopySpanConverter<>).MakeGenericType(args); } else if (def == typeof(ReadOnlySpan<>) { // as above but with a CopyReadOnlySpanConverter<> instead } } // for all other by-ref-like types, either // * delegate to user-provided selector function; or // * return null to let DynamicProxy nullify everything.
(Btw., I'm not 100% sure yet whether the
Boxmethod's parameter list is going to stay that way; it might also be possible to pass in aParameterInfowhich would tie those three parameters nicely together.)Reacted by Blair ConradYes, there is.
ref structtypes cannot be used as generic type arguments. So a proper interface type such asIByRefLikeConverter<TByRefLike>could never be instantiated.Ah, yes, of course. Thanks for the reply!
True. I don't see what we could do about that, though... do you?
No... I'm not even sure there is a solution. I'm just pointing out a limitation.
If I understand your concern correctly, this would make it only impossible to do round-tripping of
ref structvalues during interception. "Half-trips" should still work since theBox/Unboxmethods are only looked up & called when needed; in theory, if you don't need the conversion back fromobject(i.e. yourref structtype does not figure as a method return type or in aref/outparameter) you wouldn't even need to provide theUnboxconversion method.Yeah, half-trips would probably work in most cases.
I'm finally finding some time to resume my work on this.
Today I stumbled on the
System.Reflection.Pointerclass type. It may offer a way to transport by-ref-like values into and out of theIInvocation.Argumentsarray without any copying... see this small proof of concept. This involves someunsafepointer acrobatics, but I imagine it could be made reasonably safe for general use. Those pointers would have to be erased from theArgumentsarray at the end of the invocation pipeline in order to prevent access to expired (stack-allocated) by-ref-like values... that'd be important e. g. in async interception scenarios.As seeing
System.Reflection.Pointers in the invocation'sArgumentsarray would not be very intuitive, and most users probably wouldn't have a clue whether and/or how to manipulate them, this conversion probably shouldn't be made an automatic default. Instead, the conversion logic could be wrapped up in aByRefLikePointerConverterclass that's included in the library, users would then have to opt-into it by setting up aIByRefLikeConverterSelectormaking use of it.16 remaining items
Thanks for that latest repro. It is a nice demonstration how the current API allows you to casually circumvent C#'s ref safety rules. If an API involving
scopedcan indeed categorically prevent this, then it is definitely worth looking into.(Another approach would be to recommend interceptors be extra-careful in what they return, especially if they have no knowledge of what the calling code will be doing with the returned byref-likes. For instance, the interceptor could err on the safe side an allocate a copy of the incoming span's data on the heap, then return a span referring to that. But I hear you: such usage rule recommendation are much less elegant than being able to actually make the error impossible in the first place using a
scoped-based API.)I spent quite some time not understanding at all why it wouldn't work :D
I was rather shocked seeing you say that even such a simple use case didn't work. If that actually hadn't worked, I would've judged the whole feature an abject and total failure. 😁
Reacted by Oleksandr Povar@zvirja, I've given it some more thought. I am addressing several points, so apologies that this gets a bit long.
Suggestion 3
Better protect
ByRefLikeReference.GetPtr()or remove it all.I did originally start with an API based solely on managed pointers (
ref) andUnsafe.As{Pointer,Ref}like you suggested above, instead of using unmanaged pointers (*) and all the manual address-taking and pointer dereferencing. Admittedly that would have been much nicer and felt safer, too.Unfortunately, I soon ran into limitations with that approach, perhaps the major one being that it would have relied on
allows ref structanti-constraints in more places, which aren't available on .NET 8. So I also couldn't useUnsafe.As{Pointer,Ref}on that platform. Sincenet8.0is still one of this library's supported TFMs, I was looking for other ways to have as-decent-as-possible byref-like support there, too. That's how I ended up with the current unmanaged pointer-based current API.I am not claiming to have done an exhaustive research over all possible alternative designs, perhaps I missed a better one? But I just didn't see a way forward with something nicely-typed using
Unsafe.As{Pointer,Ref}.(As an aside, the current pointer API allows for fairly easy proxy code generation, which is somewhat important with respect to longer-term maintenance of DynamicProxy. Because this API is defined in the base class of the various substitute types
SpanReference<>,ReadOnlySpanReference<>etc., DynamicProxy can treat all of these exactly the same in most places except where they get instantiated; and even there the constructors all have the same signature, so DP only needs to figure out the most derived type to use for a particular parameter. I'm not sure that code generation could have been equally straightforward otherwise.)
Suggestion 1
I suggest to use
scopedkeyword to make sure there is no possibility to leak the value at all [...]As I see it, this suggestion (along with the proposed
UseValue) has definite merits, but also some potential drawbacks:- Benefit: It helps prevent ref safety mistakes in certain potentially dangerous situations (such as your recent example code above).
- Drawback: It might make it harder or more inefficient to use byref-like arguments in more harmless scenarios; say, when intercepting a method that only has "incoming" byref-like values but does not return byref-likes in any way at all.
UseValuewould force you to wrap parts or all of your actual interceptor code in a callback lambda without any actual benefit. In situations where performance is a concern, this may be undesirable. - Minor API design issue: Say you intercept a method with two or more byref-like parameters, and you want to access both/several of them simultaneously in your interceptor. You cannot simply nest
UseValuecallbacks inside one another because byref-likes cannot be captured (closed over) and then used "across a lambda boundary". We'd have to provide a whole bunch ofUseValueoverloads for varying numbers of parameters, similar to how there are many variants of theActionandFuncdelegate types. This is entirely feasible, but not ideal from a standpoint of API simplicity. - Drawback: Your
UseValueproposal appears to be motivated (at least in part) by the idea that making heap-allocated copies of spans is the easy and obvious way to prevent use-after-free errors, and thatUseValuein combination withscopedwould force users to make such copies in order to return values. And I partially agree. However, copies may not always be desirable. And not everything is a span. For some byref-like types, there might not be an easy way to copy them to the heap using something as simple asspan.ToArray. I fear that because of that, your proposed API might end up being too restrictive.
I am almost sure that if we release the current
ValueAPI into the wild, it won't be long before we get issues of the sort, "Why does this code explode in my face!?", with us having to analyze and explain use-after-free errors and ref safety violations... and I am definitely not looking forward to those.That being said, I still believe that safety should not trump everything else. I guess that's where we fundamentally differ: in the amount of safety DynamicProxy should guarantee. In my eyes, DynamicProxy is a low-level, mostly unopinionated tool with the primary goal of enabling scenarios (here: accessing and manipulating byref-like parameters). Its API should be designed in such a way as to steer/guide users towards safe usage, but not necessarily at the cost of preventing legitimate scenarios or efficiency. You're obviously assigning safety a higher priority than I do... which is perfectly fine and understandable.
Short intermezzo:
Suggestion 2
[...] Therefore I suggest to add another simple constrain - allow to access the argument from the original thread only.
That's a bit too paranoid for my personal taste. 😁 People accessing stack-only values across threads are basically begging for trouble and I'm not sure its DynamicProxy's responsibility to prevent something so obviously reckless. But yes, the
ByRefLikeReferencebase class could perhaps capture the thread and checking it upon access, without taking performance.Then again, I'm not sure how thread-safe
IInvocationimplementations are today, even without the new byref-like stuff. So perhaps thread safety would have to be considered more widely first, before we start patching single aspects.
Where could our different positions regarding the proper amount of safety guarantees meet?
Option 1: your API
Going with an alternative API identical or similar to the one you suggested, with all of its advantages and drawbacks.
Option 2: the current API, augmented with your API
We could add your
UseValuemethod as a "safe" choice for people who do not want to educate themselves on the intricacies of ref safety rules, but keep the current API around for "advanced" users.I was briefly thinking about replacing the read-write
Valueproperty with a pair of methods, which could then be namedUnsafeGetValueand/orUnsafeSetValue, but such a unsafe hint in the names would be imprecise because neither operation is by itself inherently unsafe; the unsafety results from how the methods are combined. So replacing or renaming the property would probably not have much benefit.Option 3: Better documentation
The current documentation completely overlooks dangerous scenarios where things can go wrong, like yours shown above. The current usage recommendations aren't sufficient, I think the documentation needs additional information about ref safety.
Let's quickly go back to your latest code example. I've noted that the method you're intercepting has a
scopedparameter:public interface ISomething { Span<int> Method(scoped Span<int> arg1); }
If it weren't for that
scoped, the C# compiler would have rejected yourInvokeMethodimplementation due to a potential ref safety violation. I don't have a perfect intuitive understanding ofscopedyet, but here,scopedcan be understood as a "promise" thatMethodwill not letarg1"escape" from it.I think it would be fair to expect interceptors to heed that promise, too. Perhaps we add a paragraph or two about interceptors having to be careful what they are returning, especially when the intercepted method promises not to leak
scopedparameters; and that interceptors should make defensive data copies if in doubt...?Option 4: Leave it up to downstream code (FakeItEasy, NSubstitute, etc.) to prevent unsafe scenarios
I think it's safe to say that most developers only ever come into contact with DynamicProxy indirectly, via the popular mocking libraries. Typically, these libraries have an API of their own and typically don't expose DynamicProxy's directly to the user. Therefore I think it's fair to expect that when it comes to byref-likes and ref safety, these libraries should share some of the responsibility for providing safety. For example, it would be entirely possible for a mocking library to perform span copying by default, and never expose
SpanReference<>etc. at all (at the cost of perhaps having no support for byref-like types other than spans).Option 5: scratch the current API
... and do something totally different, or nothing at all.
I personally am gravitating towards both options 3 (documentation) which I think is a must, and 4 (let mocking libraries provide additional safeties) which I think is a fair ask and might be a fitting split of responsibilities at different levels.
I'd be OK with 2 (my and your APIs as side-by-side alternatives). I think I am against 1 (just your API) as it seems to restrictive to me (see concerns outlined above), and I am definitely not very keen on 5 (start over again) but it's an option that someone else might want to look into.
Hey @stakx!
Thanks for the verbose and detailed analysis of my reply and for the possible options. I see your reasoning and would like to elaborate on it.
Safety
You are totally right to see that we have a very different view on the concern around safety and for me it's indeed a top priority. Unlike multi-threaded programming issues (like race-conditions), stack corruption is way more dangerous and is a totally different class of problems. If you mess up with stack, you can make stack unwalkable (if you somehow corrupt the return address), lead to peculiar GC behavior - not even mentioning bizzare computation results. The critical issue here is that runtime is not well-prepared for such kind of problems. Neither are most of the developers, even senior. It's not just a regular
NullReferenceExceptionwhich is handled pretty well - runtime could just start hallucinating. It's not a joke at all. Especially because it could happen very randomly, as you never know what people above a few levels of abstractions are doing. People who use it via some other third-party libraries might not have even a slightest idea what DynamicProxies and Interceptors are. The cost of troubleshooting it and frustration it could bring is just beyond imagination.Another aspect is that current API would violate .NET language philosophy in my opinion. Currently the runtime promise is that unless you mess up with
unsafeorP/Invoke, it is impossible to corrupt CLR no matter what you do. Here we are offering very much safe API which gives you means to do it. You can break CLR easily staring to deal withUnsafe.*or pointers - but then you have to be in theunsafecontext, so it's kind of obvious that you are a bit on your own.It would be careless to provide such a dangerous API so casually. We cannot just say "just read the documentation, we warned you" as again:
- nobody realistically reads the documentation in most of the cases - people just use the API available
- people might use code indirectly via other libraries
- code could be non-trivial, so it might be very tricky to spot the bug even if you know what you are doing
- code could evolve by refactorings, so it might be invisible that you introduce an issue
Offloading responsibility on the mock library developers also doesn't look good or realistic:
- if underlying abstraction is unsafe, the exposed API could be similarly unsafe. It could be transient.
- it might be not realistically possible to do analysis of what user is doing with the API to verify it's legit. Like I am a bit aware of
NSubstitutecode and I cannot imagine doing proper analysis to detect that users assign arguments to return values in a wrong way - if all the libraries have to build safety around unsafe API and API is dangerous to be used as-is, then it might be a good idea to provide better API in the first place.
The basic API shall be totally safe, even if it forces you to have overheads and inconveniences. We could provide means for optimizations, but that API shall not be the primary one if it's unsafe.
In my eyes, DynamicProxy is a low-level, mostly unopinionated tool with the primary goal of enabling scenarios (here: accessing and manipulating byref-like parameters). Its API should be designed in such a way as to steer/guide users towards safe usage, but not necessarily at the cost of preventing legitimate scenarios or efficiency.
We come a bit into design philosophy, but speaking generically would say that if a certain feature has a risk of stack corruption by using it wrongly, then I would probably go as far as saying that we shall refuse to admit that the feature is possible at all. Because runtime safety is not something you negotiate and take optionally - it's a hard constrain. You could for example create a library which allows you to hot-swap two methods my messing up with MethodTable structures - so when function is invoking one method, it invokes a substitute implementation instead. It's a powerful feature - you can use it for diagnostics, to log third-party methods and so on. Very handy. But the risks are above the roof and what about inlining, tiered-compilation and so on. I've tired doing it myself, it works and code is simpler than the one you wrote here for this feature. But just because it is doable - it's not enough to say that it's something to consider even as an option.
DynamicProxy is indeed a low-level tool. But so far you never had a risk of corrupting runtime by using some features without enough care. The biggest risk was a weird exception when you used a language feature which wasn't supported yet. So it would be a quality change in the nature of the provided API.
Okay, I feel I am boring you at this point :) I guess you got my point that IMO it's not just a risk - it's a critical risk.
Thread Safety
Checking for thread costs nothing, but it will prevent weird behavior and stack corruption. Of course it's stupid to access it from another thread. But again - you never know what happens above the layers of abstractions and how complex the code is.
Stack corruption and concurrency issues - are very different issues. So we cannot use non-thread-safety of
IInterceptorAPI as an argument - it's a totally different class of the issues.It's like .NET implements finalizers for handles wrappers. You shall in principle dispose things you own. But they would rather play safe to not leak native resources. Even though they mention in the documentation that you shall dispose things. And there is a compiler warning (if I am not mistaken).
Inefficiency
Minor API design issue: Say you intercept a method with two or more byref-like parameters, and you want to access both/several of them simultaneously in your interceptor.
... We'd have to provide a whole bunch of UseValue overloads for varying numbers of parameters, similar to how there are many variants of the Action and Func delegate types. This is entirely feasible, but not ideal from a standpoint of API simplicity. ...
However, copies may not always be desirable. And not everything is a span. For some byref-like types, there might not be an easy way to copy them to the heap using something as simple as span.ToArray. I fear that because of that, your proposed API might end up being too restrictive.
So far we are building the first version and enabling the scenario which was not possible at all. We don't know how API is going to be used and whether it would be a real concern. People might be totally OK with it if they are explained that otherwise there could be a huge risk of runtime integrity issues. A lot of code is just for testing and there it's totally OK to have not the most effective code.
Some things like accessing multiple arguments at the same time might be not a real issue due to the way how mock libraries expose the API on their level. It's hard to guess.
I wouldn't be worried about it ahead of time. I would just provide the first version of API and look for the feedback. If such a concern comes, we could always add more API and think more about it later.
I am very much a performance guy myself, so I perfectly relate to you and see where you are coming from. It's just hard to understand if the concern is real. Consumers might just prefer safety. But nevertheless I still have an idea how to satisfy it.
Options feedback
Option 3: Better documentation
It is not a solution at all in my opinion. We cannot rely that realistically people will read it. And those who read might not fully understand all the cases it concerns. Sure, maybe AI will. But it's just more like an apology to me rather than something real.
Option 4: Leave it up to downstream code (FakeItEasy, NSubstitute, etc.) to prevent unsafe scenarios
Be kind to your fellow brothers (yourself included) :D Now we rely that 3+ mocking libraries properly analyze the domain, understand all the implications and scenarios (I am not even sure about myself; I found a couple of simple repro cases, but I am pretty sure there are more exotic ones) - and correctly handle the concerns. Each has to take the responsibility (without maybe even fully realizing it) that wrong implementation could lead to stack corruption and C-world fun in managed runtime. I would rather not smear the problem everywhere, as then chances of making it wrong approach 100%.
Suggested solution
I suggest slightly modified hybrid of Option 1 and Option 2.
Option 1
Just go with providing safe (and limiting) API only and wait for the feedback. If there would be feedback that API is not enough - then look at the scenarios and add extra unsafe API.
Option 2
This option is a nice of compromise which shall cover your concerns. Let's follow the design of the
CollectionsMarshal. The idea is that main collections don't expose unsafe API, but you can use some other class to have features with higher risk.- I suggest to only have
UseValue/SetValueAPI on the wrappers - Introduce
ByRefLikeReferenceUnsafe/ByRefLikeReferenceMarshalwhich allows to readref structwithoutscopedprotection:- still keep Dispose protection
- still keep the origin thread protection
- make methods/class
unsafeto require usage ofunsafecontext.
This way:
- You cannot corrupt the stack if use just primary API
- If you need optimizations, you still could do it. But then you enter the
unsafecode territory - You explicitly enter
unsafecode which is an extra guard that you are doing something risky - The API exists "on a side", so it's not immediately discoverable. You only look for it if you need it (or read the documentation) and then you probably know what you are doing
I would say that code must be unsafe when you have unprotected ref struct. Just like pointers - there is nothing wrong with using them. But you can easily shoot yourself in a leg if you overlook something. Same here. It's mostly about risks, not about what you do per se.
I think it's a beautiful compromise. As long as you use safe API, we promise that there are no issues in a price of inconveniences. There is unsafe API on a side to enable writing more efficient code, but it imposes risks and requires you to be more careful, just like
Unsafe.*.Option 3. Bonus
Apparently we already could use
UnsafeAPI to bypassscopedprotection (as long as you addallow ref structanti-constrain toUseValue) 😳😅var arg1Unprotected = arg1.UseValue((scoped value) => Unsafe.AsRef<Span<int>>(Unsafe.AsPointer(ref value))); var arg1Unprotected = arg1.UseValue((scoped value) => Unsafe.Read<Span<int>>(Unsafe.AsPointer(ref value))); var arg1Unprotected = arg1.UseValue((scoped value) => Unsafe.As<Span<int>, Span<int>>(ref value));
Maybe there is something else. All of it nicely bypasses the
scopedkeyword, so you are free to do what you want afterwards. It looks a bit scary - but it is risky to do it, so it is a "feature" in a way.The fact that such API exists and is easily available advocates to go with Option 1:
- there is a performance/convenience workaround
- we could always add API for Option 2 if we find it's justified
But I do see that it's not very elegant and a bit cumbersome. It's just that it's possible. Marshal API would look nicer than this of course.
P.S. Better protect ByRefLikeReference.GetPtr() or remove it all.
I feel like we have a bit of misunderstanding. Or I have. Could you explain how exactly you use
.GetPtr()API? My comment was not about class hierarchy orByRefLikeReferencetype itself - it was only about that single specific method. My point was that if you keep data on stack, then it looks like you don't need that API at all. Because then you could simply look at the data on stack.Your reply has a different gist (or maybe I read it this way), so let's align first.
Thanks
Thanks for your time and patience @stakx! It's a nice discussion and I have a lot of fun 😊
It's very late and I spent around 2.5 hours writing this. I apologize in advance if it's a bit repetitive - I don't have more energy to tidy it up.
@zvirja, thanks for the time and thoughts you're putting into this, I appreciate it. I'm going to answer more fully later, right now I'm a little sleepy, so just something quick.
Suppose we do it your way and change the API of e. g.
SpanReference<T>as follows:-public ref Span<T> Value +private ref Span<T> Value { get { return ref *(Span<T>*)GetPtrNocheck(); } } + +public TResult UseValue<TResult>(ValueConsumer<TResult> consumer) +{ + return consumer(Value); +} + +public void UseValue(ValueConsumer consumer) +{ + consumer(Value); +} + +public void SetValue(Span<T> value) +{ + Value = value; +} + +public delegate void ValueConsumer(scoped Span<T> value); + +public delegate TResult ValueConsumer<TResult>(scoped Span<T> value);
What are we going to do about the following?:
public interface IFoo { Span<char> Method(); } public class BadInterceptor : IInterceptor { public void Intercept(IInvocation invocation) { Span<char> chars = stackalloc char[10]; "Hi".CopyTo(chars); var returnValueRef = (SpanReference<char>)invocation.ReturnValue!; returnValueRef.SetValue(chars); } }
Or, a little more involved, but closer to your previous example:
Span<char> InvokeMethod() { Span<char> chars = stackalloc char[10]; "Hi".CopyTo(chars); var generator = new ProxyGenerator(); var foo = generator.CreateInterfaceProxyWithoutTarget<IFoo>(new BadInterceptor()); return foo.Method(chars); } public interface IFoo { Span<char> Method(scoped Span<char> arg); } public class BadInterceptor : IInterceptor { public void Intercept(IInvocation invocation) { var argRef = (SpanReference<char>)invocation.Arguments[0]!; var returnValueRef = (SpanReference<char>)invocation.ReturnValue!; argRef.UseValue((scoped Span<char> arg) => { returnValueRef.SetValue(arg); }); } }
@stakx Amazing cases!!! 🤯🤩 It changes a lot my mental model of the problem and are nice examples indeed that the API I suggested are not fully safe either.
Let me play with it for a tiny bit. So far I have 2 workable ideas, but I am curious to see if I could find something more elegant :)
@zvirja, I propose a new compromise: we initially get rid of any means to set outbound byref-like-typed parameters. (DynamicProxy will write
default(TByRef)where necessary.) A value getter on its own should not be able to cause any stack corruption, and the checks/invalidations that are already in place should sufficently guarantee that one cannot read byref-like parameters that have gone out of scope.Specifically, I propose to replace the
ref TByRefLike Value { get; }property with aTByRefLike GetValue()method. Note the absence ofrefin the return type, and the absence of a setter method.Then we wait and see if this read-only solution is actually sufficient for most real-world use cases.
- If so: great, we no longer have a safety problem!
- If not: only then do we consider adding back a setter, but we would call it
void UnsafeSetValue(TByRefLike)to draw very obvious attention to the potential unsafety, and link it to more detailed documentation about the risks (including concrete examples) of using this method incorrectly.
Regarding other, less pressing points:
- I think we should indeed throw
InvalidOperationExceptioninstead ofAccessViolationException. I'll make the change. - I don't think checking for cross-thread access is truly necessary esp. when it isn't possible to set byref-like parameters; the existing safety measures should already suffice.
Good evening, @stakx! Sorry for a bit belated reply. I took a bit of time to research on this, to play with it and to let it settle inside myself to see if I come up with something better. Nothing better comes, so I'll just share what I found and my thoughts. Let's see if we could advance the story together.
I would like to immediately say that this story quickly becomes messy, as there is no elegant solution. There are good enough solutions, even though they are a bit weird at the first glance. Part of me feels a bit sad that I didn't manage to come up with something extremely neat. I feel a bit of pressure, as I feel that I am a bit alone on this "safety" side and at the moments of despair I start to have doubts as well 😅 But I still believe that we shall not expose things which could lead to stack corruption and as long as it's technically possible to do that, I would suggest to stick to that even if API is a bit weird.
I do like a lot your approach of phased API exposure. This way we indeed could ship basic version of the feature and only care about more advanced scenarios if the need comes. I could easily see it happening that we might need to reshape API anyway as more people would start consuming it.
I would first start with answering to your points and later will describe my analysis and ideas. Bear with me. Writing this reply was quite a journey :)
Feedback on your proposals
TByRefLike GetValue()-only methodIt took me a bit of time to assess if this API is actually safe. My initial idea of breaking it would be to emulate something like:
public ref struct Container { public ref int Value; } public interface ISomething { void Method(scoped Span<int> arg1, ref Container arg2) { arg2.Value = ref arg1[0]; } }
It however is not possible, as you will never return original
arg2instance, only a copy of the struct arg. If there is no other way how we could modify the original argument and carry the value out, then it shall be safe indeed.But it is only safe if we never return values. If we want to support returning values, then API causes troubles and doesn't allow to build safe API. I'll elaborate on it below.
I personally think that it's a big limitation, as then basically it's impossible to set
ref structreturn values and those are used quite a lot. I would suggest to explore writing API as well. I think it would be fine if it supports basic scenarios only - but it shall be possible to return the value. It will also allow mocking libraries to create the full feature.Thread-safety
I don't think checking for cross-thread access is truly necessary esp. when it isn't possible to set byref-like parameters; the existing safety measures should already suffice.
It is, as we could then have scenario like this:
Code
using Castle.DynamicProxy; namespace PointersPlayground; public static class Program { private class HandlerInterceptor(Action<IInvocation> handler) : IInterceptor { public void Intercept(IInvocation invocation) => handler(invocation); } public interface ISomething { void Method(Span<int> arg1); } internal static void Main(string[] args) { var mre1 = new ManualResetEventSlim(false); var mre2 = new ManualResetEventSlim(false); var mre3 = new ManualResetEventSlim(false); var proxyGenerator = new ProxyGenerator(); var proxy = proxyGenerator.CreateInterfaceProxyWithoutTarget<ISomething>(new HandlerInterceptor(invocation => { var arg1 = (SpanReference<int>)invocation.Arguments[0]!; Task.Run(() => { var arg1ValueCopy = arg1.GetValue(); mre1.Set(); mre2.Wait(); Console.WriteLine($"Concurrent Arg 1: {arg1ValueCopy[0]}"); mre3.Set(); }); Console.WriteLine($"Arg 1: {arg1.GetValue()[0]}"); mre1.Wait(); })); InvokeMethod(proxy); Span<int> data = stackalloc[] { 1, 5, 5, 7, 8, 9, 5, 44, 3, 2 }; mre2.Set(); mre3.Wait(); } private static void InvokeMethod(ISomething proxy) { Span<int> arg1 = stackalloc[] { 42 }; proxy.Method(arg1); } private static TByRef GetValue<TByRef>(this ByRefLikeReference<TByRef> value) where TByRef : struct, allows ref struct => value.Value; }
Output:
Arg 1: 42 Concurrent Arg 1: -1002968512My analysis
GetValue()scoped leakageYou gave me a truly amazing scenario with leaking stack-allocated data and it changed a bit the way how I was thinking about the problem. I started to consider whether the issue is with returns only, or whether there is another way of misusing and leaking the argument if it's not protected by
scopedconstrain.I found the following sources of "leaking" the scoped value:
returnref/out- interior mutability of another ref-struct argument
I haven't realized that we in fact could return argument value copies everywhere, even if it's
ref Span<int>. If so, then leaking through the interior mutability shall not be possible (becauseRefStructcannot haveref RefStructfield). It took me some time to try to break it with the analysis above and explore it. I haven't found a way. That in theory significantly simplifies the issue, as then the problem is narrowed to not leaking scoped values viaSetValue()(as that API is used both forreturnand forref/out).If we solve the
SetValue()problem, then we could return scoped arguments unprotected (i.e. withoutscopedconstrain) - there is no way to leak them otherwise. The issue, unfortunately, is thatSetValue()now becomes trickier, as it shall protect not only thescoped ref struct, but basically anyref struct- while you still want to somehow craft the result. I was not able to find any implementation that would work.SetValue() scoped leakage
We need to make sure that
scopedvariables are not leaked. Like:- prevent leaking local
stackallocvariables - prevent leaking
scopedarguments
Lambda-based approach
My initial intuition was to leverage the limitation you already spotted above that
ref structcannot be captured in lambda. We alsoreturnthe value there, so compiler shall protect thescopedvariable. Then the solution would be quite nice and neat:public unsafe class ByRefLikeReference<TByRefLike> { public void SetValue(ValueGetter valueGetter) { Value = valueGetter.Invoke(); } // nit: Func<> might work as well - I was playing with API :) public delegate TByRefLike ValueGetter(); } var returnArg = (SpanReference<int>)invocation.ReturnValue!; returnArg.SetValue(() => new[] { 42 });
Yes, it might look a bit cumbersome - but then it's totally safe if all the scoped arguments are
scopedprotected. We basically return back the compiler check and could trust it doing its job.The issue comes if we have
TByRefLike GetValue()API which stripsscopedmodifier from the argument. As then it's totally possible to write the following and we are back to square one:invocation => { var arg1 = (SpanReference<int>)invocation.Arguments[0]!; var returnArg = (SpanReference<int>)invocation.ReturnValue!; returnArg.SetValue(() => arg1.GetValue()); }
So this solution is totally safe only if we have scoped args protected.
Idea - ref struct based approach
Another idea I explored was API like
arg1.ValueAcessor.SetValue(TByRefLike value). IfValueAccessorisref struct, then compiler would not allow you to give it scoped value.While it works, I am not sure how strong the guarantee is in this case. To me it sounds like it currently works because of flow analysis limitation and in the future they could enhance it. As in principle compiler could conclude that
ValueAccessorcannot possibly outlive the current stack frame, so it's safe to accept scoped value.My knowledge is not strong enough to see it fully. Given that API is not elegant anyway, I decided that lambda-based approach is better.
Notice, this approach also provides total safety only if we don't strip
scopedprotection from the argument.Scoped argument awareness
Given that it is possible to have totally safe
SetValue()only ifscopedis respected, we need to preserve it and cannot strip it. I was considering what are the options.Simple one is overprotection. We could provide
UseValue()API only, so all the values arescoped-protected. It's overkill for 99% of the cases, but it's safe.Second option would be to introduce library awareness of scoped arguments. Given that
scopedarguments are used rarely, we could then provide more convenient API for the regular arguments without trading-off safety.First, we shall extend parent class with a flag
public unsafe class ByRefLikeReference { public bool IsScoped { get; } }
Then it's fairly easy to populate the value in the dynamic proxy:
// be aware of `scoped ref Span<int>` - value itself is not scoped in that case var isScoped = parameters[i].GetCustomAttribute<ScopedRefAttribute>() != null && !parameters[i].IsByRef; method.CodeBuilder.AddStatement( new AssignStatement( reference, new NewInstanceExpression( referenceCtor, new TypeTokenExpression(dereferencedArgumentType), new AddressOfExpression(dereferencedArgument), new LiteralBoolExpression(isScoped))));
For the return values it's always non-scoped.
If we have this awareness, then we could have API like this:
public void UseValue(ValueConsumer consumer) { consumer(Value); } public TResult UseValue<TResult>(ValueConsumerWithResult<TResult> consumer) where TResult : allows ref struct { return consumer(Value); } public TByRefLike GetValue() { if (this.IsScoped) { throw new InvalidOperationException("Use UseValue method for scoped arguments"); } return Value; } public void SetValue(ValueGetter valueGetter) { Value = valueGetter.Invoke(); }
For 99% of the cases we could just use
arg.GetValue()without paying price ofUseValue()limitations. If you have scoped argument - then you would be forced to use the method.Suggested solution
I suggest to:
- Add thread-safety checks
- Introduce library awareness around
scopedarguments - it's quite trivial - Provide
GetValue()/UseValue()with constrain that for scoped arguments it's only possible to useUseValue() - Provide lambda-based
SetValue()API - [optional] Provide
ByRefLikeReferenceUnsafeunsafe API to access scoped arguments/set arbitrary value without protection. I would hide it from direct API. Maybe not add it at all, unless we find a need for it.- If you introduce this type, we could then move there all the unsafe API instead of just hiding it from intellisense like you do it now. Just make it internal and expose via
ByRefLikeReferenceUnsafe:void* GetPtr(Type checkType)Invalidate(void* checkPtr)- contructors
This way API will be cleaner and you will have better control over activation
- If you introduce this type, we could then move there all the unsafe API instead of just hiding it from intellisense like you do it now. Just make it internal and expose via
Unless I am missing something, it shall enable all the possible scenarios with total safety:
- You can return heap-based ref struct value
- You can return another argument (for
scopedargit will throw):invocation => { var arg1 = (SpanReference<int>)invocation.Arguments[0]!; var returnArg = (SpanReference<int>)invocation.ReturnValue!; returnArg.SetValue(() => arg1.GetValue()); })
- You cannot return local variables
- You cannot return scoped arguments
I like that API is safe, quite trivial (except unusual
SetValue()) and for non-scoped arguments it's extremely convenient. If there are some limitations, we could address them in V2. But so far it looks like all the scenarios are covered.Another big pros is that
scopedwill never be stripped, so we will have proper compiler check for all other possible leakage scenarios we haven't considered.What do you think?
P.S. After doing this analysis I am even more confident that we shall ship safe API only. Analyzing it all was quite a hell and one cannot expect normal human to be aware of all of this 🤯 If API is dangerous, one will definitely shoot those legs one day or another.
Apologies for the delay @zvirja. I'll see if I can look into this on the weekend and reply to your latest ideas and suggestions then.
@stakx No worries! I saw your idea to release it as soon as possible and I do believe that we shall incorporate safety for
ref structin the very first release. That's better than deprecating it all later and changing the surface. I would be more than happy to prepare PR myself, so you could later do the follow up. That shall reduce the pressure from you and distribute the load a bit 😉I'll try to look in the coming day/s and build it.
I did say "as soon as possible" but I don't intend to completely sacrifice this ongoing discussion for that, so don't worry. On the other hand, I do hope we'll soon reach a point where we can ship something reasonably safe. I'd also prefer shipping an incomplete solution rather than one we'll have to go back on.
(I still don't believe that 100% ref safety will be achievable btw.)
@stakx I got a bit busy these days, but would be happy to work on it on Monday if you need some time to enjoy spring. Would feel happy to help!
@zvirja, I'm a little late too but let me take a more detailed look at your post above first, I'll get back to you.
Is there still ongoing work on this topic or is future ref support now gone?
Best regards,
D.R.No, not gone, I just need to find a quiet moment to finalize the pending ref safety concerns. I haven't forgotten about it, I'm just a bit short on time.
An approach to address ref safety was suggested above though it seems somewhat too complex IMHO, and I'd prefer to keep the API as simple as possible. For me, the likelier way to address the safety concerns would be to scale back the current byref-like support to a pure read-only interface, for starters... and only add back write support if and when people actually demand it. Admittedly, that only delays having to deal with the ref safety Pandora's box to some point in the future, but that's probably still better than not releasing anything at all.
Apologies for the wait, in either case.
Reacted by Frulfump
DynamicProxy does not currently support those because by-ref-like values cannot be boxed to be put in the
object[] IInvocation.Argumentsarray.I haven't yet been able to think of a general way how all by-ref-like types (including user-defined ones) could be supported, ecause of their various limitations. If anyone has ideas, I'd be interested in hearing them!However, it might be fairly easy to at least add support forSpan<T>andReadOnlySpan<T>specifically. Those types are becoming more and more common in the .NET FCL, so while not an ideal solution, it might still be sufficient for most use cases:We could introduce a new type in DynamicProxy's namespace:Then we could introduce a new property toProxyGenerationOptions,ISpanMarshaller SpanMarshaller { get; set; }, which would get injected into generated proxies so they could use it to transfer spans into and out of theobject[] IInvocation.Argumentsarray.I don't really like an addition that deals with two very specific types, but they are becoming more and more common, and I haven't yet been able to come up with a more general mechanism.Update
See the PR linked further down for an updated proposal that supports arbitrary by-ref-like types, including user-defined ones.
Opinions, or alternate ideas, anyone?