CA2202, comment résoudre ce cas

102

Quelqu'un peut-il me dire comment supprimer tous les avertissements CA2202 du code suivant?

public static byte[] Encrypt(string data, byte[] key, byte[] iv)
{
    using(MemoryStream memoryStream = new MemoryStream())
    {
        using (DESCryptoServiceProvider cryptograph = new DESCryptoServiceProvider())
        {
            using (CryptoStream cryptoStream = new CryptoStream(memoryStream, cryptograph.CreateEncryptor(key, iv), CryptoStreamMode.Write))
            {
                using(StreamWriter streamWriter = new StreamWriter(cryptoStream))
                {
                    streamWriter.Write(data);
                }
            }
        }
        return memoryStream.ToArray();
    }
}

Avertissement 7 CA2202: Microsoft.Usage: L'objet 'cryptoStream' peut être supprimé plusieurs fois dans la méthode 'CryptoServices.Encrypt (string, byte [], byte [])'. Pour éviter de générer une exception System.ObjectDisposedException, vous ne devez pas appeler Dispose plus d'une fois sur un objet.: Lignes: 34

Avertissement 8 CA2202: Microsoft.Usage: L'objet 'memoryStream' peut être supprimé plusieurs fois dans la méthode 'CryptoServices.Encrypt (string, byte [], byte [])'. Pour éviter de générer une exception System.ObjectDisposedException, vous ne devez pas appeler Dispose plus d'une fois sur un objet: Lignes: 34, 37

Vous avez besoin de l'analyse du code Visual Studio pour voir ces avertissements (ce ne sont pas des avertissements du compilateur c #).

testalino
la source
1
Ce code ne génère pas ces avertissements.
Julien Hoarau
1
Je reçois 0 avertissement pour cela (niveau d'avertissement 4, VS2010). Et pour quelqu'un qui recherche des problèmes dans ce domaine sur Google, veuillez également ajouter le texte des avertissements.
Henk Holterman
29
Les avertissements CAxxxx sont générés par l' analyse de code et FxCop.
dtb le
Cet avertissement ne s'applique pas au code affiché - les avertissements peuvent être supprimés exactement pour ce scénario. Une fois que vous avez examiné votre code et accepté cette évaluation, placez ceci au-dessus de votre méthode: " [SuppressMessage("Microsoft.Usage", "CA2202:Do not dispose objects multiple times", Justification="BrainSlugs83 said so.")]" - assurez-vous d'avoir une using System.Diagnostics.CodeAnalysis;déclaration " " dans votre bloc d'utilisations.
BrainSlugs83
2
Jetez un œil à: msdn.microsoft.com/en-us/library/…
Adil Mammadov le

Réponses:

-3

Cela compile sans avertissement:

    public static byte[] Encrypt(string data, byte[] key, byte[] iv)
    {
        MemoryStream memoryStream = null;
        DESCryptoServiceProvider cryptograph = null;
        CryptoStream cryptoStream = null;
        StreamWriter streamWriter = null;
        try
        {
            memoryStream = new MemoryStream();
            cryptograph = new DESCryptoServiceProvider();
            cryptoStream = new CryptoStream(memoryStream, cryptograph.CreateEncryptor(key, iv), CryptoStreamMode.Write);
            var result = memoryStream;              
            memoryStream = null;
            streamWriter = new StreamWriter(cryptoStream);
            cryptoStream = null;
            streamWriter.Write(data);
            return result.ToArray();
        }
        finally
        {
            if (memoryStream != null)
                memoryStream.Dispose();
            if (cryptograph != null)
                cryptograph.Dispose();
            if (cryptoStream != null)
                cryptoStream.Dispose();
            if (streamWriter != null)
                streamWriter.Dispose();
        }
    }

Modifier en réponse aux commentaires: je viens de vérifier à nouveau que ce code ne génère pas l'avertissement, contrairement à l'original. Dans le code d'origine, CryptoStream.Dispose()etMemoryStream().Dispose( ) sont en fait appelés deux fois (ce qui peut ou non être un problème).

Le code modifié fonctionne comme suit: les références sont définies sur null, dès que la responsabilité de l'élimination est transférée à un autre objet. Eg memoryStreamest mis à une nullfois que l'appel au CryptoStreamconstructeur a réussi. cryptoStreamest défini sur null, après que l'appel au StreamWriterconstructeur a réussi. Si aucune exception ne se produit, streamWriterest disposé dans le finallybloc et disposera à son tour CryptoStreamet MemoryStream.

Henrik
la source
85
-1 C'est vraiment mauvais de créer du code laid juste pour se conformer à un avertissement qui devrait être supprimé .
Jordão
4
Je conviens que vous ne devriez pas massacrer votre code pour quelque chose qui pourrait finir par être corrigé à un moment donné dans le futur, supprimez simplement.
peteski
3
Comment cela résout-il le problème? CA2202 est toujours signalé car memoryStream peut toujours être supprimé deux fois dans le bloc finally.
Chris Gessler
3
Puisque CryptoStream appelle Dispose sur le MemoryStream en interne, il peut être appelé deux fois, ce qui est la raison de l'avertissement. J'ai essayé votre solution et je reçois toujours l'avertissement.
Chris Gessler
2
Oh putain, vous avez raison - je ne m'attendais pas à ce qu'il y ait une logique de nettoyage mélangée à votre ... logique-logique ... - c'est juste bizarre et cryptique - c'est certainement intelligent - mais encore une fois, effrayant - ne faites pas cela dans le code de production; pour être clair: vous comprenez qu'il n'y a aucun problème fonctionnel réel que cela résout, n'est-ce pas? (Il est normal de disposer ces objets plusieurs fois.) - Je supprimerais le vote négatif si je le pouvais (SO m'empêche, cela dit que vous devez modifier la réponse) - mais je ne le ferais qu'à contrecœur ... - et sérieusement, ne fais jamais ça.
BrainSlugs83
142

