Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InvalidProgramException when proxying MemoryStream with .NET 7 #651

Open
drauch opened this issue Feb 23, 2023 · 15 comments · May be fixed by #665
Open

InvalidProgramException when proxying MemoryStream with .NET 7 #651

drauch opened this issue Feb 23, 2023 · 15 comments · May be fixed by #665

Comments

@drauch
Copy link

drauch commented Feb 23, 2023

The following repro sample unfortunately fails with the .NET SDK 7.0.200 (it worked like a charm with .NET 6).
I'm using Castle.Core 5.1.1.

using System;
using System.IO;
using System.Text;
using System.Xml;
using System.Xml.Linq;
using Castle.DynamicProxy;

var xmlString = "<MyXml></MyXml>";
var stream = new MemoryStream(Encoding.UTF8.GetBytes(xmlString));

var proxyGenerator = new ProxyGenerator();
// THIS IS THE CULPRIT - COMMENTING LINE 12 MAKES IT WORK AGAIN
stream = proxyGenerator.CreateClassProxyWithTarget(stream, new MyInterceptor());

using var reader = XmlReader.Create(stream);
var xdoc = XDocument.Load(reader);
Console.WriteLine(xdoc.Root!.Name);

class MyInterceptor : IInterceptor
{
  public void Intercept (IInvocation invocation)
  {
    invocation.Proceed();
  }
}

The exception is:

