microsoft / infersharp

Infer# is an interprocedural and scalable static code analyzer for C#. Via the capabilities of Facebook's Infer, this tool detects null dereferences, resource leaks, and thread-safety violations. It also performs taint flow tracking to detect critical security vulnerabilities like SQL injections.
MIT License
730 stars 29 forks source link

Resource leak does not display leaked resource type. #92

Closed Kuinox closed 2 years ago

Kuinox commented 2 years ago

I'm trying to run InferSharp, but the error does not display the leaked ressources, only it's location, which make it hard to find the actual leak:

C:\dev\CK-MQTT\CK.MQTT.Common\BasicImplementations\MemoryStoreFactory.cs:12: error: Dotnet Resource Leak
  Leaked { n$8 -> 1 } resource(s) in method "ValueTask`1<!0> MemoryStoreFactory.CreateAsync(IActivityMonitor,ProtocolConfiguration,MqttConfigurationBase,String,Boolean)" at type(s) CK.MQTT.MemoryPacketStore.

C:\dev\CK-MQTT\CK.MQTT.Common\BasicImplementations\TcpChannelFactory.cs:18: error: Dotnet Resource Leak
  Leaked { n$10 -> 1 } resource(s) in method "ValueTask`1<!0> TcpChannelFactory.CreateAsync(IActivityMonitor,String)" at type(s) CK.MQTT.TcpChannel.

C:\dev\CK-MQTT\CK.MQTT.Common\BasicImplementations\MemoryStoreFactory.cs:27: error: Dotnet Resource Leak
  Leaked { n$8 -> 1 } resource(s) in method "ValueTask`1<!0> MemoryStoreFactory.CreateAsync(IInputLogger,ProtocolConfiguration,MqttConfigurationBase,String,Boolean)" at type(s) CK.MQTT.MemoryPacketStore, CK.MQTT.MemoryPacketStore.

The n$8 make me think of a placeholder not replaced ?

xi-liu-ds commented 2 years ago

Hi Kuinox, thanks for your posting! According to the resource leak warnings you shared, the leaked types are CK.MQTT.MemoryPacketStore, CK.MQTT.TcpChannel and CK.MQTT.MemoryPacketStore, accordingly. You may find these types highlighted in the warnings attached below.

image

Please let me know if you have any further question.

matjin commented 2 years ago

Just to add -- it's not always possible to tie a resource to a variable, which can result in identifying a resource via an identifier like n$8. For example, if a resource is leaked by its allocation within the constructor of another class TestClass, i.e. TestClass(new StreamWriter("file.txt")), then there is no variable identifier for StreamWriter("file.txt").

Kuinox commented 2 years ago

For example, if a resource is leaked by its allocation within the constructor of another class TestClass, i.e. TestClass(new StreamWriter("file.txt")), then there is no variable identifier for StreamWriter("file.txt").

Ok, got it.
The first and thirds error are to be expected, this is an In-Memory store with currently no way to clean it. (Is there a built in way to silence errors/warnings ?)

The second error is surprising:

public ValueTask<IMqttChannel> CreateAsync( IActivityMonitor? m, string connectionString )
{
    string[] strs = connectionString.Split( ':' );
    return new( new TcpChannel( strs[0], int.Parse( strs[1] ) ) );
}

TcpChannel implement IMqttChannel, which itself implement IDisposable, I don't see the leak here, is this a false positive ?

matjin commented 2 years ago

If the inheritance pattern follows TcpChannel <- IMqttChannel <- IDiposable as you say, I think the leak here could be referring to you allocating TcpChannel as part of IMqttChannel and potentially not freeing it in in all cases -- does the IMqttChannel implementation of Dispose() free TcpChannel?

Kuinox commented 2 years ago

I don't understand, TcpChannel implement the IMqttChannel, and the TcpChannel Dispose it's own private TcpClient.
I would understand if the factory method was used somewhere and the calee leak the TcpChannel, but currently I don't see how the factory method itself leak the TcpChannel.

matjin commented 2 years ago

Yes, you're right -- if indeed all callees of the factory method take care to free the resource, then this is a false positive warning. Reporting in the factory method itself rather than callees of the factory method is an artifact of the analysis we are looking into improving. With that said, we have seen in practice that it is often the case that such callees do typically fail to free the resource -- it might be worth double-checking that TcpChannel does get disposed by all of the callees of the factory in your case

Kuinox commented 2 years ago

If this is a false positive, are you interested in a minimal reproduction ?
I glanced at the code yesterday and couldn't find the leak, but the objetcs lifecycle are not trivial.

matjin commented 2 years ago

If I understand the issue you've been pointing to, the false positive reproduction would look something like the below; is that what you had in mind? We are actively investigating this issue.

image

Kuinox commented 2 years ago

I have probably a variant of this one. Thanks for the explanations!