Memory leak in CFile constructor.
Giles R. Chamberlin -- GRC@langley.softwright.co.uk
Friday, August 16, 1996
Environment: VC+4.1, MFC4.1, Win 95, Win NT4 beta2
SUMMARY
One of the CFile constuctors throws an exception without cleaning up,
causing a memory leak.
TEST SETUP:
AppWizard generated dialog box, add a button to the dialog,add an on
click handler for the button as follows:
void CBugDlg::OnMsgbox()
{
UINT flags = CFile::modeRead | CFile::shareDenyNone;
CString strFile = "NoFile.txt"; // This file does not exist
try
{
CFile* pFile = new CFile(strFile, flags); // Constructor throws an
exception
// Assignment to pFile never happens.
delete pFile; // Never get here.
}
catch(CFileException e)
{
TRACE("Caught an Exception")
}
}
The constructor throws an exception and pFile is never assigned. On
quitting the program, a 16 byte CObject memory leak is detected.
_CrtSetBreakAlloc() confirms that this is the object created by the new
CFile.
PROBLEM CODE:
In MFC\SRC\Filecore.cpp we have:
CFile::CFile(LPCTSTR lpszFileName, UINT nOpenFlags)
{
ASSERT(AfxIsValidString(lpszFileName));
m_strFileName.Empty();
CFileException e;
if (!Open(lpszFileName, nOpenFlags, &e))
AfxThrowFileException(e.m_cause, e.m_lOsError, e.m_strFileName);
}
WORK AROUND:
To experiment, I created a similar class, derived directly from CObject
and called its constructor in a similar test frame as the CFile case
above.
Noddy::Noddy()
{
TRACE("Constructor\n");
delete this; // Without this we get a memory leak as per CFile.
throw i;
}
Noddy::~Noddy()
{
TRACE("Destructor\n");
}
QUESTION:
The addition of "delete this" prior to throwing the exception solves the
memory leak problem, but is concerning me slightly as ~Noddy() is called
and I don't yet have a valid object. So, should the MFC CFile::CFile be
calling delete this before throwing the exception? Or should I be
handling things differently in my calling routine.?
Any comments?
Giles Chamberlin
Softwright
Mike Blaszczak -- mikeblas@nwlink.com
Sunday, August 18, 1996
[Mini-digest: 8 responses]
At 09:33 AM 8/16/96 PDT, you wrote:
>Environment: VC+4.1, MFC4.1, Win 95, Win NT4 beta2
>void CBugDlg::OnMsgbox()
>{
> UINT flags = CFile::modeRead | CFile::shareDenyNone;
> CString strFile = "NoFile.txt"; // This file does not exist
>
> try
> {
> CFile* pFile = new CFile(strFile, flags); // Constructor throws an
>exception
> // Assignment to pFile never happens.
> delete pFile; // Never get here.
> }
> catch(CFileException e)
> {
> TRACE("Caught an Exception")
> }
>}
Your code has two flaws:
> catch(CFileException e)
First, you should be catching "CFileException* e", not "CFileException e".
> {
> TRACE("Caught an Exception")
> }
Second, you should be coding (now that you've fixed the first problem):
delete e->Delete();
If you fix these flaws and still have a problem, please write us back.
.B ekiM
http://www.nwlink.com/~mikeblas <--- trip report central
1995 Honda VFR750F (Serial number 00050!) 4 Corners 1996!
1987 Yamaha FZ700 (damaged) AMA, HRC, VFROC
These words are my own: I do not speak for Microsoft.
-----From: Jon Shadforth
*** SNIP ***
The leak is not a problem with the CFile constructor. It occurs because you are
dynamically creating a CFile instance which can only be cleaned up correctly
with a call to delete. But, if an exception is thrown by the CFile constructor
then your 'delete pFile' code will not get called. There are two
simple workarounds to this problem:
1. Create the CFile instances locally on the stack - as soon as the instance
looses scope it will be destructed automatically.
2. Declare pFile before the try block and initialise it to NULL, and also
re-NULL it once you call delete. Then, during the exception handler you can test
to see if the pointer was created and delete it.
Also, I would never call 'delete this' from within a constructor, or from
within any other class member - conceptually it is too wierd to try and figure
out, especially since the object could have been created automatically on the
stack!!!
Jon :-)
--
---------------------------------------------------------------------------
| Jon Shadforth EMail jon@egg-nogg.demon.co.uk |
---------------------------------------------------------------------------
-----From: Mario Contestabile
That can't be true.
C++ only destroys fully constructed
objects, and an object isn't fully constructed until its constructor has
run completion. If an exception is thrown during b's construction, b's
destructor will never be called. C++ won't clean up after objects that throw
exceptions during construction. You must design your constructors such that
they clean up after themselves.
It would be cleaner to simply have a m_CFile, and avoiding the calls to new and
delete
by creating the object, class local.
m_CFile(strFile, flags);
In any case, using the Microsoft try-catch convention
TRY{
CFile* pFile = new CFile(strFile, flags); // Constructor throws an
exception
// Assignment to pFile never happens.
delete pFile; // Never get here.
}
CATCH(CFileException, e){
TRACE("Caught an Exception");
}
END_CATCH
will eliminate your leak.
mcontest@universal.com
-----From: Wolfgang Loch
Hi Giles.
You address the same problem we discussed recently on mfc-l.
"exception in constructor results in memory leak"
You may want to read Mike B.'s comment on this subject. Although
he said the problem should be fixed in VC4.1.
Calling "delete this" in the constructor is definitely not a
good idea. Think about what would happen if the object is not
constructed on the heap. You would try to free memory from
the stack. And maybe the destructor will be called twice.
A Solution for your special CFile problem should be:
CFile* pFile = new CFile(); // no exception, no problem
CFileException e;
if( !pFile->Open(strFile, flags, &e) )
{
delete pFile;
TRACE("File not Opened")
}
No pretty exception handling. But it works.
I made the same experience, that the object's destructor is
being called although no valid object could be created. Nobody
seamed to believe me.
I guess the c++ language standard says the destructor should
never be called if the constructor is not finished successfully.
But what happens when I do this:
CMyClass::CMyClass() {
m_pInt = new int[10];
if( !DoCriticalThing() )
throw new CException;
}
CMyClass::~CMyClass() {
delete m_pInt
}
Here the destructor should be invoked independend of the
the exception being thrown or not.
Wolfgang
--
/-------------------------------------------------\
| Wolgang Loch (Technical University of Ilmenau) |
| e-mail: Wolfgang.Loch@rz.TU-Ilmenenau.DE |
| www : http://www.rz.tu-ilmenau.de/~wolo |
\-------------------------------------------------/
-----From: "Giles R. Chamberlin"
Mario,
Thanks for your answer, but I must have explained myself badly -
> If an exception is thrown during b's construction, b's
> destructor will never be called. C++ won't clean up after objects that
throw
> exceptions during construction.
I agree with you here.
> You must design your constructors such that
> they clean up after themselves.
And agree with this. Unfortunately the CFile constructor in question
(from MFC, not mine) does not clean up.
> It would be cleaner to simply have a m_CFile, and avoiding the calls
> to new and delete by creating the object, class local.
Possibly so, but more generally if the new operator is accessible for a
given class, then it should be allowable to call it. This isn't an
attack on MFC - if I am right and there is a bug in this constructor, big
deal. The reason I'm pursuing this is that I don't fully understand
exceptions and am trying to ascertain what should happen and why.
> In any case, using the Microsoft try-catch convention
> TRY{...} CATCH(CFileException, e){...}END_CATCH
> will eliminate your leak.
'Fraid not, the problem remains exactly the same. As an aside, according
to the online docs, the TRY/CATCH macros are no longer the method of
choice. Now that C++ style try/catch is available, this is the method
of choice (See "Exceptions" in "Programming with MFC" in the online
help).
Thanks,
Giles Chamberlin
Softwright
-----From: Pradeep Tapadiya
ANSWER:
NEVER call "delete this" from a constructor.
1) It may call the destructor for a partially constructed object.
2) More importantly, you are freeing a memory location twice - once
by delete and once by implicit exception handling mechanism.
Please look at the mfc-l archive (is there one?) for a mail I sent
out two weeks ago. Basically, there is a bug in the way Microsoft
has implemented their heap cleanup when a constructor having an
overloaded new operator throws an error. The work around is just to
make sure that the constructor can never throw an exception. In your
case, perhaps you can look at what exception is being thrown and handle
this case even before you do a "new CFile."
By the way, the leak that you see will happen only in the debug mode
but not in the release mode. Operator new is overloaded under debug
mode so that it can store file/line information.
Hope this helps.
Pradeep
pradeep@nuview.com
-----From: bop@gandalf.se
Even more concerning is that "delete this" assumes that you are
allocating the object on the heap. What if the object is allocated
on the stack?
I also believe that "delete this" in the constructor does NOT
call virtual destructors for derived objects. This could introduce
further problems.
Bo Persson
bop@gandalf.se
-----From: Mike Blaszczak
I guess this one is going to turn interesting: I had a few minutes to clean
up the code and try to reproduce the problem. Even after properly catching
the exception and deleting it, this code still leaks some memory.
It shouldn't, because VC++ 4.1 and VC++ 4.2 have compilers that are supposed
to properly unwind the allocations made in a try block.
The code doesn't seem to be getting set up--you can easily demonstrate that
it works if you use something that's _not_ derrived from CObject, so maybe
there's a problem in the compiler that MFC should work around. I'll have to
get this in front of the compiler guys to figure out for sure, though.
.B ekiM
Mike Bergman -- mbergman@datatree.com
Monday, August 19, 1996
[Mini-digest: 3 responses]
>
>Environment: VC+4.1, MFC4.1, Win 95, Win NT4 beta2
>
>SUMMARY
>One of the CFile constuctors throws an exception without cleaning up, =20
>causing a memory leak.
>
>void CBugDlg::OnMsgbox()
>{
> UINT flags =3D CFile::modeRead | CFile::shareDenyNone;
> CString strFile =3D "NoFile.txt"; // This file does not exist
>=20
> try
> {
> CFile* pFile =3D new CFile(strFile, flags); // Constructor throws an =20
>exception
> // Assignment to pFile never happens.
> delete pFile; // Never get here.
> }
> catch(CFileException e)
> {
> TRACE("Caught an Exception")
> }
>}
>
>The constructor throws an exception and pFile is never assigned. On =20
>quitting the program, a 16 byte CObject memory leak is detected. =20
> _CrtSetBreakAlloc() confirms that this is the object created by the new =
=20
>CFile.
The first problem you are having (why you are throwing the exception in the
first place) is that=20
you are trying to open a file that does not exist for reading
("NoFile.txt"). If the file does not exist then you should be using a
CFile::modeCreate. If your function does not know whether a file exists
initially and _cares_ about it, then you should call findfirst() (or
whatever flavor is currently in vogue) then set the flag to modeCreate or
modeRead. There are other ways around this, I won't go into them... you
should go back a study your CFile class for other options.
The second problem is that you're putting your delete pFile in the try
rather than the catch. As soon as you get an exception in your try, it will
jump to the catch. Really, you should not be putting the delete in the
catch either as it should go at the end of the function or the end of the
program. I am assuming that you are using new so that the CFile object
survives the scope of the current function... so it should go wherever you
are cleaning up your program before termination.
//---------------------------------------------------
// =B5=DF -- Michael Bergman
// Datatree Corporation
// Voice: (619) 231-3300 x130
// Fax: (619) 231-3301
// mbergman@datatree.com
//=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D
// Fear? I don't know the meaning of the word fear!
// Come to think of it, I don't know the meaning of=20
// the word courage either! --Groucho Marx
//---------------------------------------------------
-----From: "Giles R. Chamberlin"
Thanks to all who've responded to my enquiry. I've learnt a lot,
specifically:
- no matter how many times you check, the code you post is never the
version you meant to. Catching
CFileException rather than a pointer was just plain wrong!
- delete this. I was getting blinded by the trees. Yup, it would be a
disaster to try to free the memory twice
if the object was allocated on the stack.
- and sometimes, just sometimes, the bugs may not be (totally) down to
me.
So, wiser and almost confident of handling exceptions without someone to
hold my hand I proceed.
Thanks,
Giles Chamberlin
Softwright
-----From: Mike Blaszczak
I did a little debugging with respect to this problem. A lot of the responses
posted to the list for this question are just plain wrong, I'm afraid.
It's recently come to my attention that people think I'm a "Pr*ck" for
trying to teach people why what they think is wrong isntead of just coughing
up answers, so I'll try to stick to regurgitating facts here without any
reason or background. All of this is spelled out if you look in the right
places, but explaining the details of the documentation also makes me a "Pr*ck".
So: the code the original poster provided was flawed--it would leak memory
no matter what because it never cleaned up the CException object that was
thrown. (When you catch a CException-derived exception, you need to call
the Delete() member of that object to clean up the exception.) Most people
who responded didn't point this out.
Even doing that, though, the code still leaks memory. After applying the
fix, the code could look something like this:
void CMyDialog::OnOK()
{
UINT uFlags = CFile::modeCreate;
try {
CFile* pFile = new CFile("This file ain't there.txt", uFlags);
// party on the file
pFile->Close();
delete pFile;
} catch (CException* pEX)
{
pEX->ReportError(); // tell the user
pEX->Delete();
}
}
this code would live in an MFC program. There's nothing wrong with this
code: it has zero defects and can be registered with ISO-9001 as such.
You can't use a special value (eg, NULL) to initialize pFile and notice
that it was ever allocated. Since the constructor is run during the call
to new up there, if the constructor throws an exception, new never
returns and therefore never assigns its valuie to pFile. This approach
just assures that pFile is NULL all the time.
The ANSI UnStandard committtee finally spelled out what should happen in
this case about six months ago. They said that the compiler has to make
sure that the memory gets deleted before the exception is actually
thrown. Nobody in the world (except, maybe, the memory manager) has
a copy of the pointer that new actually allocated. Only the compiler
(for a brief little heartbeat) and runtime library know what happend,
so they must be the ones to chunk out code to delete the problem.
Since the UnStandard folks finally specified this only recently, you
need to have VC++ 4.1 or VC++ 4.2 to get the cleanup code. The cleanup
code _might_ be in VC++ 4.0, but I really can't remember. If you
have those versions, you can create a trivial program that shows this
mechanism really does, indeed, work. Try this, for example:
// compile me with
// cl /MTd /GX thisfile.cpp
#ifndef _DEBUG
#error This program is usless without _DEBUG. Use /MTd or /MDd.
#endif
#include
#include
class CExplosion
{
char szSwollen[1234]; // make our size very big and identifiable
public:
CExplosion() { throw 39; }
~CExplosion() { }
};
void tester()
{
try
{
CExplosion* pEx = new CExplosion;
delete pEx; // maybe it didn't throw
}
catch (int x)
{
printf("Got exception %d\n", x);
}
}
void main()
{
_CrtMemState s1, s2, s3;
_CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE);
_CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDOUT);
_CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE);
_CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDOUT);
_CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE);
_CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDOUT);
_CrtMemCheckpoint(&s1);
tester();
_CrtMemCheckpoint(&s2);
if (_CrtMemDifference(&s3, &s1, &s2))
_CrtMemDumpStatistics(&s3);
_CrtDumpMemoryLeaks();
}
You'll see that nothing is dumped by the CRTL's diagnostics. If you
know assembler language, you can disassemble the code the compiler
makes for the tester() function and see that it's tossing around
some flags to make sure there is no problem.
The rub comes in when you add what most MFC modules have in
the first few lines of their code. It looks something like this:
#ifdef _DEBUG
#define new DEBUG_NEW
#undef THIS_FILE
static char THIS_FILE[] = __FILE__;
#endif
If you swipe the declaration of DEBUG_NEW that MFC has in AFX.H
(around line 1580 in MFC 4.2), which is just one more define that
looks like this:
#define DEBUG_NEW new(THIS_FILE, __LINE__)
and you'll also need the operator new override that's provided
in AFXMEM.H, too:
void* __cdecl operator new(size_t nSize, char* pszFileName, int nLine)
{
return ::operator new(nSize, _NORMAL_BLOCK, pszFileName, nLine);
}
and add it _before_ everything we have above (but after,
of course, the #include directives), you'll be accurately simulating
the MFC compile-time environment. And, if you recompile the test
program above with those changes, you will see that the program
_DOES_ leak memory.
There is _no_ bug in the CFile constructor. MFC has every right
to expect that the compiler behaves within the standard, and it
doesn't, so we spring a leak.
Even given this discussion, though, there _could_ have been a bug
in the constructor that _would_ cause a leak. As someone else
pointed out, only completely constructed objects are destructed.
That means, in the example of CFile, if the CFIle constructor throws
an exception, it isn't fully constructed and therefore the CFile
destructor will not run in that case. However, since CFile derives
from CObject, and the CObject constructor has run fully (because
objects are constructed from "bottom to top"), you know that the
CObject destructor will run if CFile throws an exception but
the CFile destructor _won't_ run.
So, you could make a bug:
class CExplosion
{
char* pszBuffer;
public:
CExplosion() {
pszBuffer = new char[4096];
if (some_condition)
throw 125;
}
~CExplosion() {
delete [] pszBuffer;
}
};
CExplosion has a bad constructor becaue it will leak 4096 bytes each
time it is called and throws an exception. The memory allocated by
the constructor isn't freed because the destructor never runs if the
exception is thrown. The proper way to code this is:
class CExplosion
{
char* pszBuffer;
public:
CExplosion() {
pszBuffer = new char[4096];
if (some_condition)
{
delete [] pszBuffer; // won't do it later
throw 125;
}
}
~CExplosion() {
delete [] pszBuffer;
}
};
Anyway, back to the issue at hand: with the DEBUG_NEW macro hooked
up, the code ends up calling an override of global operator new that
knows how to track file names and line numbers for leaked memory.
In my example, the call
CExplosion* pEx = new CExplosion;
ends up invoking those macros to eventually resolve to something like
CExplosion* pEx = new("thisfile.cpp", 52) CExplosion;
which gives the new operator the information it needs to track the
problem. But that override causes the compiler to forget that it
needs to emit the cleanup code.
So, here are your choices:
1) Change your code to not use dynamically allocated CFile's. CFile
was written back when the compiler didn't understand exceptions
_at all_, so there are ways to use the class that don't rely on
any of these mechanisms.
2) Change your code so that it doesn't use DEBUG_NEW. You'll still
get leak detection, but the leaks won't be identified by line
number and file-name. It's up to you to decide if that's more or
less important than phony leaks.
It's worth noting that, when you compile for release mode, the
new-opreator defined-symbol switchorama isn't in effect so you
won't have this problem in non-_DEBUG builds. That is, there's
no reason to sweat this bug for shipping code.
The compiler is clearly at fault here: I'm working with someone on
the compiler team to make sure the problem gets fixed up. We might
find a work around that we can tuck into MFC 4.2a, but I don't
think it looks very promising right now. Anything can happen, though.
.B ekiM
http://www.nwlink.com/~mikeblas <--- trip report central
1995 Honda VFR750F (Serial number 00050!) 4 Corners 1996!
1987 Yamaha FZ700 (damaged) AMA, HRC, VFROC
These words are my own: I do not speak for Microsoft.
Ash Williams -- ash@digitalworkshop.co.uk
Tuesday, August 27, 1996
Mike,
Personally I don't think anyone's a prick for giving helpful hints and
advice.
As for CFile, my strategy is not to bother to call the buggy
constructor, which doesn't clear up after itself, rather I use the Open
function:
extern g_name;
extern g_flags;
void Yo
{
CFileException e; // use this information if you want to
CFile* pFile = new CFile; // ctor never throws ( but new might )
if( pFile -> Open( g_name, g_flags, &e ) ) // Open never throws
{
// success!
}
else
{
pFile -> Abort(); // Abort never throws
FailMessage();
}
delete pFile;
}
Ash
Pradeep Tapadiya -- pradeep@nuview.com
Thursday, August 29, 1996
Hi,
Just wanted to point it out that when "new" throws an exception,
pFile is not pointing to NULL. It is pointing to whatever
location it picked up on the stack. Therefore, delete pFile
may produce some unexpected results.
Please take this as a constructive criticism.
Pradeep
pradeep@nuview.com
>void Yo
>{
> CFileException e; // use this information if you want to
>
> CFile* pFile = new CFile; // ctor never throws ( but new might )
> if( pFile -> Open( g_name, g_flags, &e ) ) // Open never throws
> {
> // success!
> }
> else
> {
> pFile -> Abort(); // Abort never throws
> FailMessage();
> }
>
> delete pFile;
>}
>
>Ash
>
>
Bradley V. Pohl -- brad.pohl@pobox.com
Saturday, August 31, 1996
>Just wanted to point it out that when "new" throws an exception,
>pFile is not pointing to NULL. It is pointing to whatever
>location it picked up on the stack. Therefore, delete pFile
>may produce some unexpected results.
>Please take this as a constructive criticism.
Some meta-level contructive criticism: you probably should have looked at the code
a bit more carefully before issuing your "contructive criticism."
If operator new throws an exception, the "delete pfile" line will _not_ be executed because
no try {} block is present to catch the thrown exception. The call stack will continue
to unwind past the body of this function.
--Brad
>Pradeep
>pradeep@nuview.com
>void Yo
>{
> CFileException e; // use this information if you want to
>
> CFile* pFile = new CFile; // ctor never throws ( but new might )
> if( pFile -> Open( g_name, g_flags, &e ) ) // Open never throws
> {
> // success!
> }
> else
> {
> pFile -> Abort(); // Abort never throws
> FailMessage();
> }
>
> delete pFile;
>}
>
>Ash
Pradeep Tapadiya -- pradeep@nuview.com
Monday, September 02, 1996
At 09:35 PM 8/31/96 -0400, you wrote:
>>Just wanted to point it out that when "new" throws an exception,
>>pFile is not pointing to NULL. It is pointing to whatever
>>location it picked up on the stack. Therefore, delete pFile
>>may produce some unexpected results.
>
>>Please take this as a constructive criticism.
>
>Some meta-level contructive criticism: you probably should have looked at
the code
>a bit more carefully before issuing your "contructive criticism."
>
>If operator new throws an exception, the "delete pfile" line will _not_ be
executed because
>no try {} block is present to catch the thrown exception. The call stack
will continue
>to unwind past the body of this function.
>
I realized this just after I sent the e-mail out. My apologies. Guess I
made a fool of myself :(.
Pradeep
| Вернуться в корень Архива
|