Skip to content

Conversation

@ds5678
Copy link
Contributor

@ds5678 ds5678 commented Oct 9, 2025

Problem

Resolves #3584

Solution

  • Any comments on the approach taken, its consistency with surrounding code, etc.
    • I'm not sure my changes to Box are semantically correct.
  • Which part of this PR is most in need of attention/improvement?
  • At least one test covering the code changed

@dgrunwald
Copy link
Member

dgrunwald commented Oct 11, 2025

  • Instructions.cs is generated from Instructions.tt; please don't modify the former manually. (instead, use CustomComputeFlags in the .tt and a partial class for the actual implementation)
  • SetChildInstruction already invalidates the flags; so there's no need for your added InvalidateFlags() call.
  • box not having any side-effects despite doing a heap-allocation, seems a bit questionable. But I don't see how it could cause any real issues (as the heap-allocation cannot be accessed without depending on the result of the box instruction, so incorrect reordering of heap-accesses still seems impossible).
  • I don't really like the InferType call in the low-level flag computation -- that seems like something that might risk infinite recursion. Maybe just restrict this to box T(default.value T)?

@dgrunwald
Copy link
Member

Or maybe we don't need the type comparison at all -- if it's OK to ignore the heap-allocation and heap-write in some cases, it might also be OK to do so in all cases?
The argument that no problematic reordering is possible because the heap allocation is not otherwise accessible, doesn't depend on Argument.Flags == None or the type comparison...

@ds5678
Copy link
Contributor Author

ds5678 commented Oct 20, 2025

I rebased, and CI is now failing, but I don't think that's my fault. It appears that CI has failed on every commit since #3594, though that might be a coincidence since I also saw github/codeql-action#3208 and github/codeql-action#3207 which claim that the release of .NET 10 RC2 caused the issue.

@siegfriedpammer
Copy link
Member

As long as the "Build ILSpy" action runs fine in debug and release there is no need to worry.

CodeQL has not once reported anything. @christophwille why do we have the action set up in the first place? I think, maybe it would make sense to just remove it completely?

@christophwille
Copy link
Member

The intent for it is to report nothing - it would be bad IF at some point it actually found some vulnerability.

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

Successfully merging this pull request may close these issues.

Ref locals from generic array access

4 participants