3

This question may seem to you duplicate of CA2202, how to solve this case which has an accepted answer. But you may realize that accepted answer has 5 down votes based on poor quality. Also any other up voted answers are not actually solves the issue. Most of them explains how to suppress the rule or debates about how wrong this rule is and why we should ignore it. Since that rule is there, there should be a way to satisfy it and I'm looking for community support to solve that issue.

I'm trying to figure it out how to satisfy CA2202 in the following code. I understand that the issue here is, using statement also disposes the encryptedStream object. But if I remove the finally part, it starts to throw CA2000

So, what is the correct way of writing it to comply with CA2202 and CA2000

byte[] result;

MemoryStream encryptedStream = null;
try
{
    encryptedStream = new MemoryStream();
    using (var cryptStream = new CryptoStream(encryptedStream, cryptoTransform, CryptoStreamMode.Write))
    {
        cryptStream.Write(inputInBytes, 0, inputInBytes.Length);
        cryptStream.FlushFinalBlock();
        result = encryptedStream.ToArray();
    }
}
finally
{
    encryptedStream?.Dispose();
}
string output = Convert.ToBase64String(result);
Community
  • 1
  • 1
cilerler
  • 9,010
  • 10
  • 56
  • 91
  • The accepted answer has -5, it is true, but other answers have as many as 60 upvotes and look relevant to the question. Why aren't any of those the answer to your question, as they appear to be? – BobRodes Feb 11 '15 at 03:26
  • @BobRodes please read those, nobody actually provides useful information. Since when providing method for disabling the alert is an answer? Clearly the question has been hijacked and I'm trying to solve the same issue and read every single one of the question in StackOverflow before I post it. – cilerler Feb 11 '15 at 03:29
  • Since when? Since "code that deals with disposables should be consistent, and you shouldn't have to know that other classes take ownership of the disposables you created." Seems reasonable to me, and apparently to a lot of other people too. It doesn't seem reasonable to me to say that the 80 or so people who upvoted this point of view did so to help hijack the thread. It's also not reasonable to assume that people who disagree with you didn't bother to read the answer. – BobRodes Feb 11 '15 at 04:17
  • @BobRodes it is an educated debate not the actual solution my friend. I already stated the root cause in my question. But if you see the exact solution in that question, please help me to properly implement the code so I can pass the CA2202 without suppressing it. I really do understand your point but please try to look from my perspective. My task is satisfying the rule, not to relaying on debates. I was looking for a community support instead I'm trying to prove why this question is valid and making you guys upset. I'm sorry to take your time and thank you for actually reading it. – cilerler Feb 11 '15 at 04:43
  • @cilerler We're not asking you to be silent; all we're asking you to do is to edit your question to explain *why* the answers on the other question do not address your question. Once you do that, we can re-open this question. – George Stocker Feb 11 '15 at 13:08
  • @GeorgeStocker well, I'm done. Hope it satisfies everyone and I can get my last 2 votes to re-open it. Thank you for your time. – cilerler Feb 11 '15 at 13:28
  • Just as an aside - you can just call `ToArray` on a `MemoryStream` to obtain a `byte[]` of the correct length. No need to muck about with positioning, manually allocating and reading. – Damien_The_Unbeliever Feb 11 '15 at 13:41
  • @Damien_The_Unbeliever thank you! updated in the code – cilerler Feb 11 '15 at 14:11

2 Answers2

6

This is a literal answer to your question, in that it will not issue CA warnings without suppressing them, and will only ever call every Dispose once:

MemoryStream encryptedStream = null;
CryptoStream cryptStream = null;
try {
    encryptedStream = new MemoryStream();
    cryptStream = new CryptoStream(encryptedStream, cryptoTransform, CryptoStreamMode.Write);
    cryptStream.Write(inputInBytes, 0, inputInBytes.Length);
    cryptStream.FlushFinalBlock();
    result = encryptedStream.ToArray();
} finally {
    if (cryptStream != null) {
        cryptStream.Dispose();
   } else {
        if (encryptedStream != null) encryptedStream.Dispose();
   }
}
string output = Convert.ToBase64String(result);

But any developer worth their salt should take a look at this and go "hmm, it's like they didn't know using, I'd better rewrite that". Do not do this in production code. Suppress the warning. Getting code like this correct (and having it remain correct in the face of changes) is actually harder than writing code that uses using with suppression of spurious warnings (indeed, I'm not entirely sure the above code is correct!). It defeats the entire point of having static code analysis in the first place: to write reliable code. You should see code analysis as a tool, not an arbiter of correctness.

Jeroen Mostert
  • 27,176
  • 2
  • 52
  • 85
2

Really, really, suppress the warning. The warning is wrong. It's okay to do. :-)

Do a Google search for "CA2202 nested using" and you will find dozens of stackoverflow and MSDN forum posts on this issue. https://www.google.com/search?q=ca2202+nested+using

Ultimately, CA2202 and CA2000 are limited because they can't understand the behavior of nested IDisposable objects. Some streams can be configured to leave the underlying stream open, but usually they don't. Suppression really is the correct solution. The problem is that you are trying to be a good citizen, so you are trying to comply with the warnings you are given. But static analysis just isn't smart enough to handle this.

Moby Disk
  • 3,761
  • 1
  • 19
  • 38
  • Thank you Moby, I understand that suppressing is an option. But I would like to satisfying it some how. Thanks! – cilerler Feb 11 '15 at 13:48
  • 1
    Even disregarding static analysis, the [contract for `IDisposable.Dispose()`](https://msdn.microsoft.com/en-us/library/system.idisposable.dispose%28v=vs.110%29.aspx) states "If an object's `Dispose` method is called more than once, the object must ignore all calls after the first one" (and this is not a recent addition). While it *may* be an error or oversight in some code to call `Dispose` more than once, it's unavoidable in some scenarios, and a well-written implementation must handle this case anyway. – Jeroen Mostert Feb 11 '15 at 13:50
  • @JeroenMostert I agree. I originally wrote "CA2202 and CA2000 are wrong" then I backpedaled because I didn't want someone to downvote me. In my projects, I just turn them off entirely because the false positive rate is so high. – Moby Disk Feb 11 '15 at 13:53
  • @cilerler Suppressing the warning is _the unique_ way to go. This problem is all about _streams taking ownreship of the inner stream_. You _can't_ eliminate both CA2202 and CA2000 without butchering your code. – ken2k Feb 11 '15 at 13:59
  • @ken2k Thank you! I understand that and stated in my question. Suppressing is out there, people who wants to suppress they can, we all know what to do. But I'm interesting to satisfy part. – cilerler Feb 11 '15 at 14:02
  • @cilerler My answer in the linked question does exactly that. It was downvoted, because it looks ugly, but it's the only way. – Henrik Feb 11 '15 at 14:30
  • Instead of posting a link to a google search, why not quote the various passages and link directly to them? – George Stocker Feb 11 '15 at 14:46
  • @GeorgeStocker Effort. :-) They don't really say anything that isn't in the linked SO entries that the original post quoted. It's more fuel just to say "it's not a bad thing to suppress messages. The warnings just stink." Still, I will consider doing that in future answers. Thanks for the suggestion. – Moby Disk Feb 11 '15 at 15:49
  • @Henrik my apologies, I saw the similarity with the accepted answer. While I testing it, it threw CA2000 which shouldn't do since it is almost same. Again, I'm sorry :'( – cilerler Feb 11 '15 at 17:49