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

[feature request] Allow set custom attributes on inner method #205

Open
moonheart opened this issue Mar 21, 2024 · 4 comments
Open

[feature request] Allow set custom attributes on inner method #205

moonheart opened this issue Mar 21, 2024 · 4 comments

Comments

@moonheart
Copy link

The problem:

When calling extern method, it may throw System.AccessViolationException, which can not be catched and will crashes the process.

To catch this exception (.NET Framework only), the caller must set two attributes on current method: HandleProcessCorruptedStateExceptionsAttribute and SecurityCriticalAttribute.

But the weaver did not copy these attributes to the generated innner method, and causes the process crash.

Possible solution

  1. Always copy these two attributes to inner method if exist
    This is the easist but not the best, because it takes extra steps to check attributes.
  2. Allow set custom attributes in user advices.
    Users can set attributes based on their logic.
@picrap picrap added the bug label Mar 25, 2024
@picrap
Copy link
Member

picrap commented Mar 25, 2024

The attributes are still there, I don’t understand why this happens.

@picrap picrap removed the bug label Mar 25, 2024
@moonheart
Copy link
Author

csproj file:

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net462</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
        <LangVersion>latest</LangVersion>
    </PropertyGroup>

    <ItemGroup>
        <PackageReference Include="MrAdvice" Version="2.15.0" />
    </ItemGroup>
</Project>

Program.cs:

using System.Runtime.ExceptionServices;
using System.Security;
using ArxOne.MrAdvice.Advice;

internal class Program
{
    public static void Main(string[] args)
    {
        TestMethod();
    }

    [Test]
    [HandleProcessCorruptedStateExceptions]
    [SecurityCritical]
    private static void TestMethod()
    {
        Console.WriteLine("May throw System.AccessViolationException here.");
    }
}

public class TestAttribute : Attribute, IMethodAdvice
{
    public void Advise(MethodAdviceContext context)
    {
        context.Proceed();
    }
}

Generateed Code (decompiled):

using ArxOne.MrAdvice;
using ArxOne.MrAdvice.Annotation;
using System;
using System.Runtime.ExceptionServices;
using System.Runtime.InteropServices;
using System.Security;

#nullable enable
internal class Program
{
  public static void Main(string[] args) => Program.TestMethod();

  [Test]
  [HandleProcessCorruptedStateExceptions]
  [SecurityCritical]
  private static void TestMethod()
  {
    // ISSUE: method reference
    // ISSUE: method reference
    // ISSUE: method reference
    ⚡Invocation.ProceedAspect(__methodref (Program.TestMethod), __methodref (Program.TestMethod′), __methodref (Program.TestMethod″));
  }

  [ExecutionPoint]
  private static void TestMethod()
  {
    Console.WriteLine("May throw System.AccessViolationException here.");
  }

  private static 
  #nullable disable
  object TestMethod([In] object obj0, [In] object[] obj1)
  {
    Program.TestMethod();
    return (object) null;
  }
}

As you can see, the generated method TestMethod′ is not marked with [SecurityCritical] and [HandleProcessCorruptedStateExceptions] attributes.

@picrap
Copy link
Member

picrap commented Jul 10, 2024

Sorry for huge delay.

I’ve been thinking to this problem and I don’t know how to solve this.
Let me rephrase your needs :

  • currently, MrAdvice weaves its advices inside the method, so the method attributes are left outside.
  • your needs are the opposite, as you want the advice to be applied outside.

So how to solve this ?

  • changing global behavior is a bad idea
  • having a mixed behavior would actually require two weavings : one inside (as it is now), one outside, as you request.

Adding custom attributes beside the [ExecutionPoint] at build could be an idea. Do you think you could go with that ?

@moonheart
Copy link
Author

In most cases, the current solution works well. However, the HandleProcessCorruptedStateExceptionsAttribute and SecurityCriticalAttribute are special cases. Therefore, I believe we should only copy these two attributes in AspectWeaver.WeaveAdvices. There is no need to copy all attributes besides [ExecutionPoint], as this could introduce breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants