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

Suppress emitting of localsinit flag. (VS 16.8, .NET 5) #1738

Open
tannergooding opened this issue Jul 25, 2018 · 20 comments
Open

Suppress emitting of localsinit flag. (VS 16.8, .NET 5) #1738

tannergooding opened this issue Jul 25, 2018 · 20 comments

Comments

@tannergooding
Copy link
Member

@tannergooding tannergooding commented Jul 25, 2018

Adding SkipLocalsInitiAttribute to CoreCLR: dotnet/coreclr#20093

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Jul 25, 2018

CC. @jaredpar. I logged this issue to track: https://github.com/dotnet/csharplang/blob/master/proposals/skip-localsinit.md

I believe we need a new proposal champion to finish pushing this through the compiler.

@MrJul
Copy link

@MrJul MrJul commented Jul 26, 2018

Will the SkipLocalsInitAttribute be restricted to unsafe methods only? (either with a warning or the attribute simply being ignored on safe methods).

While reading the proposal, I assumed that there'll be some kind of restriction (because of potentially unverifiable code), but that's not stated explicitly.

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Jul 27, 2018

This is linked to the CoreFX API: https://github.com/dotnet/corefx/issues/29026

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Jul 27, 2018

@MrJul, not as proposed.

If the user adds the attribute to a method, they are opting into the behavior and into any "unverifiable" functionality.

NOTE: For most "safe" code, dropping localsinit is "safe" and can be made verifiable by updating the tooling to check that the local is written before first use. One exception would be for Span<T> x = stackalloc T[];, which is allowed in safe code but would not be verifiable.

@tannergooding tannergooding added this to the 8.0 candidate milestone Jul 27, 2018
@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Jul 27, 2018

This already has a prototype/implementation: https://github.com/dotnet/roslyn/tree/features/localsinit

The remaining work is just the general cleanup required to merge this back into master.

We should look at getting this done for 8.0, if possible. CC: @agocke, @jaredpar, @jcouv

@jkotas
Copy link
Member

@jkotas jkotas commented Jul 27, 2018

NOTE: For "safe" code, dropping localsinit is "safe" and can be made verifiable by updating the tooling to check that the local is written before first use.

Nit: This is not true for Span stackalloc.

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Jul 27, 2018

Ah right, Span<T> x = stackalloc T[] is allowed in safe code now. I've updated the comment about to indicate "most safe code" and to give the above as an example of where it is not.

@jcouv jcouv modified the milestones: 8.0 candidate, 8.X candidate Jul 18, 2019
@gafter gafter modified the milestones: 8.X candidate, 9.0 candidate Sep 16, 2019
@pentp
Copy link

@pentp pentp commented Sep 22, 2019

To keep safe code safe, it would help if the compiler disallowed Span stackalloc in methods under the scope of this attribute, unless the method is marked unsafe. This wouldn't break any existing code and should be a pretty simple check on the stackalloc expression.

@agocke
Copy link
Contributor

@agocke agocke commented Sep 23, 2019

@jkotas and I talked about this at length. The two reasons why I decided against that:

  1. The level of unsafety is not that high. Since you can only stackalloc unmanaged types and Span still provides bounds checking, you can't really break the type system, you can only end up with garbage data.

  2. There are actually a bunch of ways, even in pure safe code, to end up with garbage data. The most common one is using various interop/marshalling APIs, but recently CoreFX added ArrayPool, which doesn't clear the arrays when freeing. The same thing can happen through various Memory<T> manipulations.

Both of these things together made me think that this wasn't necessarily worth marking every method with stackalloc unsafe, but it was worth at least a small warning, which is why I changed the SkipLocalsInit attribute to require that /unsafe is passed to the compiler. This felt like a good middle ground, to me.

@GrabYourPitchforks
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks commented Apr 26, 2020

The level of unsafety is not that high. Since you can only stackalloc unmanaged types and Span still provides bounds checking, you can't really break the type system, you can only end up with garbage data.

I don't think that is 100% true. There are many unmanaged types that fall over dead in unexpected ways with you try bitblting random data into them: DateTime, decimal, Rune, Nullable<T>, StandardFormat, etc. All of these types have validating ctors that forbid initialization from garbage data. The ability to bypass the ctors and have the backing fields set to arbitrary bit patterns seems to defeat the purpose of the type system.

@Joe4evr
Copy link

@Joe4evr Joe4evr commented Apr 26, 2020

There are actually a bunch of ways, even in pure safe code, to end up with garbage data. The most common one is using various interop/marshalling APIs

Yeah, but all of those APIs should be marked with [CallerMustBeUnsafe] once that gets shipped an honored. 😛

@tannergooding
Copy link
Member Author

@tannergooding tannergooding commented Apr 26, 2020

Maybe we could also fix those types to not fall over dead. There are too many ways that simple structs can be otherwise broken.

