Discussion :

[randriano]

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 :

Sélectionner tout - Visualiser dans une fenêtre à part

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 :

Sélectionner tout - Visualiser dans une fenêtre à part

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:
TestMemoryLeakVector.zip

[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...

[randriano]

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 :

Sélectionner tout - Visualiser dans une fenêtre à part

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

[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.

[randriano]

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:

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

[Médinoc]

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...

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é.

[randriano]

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 , il faut ajouter un traitement du genre ??

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

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,

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()?

[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()).

[randriano]

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?

[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...

[randriano]

Et ben C++ et ses soucis, ça n'en finit jamais
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?

[Médinoc]

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

Code C++ :

Sélectionner tout - Visualiser dans une fenêtre à part

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.

[randriano]

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 :

Sélectionner tout - Visualiser dans une fenêtre à part

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.

[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.

[randriano]

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

[Médinoc]

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

[Médinoc]

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

Code C++ :

Sélectionner tout - Visualiser dans une fenêtre à part

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 C++ :

Sélectionner tout - Visualiser dans une fenêtre à part

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 C++ :

Sélectionner tout - Visualiser dans une fenêtre à part

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...

[randriano]

Merci pour ces codes!

En fait, je me demande si une erreur sur les NEW d'objets non STL, ça retourne quoi? NULL?

[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...

[randriano]

D'accord mais c'est confirmé donc

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
CObjet* pObjet = new CObjet();
 
if (pObjet)
{
//TO FILL
}

C'est du baratin ! ?