mgravell / Pipelines.Sockets.Unofficial

.NET managed sockets wrapper using the new "Pipelines" API
Other
412 stars 54 forks source link

Thread Safety Violation Warnings found by InferSharp static analysis tool #71

Closed udlose closed 1 year ago

udlose commented 1 year ago

I am using StackExchange.Redis v2.6.86 (which uses Pipelines.Sockets.Unofficial v2.2.2 as a dependency) in my .NET 7 API. I am using the new Microsoft Static Analysis Tool (InferSharp) to analyze my code as well as dependent packages and I found a few warnings about Thread Safety Violation (and one Null Dereference Warning):

/_/src/Pipelines.Sockets.Unofficial/Helpers.cs:160: error: Null Dereference
  `%0.Pipelines.Sockets.Unofficial.Helpers$<>c__DisplayClass4_0.failures` could be null (last assigned on line 146) and is dereferenced in the call to `Helpers.`.

/_/src/Pipelines.Sockets.Unofficial/StreamConnection.AsyncPipeStream.cs:100: warning: Thread Safety Violation
  Unprotected write. Non-private method `StreamConnection$AsyncPipeStream.Read(...)` indirectly writes to field `this.Pipelines.Sockets.Unofficial.StreamConnection$AsyncPipeStream._pendingRead` outside of synchronization.
 Reporting because another access to the same memory occurs on a background thread, although this access may not.

/_/src/Pipelines.Sockets.Unofficial/StreamConnection.AsyncPipeStream.cs:342: warning: Thread Safety Violation
  Unprotected write. Non-private method `StreamConnection$AsyncPipeStream.BeginRead(...)` indirectly writes to field `this.Pipelines.Sockets.Unofficial.StreamConnection$AsyncPipeStream._pendingRead` outside of synchronization.
 Reporting because this access may occur on a background thread.

/_/src/Pipelines.Sockets.Unofficial/StreamConnection.AsyncPipeStream.cs:382: warning: Thread Safety Violation
  Unprotected write. Non-private method `StreamConnection$AsyncPipeStream.EndRead(...)` indirectly writes to field `this.Pipelines.Sockets.Unofficial.StreamConnection$AsyncPipeStream._pendingRead` outside of synchronization.
 Reporting because this access may occur on a background thread.

/_/src/Pipelines.Sockets.Unofficial/StreamConnection.AsyncPipeStream.cs:483: warning: Thread Safety Violation
  Unprotected write. Non-private method `StreamConnection$AsyncPipeStream.ProcessDataFromAwaiterImpl()` indirectly writes to field `this.Pipelines.Sockets.Unofficial.StreamConnection$AsyncPipeStream._pendingRead` outside of synchronization.
 Reporting because this access may occur on a background thread.

/_/src/Pipelines.Sockets.Unofficial/StreamConnection.AsyncPipeStream.netcoreapp21.cs:32: warning: Thread Safety Violation
  Unprotected write. Non-private method `StreamConnection$AsyncPipeStream.Read(...)` indirectly writes to field `this.Pipelines.Sockets.Unofficial.StreamConnection$AsyncPipeStream._pendingRead` outside of synchronization.
 Reporting because this access may occur on a background thread.

/_/src/Pipelines.Sockets.Unofficial/StreamConnection.AsyncPipeStream.netcoreapp21.cs:40: warning: Thread Safety Violation
  Read/Write race. Non-private method `StreamConnection$AsyncPipeStream.Read(...)` indirectly reads with synchronization from `this.Pipelines.Sockets.Unofficial.StreamConnection$AsyncPipeStream._pendingRead`. Potentially races with unsynchronized write in method `StreamConnection$AsyncPipeStream.Read(...)`.
 Reporting because this access may occur on a background thread.
mgravell commented 1 year ago

By default I would be very unconcerned by these warnings - I strongly suspect they are all false positives, since there is no expectation of concurrent usage of overlapped multiple reads (that's simply not a scenario that Stream itself supports). I can try to take a look, but: do you have any reason to think these anything more than automated tool noise?

udlose commented 1 year ago

@mgravell I don't. I just wanted to alert you if there were issues. I can open a bug report with the InferSharp repo if you feel that is the case.

mgravell commented 1 year ago

The thing about these tools is that they can almost never be perfect or a substitute for inspection. False positives is the expected outcome, not a big as such. Sure you can try to minimize them, but that number can never be zero.