@GrabYourPitchforks
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks commented Apr 26, 2020

I don't think we need to go quite that far. Recall that bool is likewise problematic when backed by bit patterns other than 0x00 or 0x01. And aside from some academic discussions around it nobody's seriously pushing for bool's behavior to change when backed by other bit patterns.

Certain types (bool, Rune) are resilient against struct tearing due to multithreaded access. But they're not resilient against somebody using reflection or unsafe code or [FieldOffset] nonsense to give them different backing values. And meh, whatever.

TBH I'm fine with the issue as proposed here. But I saw the sentiment (paraphrased) "unmanaged structs are assumed to be resilient to any arbitrary backing bit pattern" and wanted to point out that it's not a universal truth. As such I don't think it should serve as sole justification for a feature, either for this or for any other feature that might come in the future. If there's other justification for doing something then great. :)

@theunrepentantgeek
Copy link

@theunrepentantgeek theunrepentantgeek commented Apr 27, 2020

But I saw the sentiment (paraphrased) "unmanaged structs are assumed to be resilient to any arbitrary backing bit pattern

I'd go further and suggest that this is an actively dangerous assumption. If you monkey with the innards of a type in this fashion you're firmly in undefined behavior territory and you shouldn't be surprised if bad things happen. This is precisely the kind of manipulation used by malicious actors to bypass type safeguards in attempts to provoke remote code execution. #warrantyvoidifopened

@abelbraaksma
Copy link

@abelbraaksma abelbraaksma commented May 22, 2020

Apologies in case this shouldn't be the place to ask, but zeroing memory is also done outside the stack. This currently adds unnecessary overhead for arrays that are initialized immediately after declaration, leading to double initialization (first zeroing, then whatever the user wants).

While allowing arbitrary arrays to be pointing to arbitrary memory may be considered a security concern, it would greatly improve safe types like ImmutableArray that already guarantee initialization. Likewise, it could improve certain internals in FSharp.Core, like Array.create or Array.map. Each of those will first create a new array, same size, init to zero, and then init to the target value. Zeroing is fast, but still O(n), and esp in immutable scenarios, this is a significant cost.

Bottom line, this is great for locals, but could we extend it to support heap memory as well?

Related: #1037

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented May 22, 2020

Apologies in case this shouldn't be the place to ask, but zeroing memory is also done outside the stack. This currently adds unnecessary overhead for arrays that are initialized immediately after declaration

Yes, not the right place. I think this is still on track for .NET 5: dotnet/runtime#27146 - the discussion should be moved there.

@dgrunwald dgrunwald mentioned this issue Jul 25, 2020
76 of 108 tasks complete
@acple
Copy link

@acple acple commented Jul 30, 2020

Hi. In the current (5.0.100-preview.7.20366.6) compiler always emits SkipLocalsInitAttribute IL metadata to all targets I marked. However this attribute is only used for modifying init flag in IL. So I think these attribute metadata should not be added to IL, like MethodImplAttribute.
What are your thoughts?

@333fred
Copy link
Member

@333fred 333fred commented Jul 30, 2020

Hi. In the current (5.0.100-preview.7.20366.6) compiler always emits SkipLocalsInitAttribute IL metadata to all targets I marked. However this attribute is only used for modifying init flag in IL. So I think these attribute metadata should not be added to IL, like MethodImplAttribute.
What are your thoughts?

My thoughts are that we don't have a concept of source-only attributes today, and we shouldn't try to add them for a single type. There are certainly plenty of cases where they're useful, and I think source generators will increase that to the point we'll need to consider the. But not for a single attribute. #1452 is the source-only attributes proposal.

@jnm2
Copy link
Collaborator

@jnm2 jnm2 commented Jul 30, 2020

@333fred MethodImplAttribute, SerializableAttribute, StructLayoutAttribute, and others that control IL flags are not emitted as custom attributes, so why should SkipLocalsInitAttribute be?

Also, IndexerNameAttribute is an example of compiler-recognized attribute that doesn't target IL flags specifically and is not emitted as a custom attribute.

@HaloFour
Copy link
Contributor

@HaloFour HaloFour commented Jul 30, 2020

C# has plenty of "source-only attributes", aka pseudo-attributes that the compiler recognizes and emits as some other form of metadata. I think an attribute like SkipLocalsInitAttribute fits well into that paradigm.

IIRC the motivation behind the "source-only attributes" wasn't to add new attributes that aren't emitted as attributes but instead to allow attributes to be applied to more targets like locals, assignments or expressions.

@jcouv jcouv changed the title Suppress emitting of localsinit flag. Suppress emitting of localsinit flag. (VS 16.8, .NET 5) Sep 1, 2020
@MadsTorgersen MadsTorgersen removed this from the 9.0 candidate milestone Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet