dotnet / roslyn-analyzers

MIT License
1.59k stars 465 forks source link

Warn if stackalloc would throw with a constant size at runtime #6232

Open alrz opened 6 years ago

alrz commented 6 years ago

e.g. Span<int> s = stackalloc int[int.MaxValue]

Similar to https://github.com/dotnet/roslyn/issues/8456 Relates to warning waves

jcouv commented 6 years ago

Tagging @VSadov to triage

svick commented 6 years ago

What is the maximum stack size the compiler is supposed to assume? Since the Thread constructor that lets you specify stack size takes an int, is it int.MaxValue bytes (i.e. 2 GB)?

E.g. stackalloc int[int.MaxValue / 2] would produce a warning, but stackalloc int[int.MaxValue / 4] or stackalloc byte[int.MaxValue] wouldn't?

And since this is just a warning, I assume that some future version of .Net allowed stacks larger than 2 GB is not an issue?

VSadov commented 6 years ago

The idea is that while we do not know how platform/OS handles the stack, we know that ‘localloc’ takes an int. So if the size does not fit in an int, it will certanly throw.

In reality the problems will start much earlier. Default stack limits are generally 1-4 mBs.

That makes it questionable that we would warn on outlandish sizes when anything more that couple kBs is dangerous.

svick commented 6 years ago

we know that ‘localloc’ takes an int. So if the size does not fit in an int, it will certanly throw.

As far as I can tell, that's not true. The spec says that localloc takes UIntPtr or uint. So there could be a .Net platform where stackalloc int[int.MaxValue] works fine.

tannergooding commented 6 years ago

Default stack size comes from the SizeOfStackReserve field in the PE Header.

Perhaps the warning could be tied to the default limit (1MB) and a compiler switch that allows the default to be changed could also exist (seems like a more generally useful thing with stack allocations being more readily possible in safe code anyways).

However, the warning should probably emit at percentage (maybe 50-75%) of the actual max stack, rather than at max stack itself. Even the main method has some stack taken away by locals/etc (and other method calls temporarily shrink the available space as well), so stackalloc int[maxStack - 1] will almost certainly trigger a runtime error and should be accounted for.