kniEngine / kni

KNI is a cross-platform C# game framework.
Other
98 stars 5 forks source link

zero-initialized `RenderTargetBinding` instances being accessed in `PlatformUnbindRenderTarget` #991

Closed willox closed 5 months ago

willox commented 5 months ago

I've seen the following stack traces from the main thread in a project my friend is working on:

Exception thrown: 'System.NullReferenceException' in MonoGame.Framework.dll
System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.Xna.Platform.Graphics.ConcreteGraphicsContextGL.PlatformUnbindRenderTarget(IRenderTargetStrategyGL renderTargetStrategy)
   at Microsoft.Xna.Platform.Graphics.ConcreteTexture.PlatformDeleteRenderTarget(IRenderTargetStrategyGL renderTargetGL, GraphicsContextStrategy contextStrategy)
   at Microsoft.Xna.Platform.Graphics.ConcreteRenderTarget2D.Dispose(Boolean disposing)
   at Microsoft.Xna.Platform.Graphics.GraphicsResourceStrategy.Dispose()
   at Microsoft.Xna.Framework.Graphics.GraphicsResource.Dispose(Boolean disposing)
   at Microsoft.Xna.Framework.Graphics.GraphicsResource.Dispose()
   ...

I looked in to it and noticed that the _currentRenderTargetBindings array is being cloned in a couple of places without using the value of _currentRenderTargetCount at all: https://github.com/kniEngine/kni/blob/caf0f1b14ee68bd266132acf29869a8b4888f842/MonoGame.Framework/Graphics/.GL/ConcreteGraphicsContext.cs#L947 https://github.com/kniEngine/kni/blob/caf0f1b14ee68bd266132acf29869a8b4888f842/MonoGame.Framework/Graphics/.GL/ConcreteGraphicsContext.cs#L1061

These cloned arrays are later accessed beyond the length _currentRenderTargetCount, causing the code to start accesing zero-initialized RenderTargetBindings which have null RenderTargets here: https://github.com/kniEngine/kni/blob/caf0f1b14ee68bd266132acf29869a8b4888f842/MonoGame.Framework/Graphics/.GL/ConcreteGraphicsContext.cs#L1088

Sadly I'm not able to provide a project that reproduces this bug since it's part of a complicated codebase and I don't understand the GraphicsContext code enough to know what specifically gets it in a state where _currentRenderTargetCount and the length of the _currentRenderTargetBindings array differ, but I think the fact that the count is being ignored when cloning these arrays maybe makes this a fairly obvious bug?

I wasn't really sure how to correctly compile or test the project, so I decided to just throw all of this information in an issue. Replacing the clones with _currentRenderTargetBindings.Take(_currentRenderTargetCount).ToArray() has fixed the issue for us locally.

Version

What operating system are you using:

What MonoGame platform are you using:

@Jvs34

nkast commented 5 months ago

I think what is missing, is a null check on binding.RenderTarget before calling binding.RenderTarget.GetTextureStrategy(...)

https://github.com/kniEngine/kni/blob/caf0f1b14ee68bd266132acf29869a8b4888f842/MonoGame.Framework/Graphics/.GL/ConcreteGraphicsContext.cs#L1090C25-L1090C45