Still A "Multiple Dispose" Issue, Even Though Handled

Question

In some of my projects I've been using a pair of tried-and-true data encryption/decryption methods (the encryption method is pasted below). But I have always been dogged by this nagging CA2202 warning ("Do not dispose objects multiple times"), about the memoryStream object. I believe I handle this in the appropriate manner, but I still get the warning whenever I run an analysis in Visual Studio. It's never thrown exceptions in production code, but I would still like to get rid of the warning once and for all. Is that possible? Or should I just ignore it? Thanks in advance.

public static string Encrypt(string clearText, string passPhrase, string saltValue)
{
    byte[] clearTextBytes = Encoding.UTF8.GetBytes(clearText);
    byte[] saltValueBytes = Encoding.UTF8.GetBytes(saltValue);

    Rfc2898DeriveBytes passPhraseDerviedBytes = new Rfc2898DeriveBytes(passPhrase, saltValueBytes);
    byte[] keyBytes = passPhraseDerviedBytes.GetBytes(32);
    byte[] initVectorBytes = passPhraseDerviedBytes.GetBytes(16);

    RijndaelManaged symmetricKey = new RijndaelManaged() { Mode = CipherMode.CBC };
    ICryptoTransform encryptor = symmetricKey.CreateEncryptor(keyBytes, initVectorBytes);

    byte[] cipherTextBytes = null;
    MemoryStream memoryStream = null;
    try
    {
        memoryStream = new MemoryStream();
        using (CryptoStream cryptoStream = new CryptoStream(memoryStream, encryptor, CryptoStreamMode.Write))
        {
            cryptoStream.Write(clearTextBytes, 0, clearTextBytes.Length);
            cryptoStream.FlushFinalBlock();
            cipherTextBytes = memoryStream.ToArray();
        }
    }
    finally
    {
        if (memoryStream != null)
        {
            memoryStream.Dispose();
        }
    }

    return Convert.ToBase64String(cipherTextBytes);
}

Just a side note: You are using the `using` statement for the `CryptoStream`, why not use it for the `MemoryStream` as well? — Zohar Peled, Nov 05 '18 at 11:40
@ZoharPeled The docs for the CA2202 tell you to use that. I've been in the same situation before. — Camilo Terevinto, Nov 05 '18 at 11:41
@CamiloTerevinto Thanks, I've read the docs. seems to imply that disposing the cryptoString will also dispose the MemoryStream (that might be true in this case). — Zohar Peled, Nov 05 '18 at 11:47
@ZoharPeled No problem. It *could* be true, but as I showed in my answer, it's not. — Camilo Terevinto, Nov 05 '18 at 11:53
@The doc for CA2202 is IMHO plain wrong, and the advice to replace a concise `using` block by a try/finally bewildering. The doc says that a properly implemented `IDispose` implementation can be called multiple times, and if there are some edge cases where this is "not guaranteed", then the implementations should be fixed rather than expecting every caller to go through hoops that are unnecessary 99.9...9% of the time. — Joe, Nov 05 '18 at 11:57
For those curious... when I originally wrote this method several years ago, I had indeed used a nested using block... that had brought about a CA2202 to begin with, and I had followed the docs to resolve it (removing the outer using block), which has brought me to my current situation. — blcamp, Nov 05 '18 at 12:03
Fix the missing dispose calls, verify that you're disposing everything, then tell analysis to shup up, move on. — Lasse V. Karlsen, Nov 05 '18 at 12:08
I would clean up the code to be properly written and formatted for the programmer, which means disposing of all the objects that implement `IDisposable`, using `using` statements to handle this, get rid of the try/finally block, return directly inside the innermost `using` statement, etc. If code analysis then complains, tell it to shut up. Code analysis isn't "this is wrong", it's just "heads up, have you verified this?" — Lasse V. Karlsen, Nov 05 '18 at 12:16
@Lasse, with all due respect, Code Analysis addresses quality, not necessarily correctness. These weren't drawn up on a whim by someone in a corner office in Redmond. This was carefully thought out by numerous professionals, and it's my job to get my product out the door with the highest quality and following the best of the best practices that I possibly can. While there may be *some* subjectivity involved, I need to remove as much of that opinion-based problem solving as possible prior to deployment. — blcamp, Nov 05 '18 at 19:31
I agree with all of that, apologies if I conveyed the wrong meaning, it's just that this particular rule is quite often an unnecessary one. `Dispose` **should** be allowed to be called multiple times, it's not a *problem* you should focus on fixing. **Not** disposing, however, is a problem you should be fixing, and I see numerous objects in your code that has `IDisposable` implemented without being disposed. These are more important than appeasing code analysis for this particular rule. **Is my opinion**, nothing more. — Lasse V. Karlsen, Nov 06 '18 at 07:28

