castleproject / Core

Castle Core, including Castle DynamicProxy, Logging Services and DictionaryAdapter
http://www.castleproject.org/
Other
2.2k stars 467 forks source link

`InvalidProgramException` when proxying `MemoryStream` with .NET 7 #651

Open drauch opened 1 year ago

drauch commented 1 year ago

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 commented 1 year ago

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

drauch commented 1 year ago

Here is the full repro VS 2022 solution: https://www.dropbox.com/s/4evu8rm636jv8h8/ReproInvalidProgramExceptionXmlReader.zip?dl=0

stakx commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

ulrichb commented 1 year ago

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 commented 1 year ago

... yep:

image

drauch commented 1 year ago

@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 commented 1 year ago

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.

stakx commented 1 year ago

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:

drauch commented 1 year ago

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 commented 1 year ago

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

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

stakx commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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