Vous devez supprimer les avertissements dans ce cas. Le code qui traite des jetables doit être cohérent et vous ne devriez pas avoir à vous soucier que d'autres classes s'approprient les jetables que vous avez créés et font également appel Disposeà eux.

[SuppressMessage("Microsoft.Usage", "CA2202:Do not dispose objects multiple times")]
public static byte[] Encrypt(string data, byte[] key, byte[] iv) {
  using (var memoryStream = new MemoryStream()) {
    using (var cryptograph = new DESCryptoServiceProvider())
    using (var cryptoStream = new CryptoStream(memoryStream, cryptograph.CreateEncryptor(key, iv), CryptoStreamMode.Write))
    using (var streamWriter = new StreamWriter(cryptoStream)) {
      streamWriter.Write(data);
    }
    return memoryStream.ToArray();
  }
}

MISE À JOUR: Dans la documentation IDisposable.Dispose , vous pouvez lire ceci:

Si la méthode Dispose d'un objet est appelée plus d'une fois, l'objet doit ignorer tous les appels après le premier. L'objet ne doit pas lever d'exception si sa méthode Dispose est appelée plusieurs fois.

On peut soutenir que cette règle existe pour que les développeurs puissent utiliser la usingdéclaration dans une cascade de jetables, comme je l'ai montré ci-dessus (ou peut-être que c'est juste un bel effet secondaire). De la même manière, CA2202 ne sert à rien et devrait être supprimé au niveau du projet. Le vrai coupable serait une implémentation défectueuse de Dispose, et CA1065 devrait s'en occuper (si c'est sous votre responsabilité).

Jordão
la source
14
À mon avis, c'est un bogue dans fxcop, cette règle est tout simplement fausse. La méthode dispose ne doit jamais lancer une ObjectDisposedException et si c'est le cas, vous devez la traiter à ce moment-là en déposant un bogue sur l'auteur du code qui implémente dispose de cette façon.
justin.m.chase
14
Je suis d'accord avec @HansPassant dans l'autre fil: l'outil fait son travail et vous avertit d'un détail d'implémentation inattendu des classes. Personnellement, je pense que le vrai problème est la conception des API elles-mêmes. Le fait que les classes imbriquées supposent par défaut qu'il est acceptable de s'approprier un autre objet créé ailleurs semble très discutable. Je peux voir où cela pourrait être utile si l'objet résultant allait être retourné, mais le fait de revenir sur cette hypothèse semble contre-intuitif, ainsi que de violer les modèles normaux d'IDisposable.
BTJ
8
Mais msdn ne recommande pas de supprimer ce type de message. Jetez un œil à: msdn.microsoft.com/en-us/library/…
Adil Mammadov
2
Merci pour le lien @AdilMammadov, info utile mais Microsoft n'a pas toujours raison sur ces choses.
Tim Abell
40

Eh bien, c'est exact, la méthode Dispose () sur ces flux sera appelée plus d'une fois. La classe StreamReader deviendra «propriétaire» du cryptoStream, donc l'élimination de streamWriter supprimera également cryptoStream. De même, la classe CryptoStream prend en charge la responsabilité de memoryStream.

Ce ne sont pas exactement de vrais bogues, ces classes .NET sont résilientes à plusieurs appels Dispose (). Mais si vous voulez vous débarrasser de l'avertissement, vous devez supprimer l'instruction using pour ces objets. Et souffrez-vous un peu en raisonnant ce qui se passera si le code lève une exception. Ou fermez l'avertissement avec un attribut. Ou ignorez simplement l'avertissement car c'est idiot.

Hans Passant
la source
10
Avoir des connaissances particulières sur le comportement interne des classes (comme un jetable s'appropriant une autre) est trop demander si l'on veut concevoir une API réutilisable. Je pense donc que les usingdéclarations devraient rester. Ces avertissements sont vraiment stupides.
Jordão
4
@ Jordão - n'est-ce pas à quoi sert l'outil? Pour vous avertir d'un comportement interne dont vous ne saviez peut-être pas?
Hans Passant
8
Je suis d'accord. Mais je n'abandonnerais toujours pas les usingdéclarations. Cela me semble juste mal de s'appuyer sur un autre objet pour disposer d'un objet que j'ai créé. Pour ce code, c'est OK, mais il existe de nombreuses implémentations de Streamet TextWriterlà-bas (pas seulement sur la BCL). Le code pour les utiliser tous doit être cohérent.
Jordão
3
Oui, d'accord avec Jordão. Si vous voulez vraiment que le programmeur soit conscient du comportement interne de l'API, alors exprimez-vous en nommant votre fonction DoSomethingAndDisposeStream (Stream stream, OtherData data).
ZZZ
4
@HansPassant Pouvez-vous indiquer où il est documenté que la XmlDocument.Save()méthode appellera Disposele paramètre fourni? Je ne le vois pas dans la documentation de Save(XmlWriter)(où je rencontre le bogue FxCop), ni dans la Save()méthode elle-même, ni dans la documentation XmlDocumentelle - même.
Ian Boyd
9

Lorsqu'un StreamWriter est supprimé, il supprimera automatiquement le Stream encapsulé (ici: le CryptoStream ). CryptoStream supprime également automatiquement le Stream encapsulé (ici: le MemoryStream ).

Ainsi, votre MemoryStream est supprimé à la fois par CryptoStream et l' instruction using . Et votre CryptoStream est supprimé par le StreamWriter et l' instruction using externe .


Après quelques expérimentations, il semble impossible de se débarrasser complètement des avertissements. Théoriquement, le MemoryStream doit être supprimé, mais vous ne pourriez théoriquement plus accéder à sa méthode ToArray. Pratiquement, un MemoryStream n'a pas besoin d'être éliminé, alors j'irais avec cette solution et supprimerais l'avertissement CA2000.

var memoryStream = new MemoryStream();

using (var cryptograph = new DESCryptoServiceProvider())
using (var writer = new StreamWriter(new CryptoStream(memoryStream, ...)))
{
    writer.Write(data);
}

return memoryStream.ToArray();
dtb
la source
9

Je ferais cela en utilisant #pragma warning disable.

Les directives du .NET Framework recommandent d'implémenter IDisposable.Dispose de telle sorte qu'il puisse être appelé plusieurs fois. À partir de la description MSDN de IDisposable .

L'objet ne doit pas lever d'exception si sa méthode Dispose est appelée plusieurs fois

Par conséquent, l'avertissement semble presque dénué de sens:

Pour éviter de générer une exception System.ObjectDisposedException, vous ne devez pas appeler Dispose plus d'une fois sur un objet

Je suppose que l'on pourrait faire valoir que l'avertissement peut être utile si vous utilisez un objet IDisposable mal implémenté qui ne suit pas les directives d'implémentation standard. Mais lorsque vous utilisez des classes du .NET Framework comme vous le faites, je dirais qu'il est prudent de supprimer l'avertissement en utilisant un #pragma. Et à mon humble avis, c'est préférable de passer par des cerceaux comme suggéré dans la documentation MSDN pour cet avertissement .

Joe
la source
4
CA2202 est un avertissement d'analyse de code et non un avertissement du compilateur. #pragma warning disablene peut être utilisé que pour supprimer les avertissements du compilateur. Pour supprimer un avertissement d'analyse de code, vous devez utiliser un attribut.
Martin Liversage
2

J'ai été confronté à des problèmes similaires dans mon code.

On dirait que toute la chose CA2202 est déclenchée car elle MemoryStreampeut être supprimée si une exception se produit dans le constructeur (CA2000).

Cela pourrait être résolu comme ceci:

 1 public static byte[] Encrypt(string data, byte[] key, byte[] iv)
 2 {
 3    MemoryStream memoryStream = GetMemoryStream();
 4    using (DESCryptoServiceProvider cryptograph = new DESCryptoServiceProvider())
 5    {
 6        CryptoStream cryptoStream = new CryptoStream(memoryStream, cryptograph.CreateEncryptor(key, iv), CryptoStreamMode.Write);
 7        using (StreamWriter streamWriter = new StreamWriter(cryptoStream))
 8        {
 9            streamWriter.Write(data);
10            return memoryStream.ToArray();
11        }
12    }
13 }
14
15 /// <summary>
16 /// Gets the memory stream.
17 /// </summary>
18 /// <returns>A new memory stream</returns>
19 private static MemoryStream GetMemoryStream()
20 {
21     MemoryStream stream;
22     MemoryStream tempStream = null;
23     try
24     {
25         tempStream = new MemoryStream();
26
27         stream = tempStream;
28         tempStream = null;
29     }
30     finally
31     {
32         if (tempStream != null)
33             tempStream.Dispose();
34     }
35     return stream;
36 }

Notez que nous devons renvoyer l' memoryStreamintérieur de la dernière usinginstruction (ligne 10) car il cryptoStreamest supprimé à la ligne 11 (car il est utilisé dans l' streamWriter usinginstruction), ce qui conduit memoryStreamà être également supprimé à la ligne 11 (car il memoryStreamest utilisé pour créer le cryptoStream).

Au moins ce code a fonctionné pour moi.

ÉDITER:

Aussi drôle que cela puisse paraître, j'ai découvert que si vous remplacez la GetMemoryStreamméthode par le code suivant,

/// <summary>
/// Gets a memory stream.
/// </summary>
/// <returns>A new memory stream</returns>
private static MemoryStream GetMemoryStream()
{
    return new MemoryStream();
}

vous obtenez le même résultat.

Jimi
la source
1

Le cryptostream est basé sur le memorystream.

Ce qui semble se produire, c'est que lorsque le flux cryogénique est disposé (à la fin de l'utilisation), le flux mémoire est également supprimé, puis le flux mémoire est à nouveau supprimé.

Shiraz Bhaiji
la source
1

Je voulais résoudre ce problème de la bonne manière - c'est-à-dire sans supprimer les avertissements et éliminer correctement tous les objets jetables.

J'ai extrait 2 des 3 flux en tant que champs et les Dispose()ai disposés dans la méthode de ma classe. Oui, la mise en œuvre de l' IDisposableinterface n'est peut-être pas nécessairement ce que vous recherchez, mais la solution semble assez propre par rapport aux dispose()appels provenant de tous les endroits aléatoires du code.

public class SomeEncryption : IDisposable
    {
        private MemoryStream memoryStream;

        private CryptoStream cryptoStream;

        public static byte[] Encrypt(string data, byte[] key, byte[] iv)
        {
             // Do something
             this.memoryStream = new MemoryStream();
             this.cryptoStream = new CryptoStream(this.memoryStream, encryptor, CryptoStreamMode.Write);
             using (var streamWriter = new StreamWriter(this.cryptoStream))
             {
                 streamWriter.Write(plaintext);
             }
            return memoryStream.ToArray();
        }

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

       protected virtual void Dispose(bool disposing)
        {
            if (disposing)
            {
                if (this.memoryStream != null)
                {
                    this.memoryStream.Dispose();
                }

                if (this.cryptoStream != null)
                {
                    this.cryptoStream.Dispose();
                }
            }
        }
   }
divyanshm
la source
0

Hors sujet mais je vous suggère d'utiliser une technique de mise en forme différente pour le regroupement des usings:

using (var memoryStream = new MemoryStream())
{
    using (var cryptograph = new DESCryptoServiceProvider())
    using (var encryptor = cryptograph.CreateEncryptor(key, iv))
    using (var cryptoStream = new CryptoStream(memoryStream, encryptor, CryptoStreamMode.Write))
    using (var streamWriter = new StreamWriter(cryptoStream))
    {
        streamWriter.Write(data);
    }

    return memoryStream.ToArray();
}

Je recommande également d'utiliser vars ici pour éviter les répétitions de noms de classe très longs.

PS Merci à @ShellShock pour remarquer que je ne peux pas accolades Omettre pour le premier usingcomme il serait memoryStreamdans la returndéclaration sur la portée.

Dan Abramov
la source
5
MemoryStream.ToArray () ne sera-t-il pas hors de portée?
Polyfun
C'est absolument équivalent au morceau de code original. J'ai juste omis les accolades, un peu comme vous pouvez le faire avec ifs (bien que je ne conseille pas cette technique pour autre chose que usings).
Dan Abramov
2
Dans le code d'origine, memoryStream.ToArray () était dans la portée de la première utilisation; vous l'avez hors de portée.
Polyfun
Merci beaucoup, je viens de réaliser que vous vouliez dire une returndéclaration. Tellement vrai. J'ai modifié la réponse pour refléter cela.
Dan Abramov
usingPersonnellement, je pense que le sans accolades rend le code plus fragile (pensez à des années de différences et de fusions). joelonsoftware.com/2005/05/11/making-wrong-code-look-wrong & imperialviolet.org/2014/02/22/applebug.html
Tim Abell
0

Évitez toutes les utilisations et utilisez des appels de suppression imbriqués!

    public static byte[] Encrypt(string data, byte[] key, byte[] iv)
    {
        MemoryStream memoryStream = null;
        DESCryptoServiceProvider cryptograph = null;
        CryptoStream cryptoStream = null;
        StreamWriter streamWriter = null;

        try
        {
            memoryStream = new MemoryStream();
            cryptograph = new DESCryptoServiceProvider();
            cryptoStream = new CryptoStream(memoryStream, cryptograph.CreateEncryptor(key, iv), CryptoStreamMode.Write);
            streamWriter = new StreamWriter(cryptoStream);

            streamWriter.Write(data);
            return memoryStream.ToArray();
        }
        finally 
        {
            if(streamWriter != null)
                streamWriter.Dispose();
            else if(cryptoStream != null)
                cryptoStream.Dispose();
            else if(memoryStream != null)
                memoryStream.Dispose();

            if (cryptograph != null)
                cryptograph.Dispose();
        }
    }
Harry Saltzman
la source
1
Veuillez expliquer pourquoi vous devriez éviter usingdans ce cas.
StuperUser
1
Vous pouvez garder l'instruction using au milieu, mais vous devez résoudre les autres. Pour obtenir une solution logique cohérente et évolutive dans toutes les directions, j'ai décidé de supprimer toutes les utilisations!
Harry Saltzman
0

Je voulais juste déballer le code afin que nous puissions voir plusieurs appels Disposesur les objets:

memoryStream = new MemoryStream()
cryptograph = new DESCryptoServiceProvider()
cryptoStream = new CryptoStream()
streamWriter = new StreamWriter()

memoryStream.Dispose(); //implicitly owned by cryptoStream
cryptoStream.Dispose(); //implicitly owned by streamWriter
streamWriter.Dispose(); //through a using

cryptoStream.Dispose(); //INVALID: second dispose through using
cryptograph.Dispose(); //through a using
memorySTream.Dipose(); //INVALID: second dispose through a using

return memoryStream.ToArray(); //INVALID: accessing disposed memoryStream

Alors que la plupart des classes .NET sont (espérons-le) résilientes contre l'erreur de plusieurs appels à .Dispose, toutes les classes ne sont pas aussi défensives contre une mauvaise utilisation des programmeurs.

FX Cop le sait et vous en avertit.

Vous avez quelques choix;

  • n'appelle Disposequ'une seule fois sur n'importe quel objet; ne pas utiliserusing
  • continuez d'appeler dispose deux fois et espérons que le code ne plante pas
  • supprimer l'avertissement
Ian Boyd
la source
-1

J'ai utilisé ce genre de code qui prend l'octet [] et renvoie l'octet [] sans utiliser de flux

public static byte[] Encrypt(byte[] data, byte[] key, byte[] iv)
{
  DES des = new DES();
  des.BlockSize = 128;
  des.Mode = CipherMode.CBC;
  des.Padding = PaddingMode.Zeros;
  des.IV = IV
  des.Key = key
  ICryptoTransform encryptor = des.CreateEncryptor();

  //and finaly operations on bytes[] insted of streams
  return encryptor.TransformFinalBlock(plaintextarray,0,plaintextarray.Length);
}

De cette façon, tout ce que vous avez à faire est de convertir une chaîne en octet [] en utilisant des encodages.

Luka Rahne
la source