Guy · Accepted Answer · 2018-11-05T12:17:40.023

1

It's because CryptoStream closes the memoryStream

You are using the constructor

public CryptoStream(Stream stream, ICryptoTransform transform, CryptoStreamMode mode)
    : this(stream, transform, mode, false) {
}

which calls

public CryptoStream(Stream stream, ICryptoTransform transform, CryptoStreamMode mode, bool leaveOpen) {
    _stream = stream;
    _leaveOpen = leaveOpen;
    //...
}

_leaveOpen and _stream are later used in Dispose

protected override void Dispose(bool disposing) {
    try {
        if (!_leaveOpen) {
            _stream.Close();
        }
        //...
    }
}

You can remove the memoryStream.Dispose();, or pass true as parameter to CryptoStream constructor

using (CryptoStream cryptoStream = new CryptoStream(memoryStream, encryptor, CryptoStreamMode.Write, true)) { }

github source code reference

edited Nov 05 '18 at 12:17

answered Nov 05 '18 at 11:48

Guy

46,488
10
44
88

When I originally wrote this method several years ago, I had used a nested using block... that had brought about CA2202 to begin with, and I had followed the docs to resolve it (removing the outer using block), which has brought me to my current situation. – blcamp Nov 05 '18 at 11:58
@blcamp I see, but that's just a side not, I will remove it. – Guy Nov 05 '18 at 11:59
I will try this later on this morning (I'm in Michigan) - will upvote if it works. :) – blcamp Nov 05 '18 at 12:06
Unfortunately (for me), I cannot use that 4-argument constructor; it is only available in .NET 4.7.2 (or Core 2.x). It's not available in 4.5.x or 4.6.x projects. – blcamp Nov 05 '18 at 19:09
@blcamp You can also just remove `memoryStream.Dispose();`, it will be disposed anyway. – Guy Nov 05 '18 at 19:20

score 1 · Answer 2 · answered Nov 05 '18 at 11:49

The problem is that the call to CryptoStream.Dispose can call dispose on the given stream:

protected override void Dispose(bool disposing) 
{
    try 
    {
        if (disposing) 
        {
            if (!_finalBlockTransformed) 
            {
                FlushFinalBlock();
            }
            if (!_leaveOpen) 
            {
                _stream.Close();
            }
        }
    }
    ...
}

If you use the constructor that takes 4 parameters:

CryptoStream(Stream stream, ICryptoTransform transform, CryptoStreamMode mode, bool leaveOpen)

The last argument determines whether the stream is Closed or not. Calling Close() , in turn, by default also calls Dispose:

public virtual void Close()
{
    /* These are correct, but we'd have to fix PipeStream & NetworkStream very carefully.
    Contract.Ensures(CanRead == false);
    Contract.Ensures(CanWrite == false);
    Contract.Ensures(CanSeek == false);
    */

    Dispose(true);
    GC.SuppressFinalize(this);
}

So, it seems that the check cannot correctly determine whether the specific Stream implementation will be Disposed or not, and falls back to think it will be- which is this case.

Note, however, that disposing twice, once or zero times a MemoryStream doesn't matter a lot in most cases.

Still A "Multiple Dispose" Issue, Even Though Handled

2 Answers2