1

I'm just reviewing some of my code and thought I'd throw this out there for comment.

I have a factory class that builds and returns a class for RSA Crypto tasks. It implements IDisposable. All the uses of the created RSA instances are created with RSA.Create() and are wrapped in using() statements.

One of the constructors allows an RSA instance to be passed into the class. At the moment this instance is also used in the same manner inside the same using() statements as RSA instance created by this class.

The basic shape of the class is (lots ommitted for brevity):

internal sealed class RsaCrypto : IRsaCrypto
{
    ~RsaCrypto()
    {
        Dispose(false);
    }

    public RsaCrypto(RSA rsa)
    {
        _rsa = rsa;
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    private void Dispose(bool disposing)
    {
        if (!_disposed)
        {
            if (disposing)
            {
                PublicKey = null;
                PrivateKey = null;
            }

            _rsa?.Clear();
            _disposed = true;
    }
}

I have two questions really.

A) Is a finalizer here really needed? Can an RSA instance be regarded as unmanaged? Since it does depend on the OS as to what actually gets created in RSA.Create() particularly on non windows boxes. (this is in a .NET Standard 2.0 library).

B) I'm thinking that since the RSA instance passed in isn't really "owned" by this class it shouldn't dispose of it in a using() statement.

Part of the intention of this class is to help promote best practice I'm just a little unsure of this current design.

  • 1
    As a library writer, don't write for the user who will do the right thing. Write for the user that makes a mistake, is a complete novice, or is wilfully trying to break your code (particularly when writing a security orientated component). Keep the finaliser. – Kain0_0 May 31 at 23:03
  • Yeah, this was exactly what I was thinking too, and why I designed it this way. Not much of a helper if it allows you to cock it all up! To be honest, a large portion of me thinks I should actually ditch this ctor altogether. – Jammer May 31 at 23:20
  • I mean, in a lot of cases it's actually going to make people write new RSACryptoServiceProvider() in their code, exactly what they shouldn't do. – Jammer May 31 at 23:25
0

A) Is a finalizer here really needed?

RSA is a managed abstract class, so it should have its own finalizer if it owns any unmanaged resources.

B) I'm thinking that since the RSA instance passed in isn't really "owned" by this class it shouldn't dispose of it in a using() statement.

It is a somewhat common pattern to dispose the provided object, but provide a way out if this is not wanted, usually with a leaveOpen parameter that defaults to false. See StreamWriter for a similar scenario.

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.