Unhandled exception. System.InvalidProgramException: Common Language Runtime detected an invalid program.
at Castle.Proxies.MemoryStreamProxy.Read(Span`1 buffer)
at System.IO.Stream.ReadAtLeastCore(Span`1 buffer, Int32 minimumBytes, Boolean throwOnEndOfStream)
at System.IO.Stream.ReadAtLeast(Span`1 buffer, Int32 minimumBytes, Boolean throwOnEndOfStream)
at System.Xml.XmlTextReaderImpl.InitStreamInput(Uri baseUri, String baseUriStr, Stream stream, Byte[] bytes, Int32 byteCount, Encoding encoding)
at System.Xml.XmlTextReaderImpl.FinishInitStream()
at System.Xml.XmlReader.Create(Stream input)
at Program.<Main>$(String[] args) in C:\Projects\ReproInvalidProgramExceptionXmlReader\ReproInvalidProgramExceptionXmlReader\Program.cs:line 13

Please fix this soon, it prevents us from upgrading to .NET 7.

Best regards,
D.R.

@drauch
Copy link
Author

drauch commented Feb 23, 2023

Note: I just checked, even without any interceptors the program fails.

@drauch
Copy link
Author

drauch commented Feb 23, 2023

@stakx
Copy link
Member

stakx commented Feb 23, 2023

I haven't looked at the repro solution yet, but I suspect this will come down to .NET 7's MemoryStream having some method with a Span<> parameter (possibly used in combination with the C# in keyword). Span<> is a by-ref-like type, and those aren't (and in all likelihood cannot be) supported by DynamicProxy. The reason is that we have no way of transferring such arguments into the invocation's object[]-typed Arguments array (since that would mean that they can escape from the evaluation stack to longer-lived heap memory, thereby violating up the lifetime guarantees C# gives you for them).

@jonorossi
Copy link
Member

I don't think we've got any tests for Span, however we do for a ref struct. DP might still be missing something to ensure it's valid, however there are a lot of limitations with references because parameters need to go into the invocation object.

In general, I think proxying a MemoryStream just sounds like a bad idea.

@jonorossi
Copy link
Member

jonorossi commented Feb 23, 2023

@stakx we did it again. Only saw your comment as I clicked the green button 😆

@ulrichb
Copy link

ulrichb commented Feb 23, 2023

Note that also on pre .NET 7 ProxyGenerator wasn't be able to intercept methods with Spans.

The following program throws an "InvalidProgramException: Cannot create boxed ByRef-like values." on .NET 3.1, 5 and 6 ... and "InvalidProgramException: Common Language Runtime detected an invalid program." on .NET 7 (actually the old exception message was better 🤦).

var proxyGenerator = new ProxyGenerator();

using var stream = new MemoryStream(Encoding.UTF8.GetBytes("some text"));

var proxiedStream = proxyGenerator.CreateClassProxyWithTarget(stream, new MyInterceptor());

Span<byte> spanBuffer = stackalloc byte[20];
proxiedStream.Read(spanBuffer);

Console.WriteLine(Encoding.UTF8.GetString(spanBuffer));

class MyInterceptor : IInterceptor
{
    public void Intercept(IInvocation invocation) => invocation.Proceed();
}

I think the reason why @drauch 's repro worked in pre .NET 7 is that they probably switched from the byte-array overload to the Span<byte> overload of Stream.Read() at the call site (within XmlReader) in .NET 7.

@ulrichb
Copy link

ulrichb commented Feb 23, 2023

... yep:

image

@drauch
Copy link
Author

drauch commented Feb 23, 2023

@ulrichb Ah, thanks, that makes it clear why it fails in .NET 7 and didn't fail before.

@stakx @jonorossi So what you're saying is that we cannot use a proxy for a Stream at all basically?

It'd be unfortunate, because we have a "whatever you call on this stream instance, if an XyzException is thrown, translate it to AbcException"-necessity (in reality it's not a MemoryStream, I just used the MemoryStream for the repro sample). Our only other option is basically to write a decorator and decorate each method manually and make sure via a test that we really have all methods decorated :-/

Best regards,
D.R.

@stakx
Copy link
Member

stakx commented Feb 24, 2023

I'll take a look at our test suite. Right now it would appear that code generation succeeds despite the invalid attempt to box a Span<> (implying PEVerify doesn't catch that error).

I don't see any way to support spans, at this time I can think of a few workarounds / mitigations.

  • DynamicProxy could skip problematic arguments by simply substituting null in the invocation argument array.

  • We could perhaps add a configurable marshalling layer that translates between problematic types (like Span<> and types that can be safely boxed. Not sure how exactly that would work and whether it's worth the effort at this time.

@stakx
Copy link
Member

stakx commented Feb 24, 2023

what you're saying is that we cannot use a proxy for a Stream at all basically?

@drauch, I still haven't looked at the repro code, nor have I recently tried to create a Stream proxy object. The answer to your quesrion depends on when exactly the error occurs:

  • During proxy type generation: In that case yes, you probably cannot proxy streams with DynamicProxy.

  • During single calls to the generated proxy: In that case you can proxy streams, but some methods will be unusable.

@drauch
Copy link
Author

drauch commented Feb 24, 2023

It's a bit unfortunate, because I don't even touch the parameters in my IInterceptor, they only need to be passed on to the wrapped Stream. But I guess it's hard to integrate that in the existing API.

Thank you for you quick replies.

@ulrichb
Copy link

ulrichb commented Feb 24, 2023

Proposal: A new ProxyGenerationOptions.SignaturesWithSpans enum setting with the following options:

  • SignaturesWithSpans.Throw (default): Validates at proxy construction time that there is no method including a Span. (Better than the current behavior at method invocation time). Note that this is a breaking change.
  • SignaturesWithSpans.ThrowOnInvocation: Only for compatibility with the old behavior.
  • SignaturesWithSpans.ReplaceWithNull: As @stakx suggested above.
  • SignaturesWithSpans.BoxIntoArray: Copies the span into an array (in case the user needs the value and accepts the perf hit and mem allocation).

The last option is the most expensive one and therefore could be added later.

@stakx
Copy link
Member

stakx commented Feb 24, 2023

@ulrichb, understood. Note that ideally, any such new option should cover all problematic (by-ref-like?) types, not just Span<> (even though that is probably the most prominent example).

@markusschaber
Copy link

@ulrichb, understood. Note that ideally, any such new option should cover all problematic (by-ref-like?) types, not just Span<> (even though that is probably the most prominent example).

I agree that Span<> and ReadOnlySpan<> are probably the most common ref structs, and they're getting more and more common within the framework, so I think a workaround for those two would fix 90% of all cases...

(I came here as a FakeItEasy user, which uses castle under the hood.)

@stakx
Copy link
Member

stakx commented Sep 1, 2023

I've opened a new issue to discuss and plan support for by-ref-like types; see #663.

@stakx stakx changed the title ProxyGenerator for Stream not .NET7 compatible InvalidProgramException when proxying MemoryStream with .NET 7 Sep 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants