Risque de memory leak selon un analyseur

Version imprimable

1 pièce(s) jointe(s)

Bonjour!

Klocwork l'analyseur de code statique dit que: Possible memory leak. Dynamic memory stored in 'pSETEndFittingProductFamilies' allocated through function 'new' dans ma partie de code:
Code:

1 2 3 4 5 6 7 8 9 10 11 12 else { m_pSubEntityTypesToLoad = new CSubEntityTypesToLoad(); CSubEntityTypeToLoad* pSETConnectedSemiEquipments = new CSubEntityTypeToLoad(enEProcessSETConnectedSemiEquipments, NULL); CSubEntityTypeToLoad* pSETEquProductFamilies = new CSubEntityTypeToLoad(enEProcessSETEquProductFamilies, NULL); if (m_pSubEntityTypesToLoad && pSETConnectedSemiEquipments/* && pSETEquProductFamilies*/) { m_pSubEntityTypesToLoad->push_back(pSETConnectedSemiEquipments); m_pSubEntityTypesToLoad->push_back(pSETEquProductFamilies); } }
alors qu'il semble, j'en suis sur qu'il n'y en a pas
Il s'agit en fait d'une classe héritant un std::vector<>

Il y a bien:
Code:

1 2 3 4 5 6 7 void CVectorSubEntityTypesToLoad::Clear() { for(long i = 0; i < (long)size(); i++) delete at(i); clear(); }
Pensez vous que le leak est possible?

Source:
Pièce jointe 38237

17/11/2008, 14h35
Médinoc

Eh bien, pour commencer, ce code n'est pas exception-safe, donc si l'un des trois new lance une exception std::bad_alloc parce qu'il n'y a plus de mémoire (ou si l'un des constructeurs lance une exception), il y aura fuite...

Et plus simplement, si le test est faux, tes deux pointeurs locaux seront leakés...

Citation:

Envoyé par Médinoc

Eh bien, pour commencer, ce code n'est pas exception-safe, donc si l'un des trois new lance une exception std::bad_alloc parce qu'il n'y a plus de mémoire (ou si l'un des constructeurs lance une exception), il y aura fuite...

Et plus simplement, si le test est faux, tes deux pointeurs locaux seront leakés...

Ah bon ! Il y a donc allocation puis s'il y aerreur et l'espace allouée ne sera jamais désallouée?

Mais si aucune exception, il n'est pas POSSIBLE une memory leak?

Pourquoi l'analyseur précise ces 2 lignes seulement:
Code:

1 2 CSubEntityTypeToLoad* pSETConnectedSemiEquipments = new CSubEntityTypeToLoad(enEProcessSETConnectedSemiEquipments, NULL); CSubEntityTypeToLoad* pSETEquProductFamilies = new CSubEntityTypeToLoad(enEProcessSETEquProductFamilies, NULL);

18/11/2008, 08h22
farscape

salut,
peut être parce qu'il ne voit pas de libération explicite de ces deux pointeurs.
et que le stockage de leur valeur est conditionnel donc risque de fuite.
18/11/2008, 09h28
randriano

Citation:

peut être parce qu'il ne voit pas de libération explicite de ces deux pointeurs.

Si vous parlez de Klocwork: si! Il voit les delete par boucle des objets pointés par des vector<>, list<>, etc.
Mais le dire de Médinoc m'intrigue un peu:

Citation:

si l'un des trois new lance une exception std::bad_alloc parce qu'il n'y a plus de mémoire (ou si l'un des constructeurs lance une exception), il y aura fuite
18/11/2008, 09h36
Médinoc
Citation:

Envoyé par randriano

Si vous parlez de Klocwork: si! Il voit les delete par boucle des objets pointés par des vector<>, list<>, etc.

Sauf que si ton if (m_pSubEntityTypesToLoad && pSETConnectedSemiEquipments) n'est pas satisfait, tes pointeurs ne seront jamais ajouté au vecteur et seront leakés...

Citation:

Mais le dire de Médinoc m'intrigue un peu:

C'est pourtant simple:
- Tu alloues le premier objet CSE, ça marche, le seul pointeur vers cet objet est un pointeur bète dans une variable locale.
- Tu tentes d'allouer le second objet EPF, mais il n'y a plus de mémoire: Une exception bad_alloc est lancée.
- Le seul pointeur vers l'objet CSE est détruit et il n'y a rien pour faire un delete dessus: L'objet CSE est leaké.

Citation:

Sauf que si ton if (m_pSubEntityTypesToLoad && pSETConnectedSemiEquipments) n'est pas satisfait, tes pointeurs ne seront jamais ajouté au vecteur et seront leakés...

D'accord je comprend maintenant ce cas :king:, il faut ajouter un traitement du genre ??
Code:

1 2 3 4 5 6 7 8 else if (!m_pSubEntityTypesToLoad) { if (pSETEndFittingProductFamilies) delete pSETEndFittingProductFamilies; if (pSETEquProductFamilies) delete pSETEquProductFamilies; }
Pour le fait que quelque chose m'intrigue,

Citation:

Tu tentes d'allouer le second objet EPF, mais il n'y a plus de mémoire: Une exception bad_alloc est lancée

Comment le gérer alors? try catch()?

18/11/2008, 13h31
Médinoc
Juste un else devrait suffire, pas besoin d'y mettre un second if en plus.
De plus, inutile de tester tes pointeurs avant le delete:
- Tes pointeurs ne seront jamais nuls car sur tout compilo conforme, new lance une exception en cas d'échec
- Même s'ils sont nuls, delete l'accepte.
Pour le problème, il faut ne pas gérer "manuellement" plus d'une ressource à la fois. Je recommande d'utiliser une classe de pointeur intelligent (ici, même std::auto_ptr<> devrait marcher), sans oublier de "détacher" le pointeur quand tu ajoutes l'objet au vecteur (std::auto_ptr<>::release()).
21/11/2008, 07h43
randriano

Citation:

Envoyé par Médinoc

Pour le problème, il faut ne pas gérer "manuellement" plus d'une ressource à la fois. Je recommande d'utiliser une classe de pointeur intelligent (ici, même std::auto_ptr<> devrait marcher), sans oublier de "détacher" le pointeur quand tu ajoutes l'objet au vecteur (std::auto_ptr<>::release()).

Plein de truc la STL!!

Mais concernant la possible fuite à cause de std::bad_alloc, ça arrive souvent???
J'hésite un peu à la gérer car je ne pense pas que c'est ça que Klocwork soupçonnait non?
21/11/2008, 09h12
Médinoc

Ce n'est clairement pas ça qu'il soupçonnait, et typiquement, quand une exception bad_alloc est lancée, tu as d'autres soucis.
Par contre, si un des constructeurs est susceptible de lancer une autre exception, ben...
21/11/2008, 09h36
randriano

Et ben C++ et ses soucis, ça n'en finit jamais :aie:
Sincèrement, les fuites et autres erreurs qui peuvent être causées par un NEW, je n'y ai jamais pensé jusqu'à maintenant
Peux-tu me donner un exemple de gestion de cela?

Pour moi, le plus simple serait un truc de ce genre:
Code:

1 2 3 4 5 6 7 8 9 10 11 12 else { assert( m_pSubEntityTypesToLoad == 0 ); m_pSubEntityTypesToLoad = new CSubEntityTypesToLoad(); std::auto_ptr<CSubEntityTypeToLoad> spSETConnectedSemiEquipments( new CSubEntityTypeToLoad(enEProcessSETConnectedSemiEquipments, NULL) ); std::auto_ptr<CSubEntityTypeToLoad> spSETEquProductFamilies( new CSubEntityTypeToLoad(enEProcessSETEquProductFamilies, NULL) ); m_pSubEntityTypesToLoad->push_back( spSETConnectedSemiEquipments.get() ); spSETConnectedSemiEquipments.release(); m_pSubEntityTypesToLoad->push_back( spSETEquProductFamilies.get() ); spSETEquProductFamilies.release(); }
PS: Tiens, je n'avais pas vu qu'en fait, sur un compilateur respectueux de la norme, ton if serait toujours vrai, puisque depuis la norme, new ne retourne jamais NULL...
PPS: Note que je ne fais le release() qu'une fois certain que le push_back() a réussi. Ainsi, en cas d'erreur mémoire lors du push_back(), je suis quand même sûr que les objets ne seront pas leakés.
PPPS: Note que la fonction release() n'a généralement pas la même signification pour les autres pointeurs intelligents: Si une classe possède une fonction detach() et une fonction release(), alors le detach() correspond sans doute à auto_ptr<>::release(), et le release() correspondrait alors à un appel à auto_ptr<>::reset() avec un argument nul.

Citation:

Envoyé par Médinoc

PS: Tiens, je n'avais pas vu qu'en fait, sur un compilateur respectueux de la norme, ton if serait toujours vrai, puisque depuis la norme, new ne retourne jamais NULL...
[

Mon Dieu, je ne savais pas cela !!! Vs etes sûr car le test souvent conseillé sur le net avant d'utiliser un objet alloué par new est "if (pObjet) { //TO DO ..."

Pourrai-je vous demander un autre truc sur le code suivant?
Code:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 class CDataChampValue_ { public: CDataChampValue_(_variant_t a_varVisibleValue, _variant_t a_varActualValue, std::wstring a_wstrFilteredValue = L""); virtual ~CDataChampValue_(void); void AddComment(std::wstring a_wstrComment); long CommentsCount(); public: _variant_t m_varVisibleValue; _variant_t m_varActualValue; std::wstring m_wstrFilteredValue; CValueComments m_vecValueComments; }; class CVecDataChampValue : public std::vector<CDataChampValue_*>{}; class CDataChampValues_ : public CVecDataChampValue { public: CDataChampValues_(void); virtual ~CDataChampValues_(void); int AddValue(_variant_t a_varValue, _variant_t a_varActualFieldValue, std::wstring a_wstrFilteredValue = L""); // modif rth CDataChampValue_* LastValue(); ............................................
L'analyseur dit que : This class instantiation, {CLASS-INSTANTIATION}rebind<CDataChampValue_*>, is unused
Alors que cette classe est héritée.

21/11/2008, 13h50
Médinoc

Pour ça, je ne peux pas t'aider, je ne connais pas l'analyseur en question et je ne comprends même pas le message d'erreur.

Pour le coup du new, c'est justement la raison de l'exception bad_alloc : On en lance une au lieu de retourner NULL.
24/11/2008, 07h45
randriano

Citation:

Envoyé par Médinoc

Pour le coup du new, c'est justement la raison de l'exception bad_alloc : On en lance une au lieu de retourner NULL.

En fait, merci pour le bout de code !

Concernant le truc du bad_alloc, merci de l'avoir mentionné, je crois que ce n'est pas seulement moi qui ne sait pas ça, il y en a beaucoup
Mais il y a aussi ce cas, ce qui arrive souvent peut-être, c'est pourquoi on dit, NEW error = return NULL: http://support.microsoft.com/kb/167733/fr
24/11/2008, 09h37
Médinoc

Merde, je ne pensais pas que ce bug était aussi présent dans une version 2005...

En tout cas, ce bug n'est pas présent sous les versions professionnelles de Visual 2005. Ce code lance une exception :

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
/*
Fred:
Ceci est un test pour new, et sa capacité à lancer ou non une exception.
*/
#include <new>
#include <iostream>
 
class BigRaiiAlloc
{
public:
	BigRaiiAlloc() : m_pBuffer(NULL)
	{
		m_pBuffer = new char[(1<<20) * 128];
	}
	~BigRaiiAlloc()
	{
		delete[] m_pBuffer;
	}
	bool IsNull(void) const
	{
		return (m_pBuffer==NULL);
	}
 
private:
	void *m_pBuffer;
};
 
static void AllocRecursively(void)
{
	BigRaiiAlloc ba;
	if(ba.IsNull())
		std::cout << "L'operateur new a retourne NULL!" << std::endl;
	else
		AllocRecursively();
}
 
void TestNew2(void)
{
	//Test allocation massive
	std::cout << "Test allocations..." << std::endl;
	try
	{
		AllocRecursively();
	}
	catch(std::bad_alloc const &)
	{
		std::cout << "L'operateur new a lance une exception." << std::endl;
	}
}

----=== Contribution: CheckNew ====----

D'ailleurs, je pense que si tu veux t'assurer d'avoir une exception, le mieux est un truc de ce genre:

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
//Classe avec vérification au constructeur
class CheckNew
{
public:
	CheckNew(void * p)
	{
		if(p==NULL)
			throw std::bad_alloc();
	}
};
 
//Ces macros donnent à l'objet un nom dépendant du n° de ligne de code:
//Définition macro CHECK_NEW_NAME(x) (en deux étapes)
#define CHECK_NEW_NAME_(x) obj_check_new_ ## x
#define CHECK_NEW_NAME(x)  CHECK_NEW_NAME_(x)
//Macro CheckNew (déclare une variable)
#define CHECK_NEW(p) CheckNew CHECK_NEW_NAME(__LINE__)(p)

Avec ça, tu peux utiliser la macro dans ton code, et tu peux même rajouter des vérifs dans une classe :

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
//-------- Code à ajouter dans le fichier d'en-tête de CheckNew --------
//Définition macro CHECK_NEW_MEMBER_NAME(name)
#define CHECK_NEW_MEMBER_NAME__(pfx,nm) pfx ## nm
#define CHECK_NEW_MEMBER_NAME_(pfx,nm)  CHECK_NEW_MEMBER_NAME__(pfx, nm)
#define CHECK_NEW_MEMBER_NAME(name)     CHECK_NEW_MEMBER_NAME_(m_, CHECK_NEW_NAME_(name))
//Définitions macros d'utilisation
#define CHECK_NEW_MEMBER_DECLARE(name)  CheckNew CHECK_NEW_MEMBER_NAME(name)
#define CHECK_NEW_MEMBER_INIT(name,p)   , CHECK_NEW_MEMBER_NAME(name)(p)
 
//-------- Exemple de code d'utilisation --------
class UseCheckNew
{
	int *m_pMonEntier;
	CHECK_NEW_MEMBER_DECLARE(chk1);
public:
	UseCheckNew(void)
	 : m_pMonEntier(new int) CHECK_NEW_MEMBER_INIT(chk1, m_pMonEntier)
	{
 
	}
 
	~UseCheckNew(void)
	{
		delete m_pMonEntier;
	}
};

Et quand on est sur une implémentation qu'on sait conforme, on peut définir ces macros comme étant égales à rien du tout...

25/11/2008, 12h57
randriano

Merci pour ces codes!:)

En fait, je me demande si une erreur sur les NEW d'objets non STL, ça retourne quoi? NULL?
25/11/2008, 13h31
Médinoc

La provenance de l'objet ne change rien.
La seule chose qui change, c'est le comportement du new s'il échoue parce que l'allocation mémoire échoue.
Le comportement en cas d'exception dans le constructeur ne change pas: L'exception est lancée et doit être attrapée dans le code qui a fait le new...
25/11/2008, 15h17
randriano
D'accord mais c'est confirmé donc
Code:

1 2 3 4 5 6 CObjet* pObjet = new CObjet(); if (pObjet) { //TO FILL }
C'est du baratin ! ?
25/11/2008, 15h21
Médinoc
Ben oui.
Sauf sous les vieux Visual.

C'est pour ça que je conseille plutôt maintenant:
Code:

1 2 3 4 CObjet* pObjet = new CObjet(); CHECK_NEW(pObjet); //TO FILL
Et là ou ça semble le plus approprié, mettre le try/catch(std::bad_alloc const &)...
26/11/2008, 07h58
randriano

OK

Je dois donc réajuster tous mes codes à partir de maintenant! Quelle erreur de ma part d'avoir cru à un vieil article verifiant juste que c'est pas NULL
En fait, j'utilise VISUAL 2003
26/11/2008, 09h39
Médinoc

Si tu utilises 2003, commence par faire le test avec la classe BigRaiiAlloc, pour vérifier le comportement.
27/11/2008, 09h49
randriano

J'ai testé la classe BigRaiiAlloc avec mon VS2003, std::cout << "L'operateur new a retourne NULL!" << std::endl; ne s'est jamais exécuté ... mais ce code plante ma machine mais bon ...
Je retiens donc que ça ne retourne jamais NULL sauf dans le cas de ce bug http://support.microsoft.com/kb/167733/fr où il faut créer son propre gestionnaire
27/11/2008, 10h04
Médinoc

Exécute-le dans le debugger avec un breakpoint sur la ligne en question.
Sous VC++ 6, j'ai bien droit à un affichage en règle de "L'operateur new a retourne NULL!", aussi bien en Debug qu'en Release.

Et oui ça ne s'affiche jamais mais de même aussi pour std::cout << "L'operateur new a lance une exception." << std::endl;

Revenons aussi à un code que vous avez posté ici
Code:

1 2 3 4 5 6 7 8 9 10 11 12 else { assert( m_pSubEntityTypesToLoad == 0 ); m_pSubEntityTypesToLoad = new CSubEntityTypesToLoad(); std::auto_ptr<CSubEntityTypeToLoad> spSETConnectedSemiEquipments( new CSubEntityTypeToLoad(enEProcessSETConnectedSemiEquipments, NULL) ); std::auto_ptr<CSubEntityTypeToLoad> spSETEquProductFamilies( new CSubEntityTypeToLoad(enEProcessSETEquProductFamilies, NULL) ); m_pSubEntityTypesToLoad->push_back( spSETConnectedSemiEquipments.get() ); spSETConnectedSemiEquipments.release(); m_pSubEntityTypesToLoad->push_back( spSETEquProductFamilies.get() ); spSETEquProductFamilies.release(); }
Je n'ai pas besoin de assert( m_pSubEntityTypesToLoad == 0 );, c'est facultatif?
Pour m_pSubEntityTypesToLoad = new CSubEntityTypesToLoad();
Donc, en gros j'ai pu retenir de ce fil que si ça se passe mal, il peut y avoir memory leak même s'il y a delete m_pSubEntityTypesToLoad au destructeur, pour cela j'ai à faire aux macros:
Code:

1 2 3 #define CHECK_NEW_NAME_(x) obj_check_new_ ## x #define CHECK_NEW_NAME(x) CHECK_NEW_NAME_(x) #define CHECK_NEW(p) CheckNew CHECK_NEW_NAME(__LINE__)(p)

27/11/2008, 11h47
Médinoc

L'assertion, c'est jusqe pour vérifier que le pointeur ne pointe pas déjà sur un truc, car l'écraser avec une nouvelle valeur serait sûrement une fuite de mémoire.

Quant aux macros, elles ne suffisent pas, il faut la classe CheckNew en plus.
27/11/2008, 12h07
randriano

Citation:

Quant aux macros, elles ne suffisent pas, il faut la classe CheckNew en plus.

C'était juste pour limiter le message! :P

L'assertion, c'est jusqe pour vérifier que le pointeur ne pointe pas déjà sur un truc, car l'écraser avec une nouvelle valeur serait sûrement une fuite de mémoire.
Tu maitrises très bien les leaks!!! :king:

Qu'est ce que j'ajouterais en plus? Merci, un bon programmeur C++ doit savoir éviter les memory leaks