Thread: Destructor will erroneously attempt to delete memory that was not allocated?

  1. #1
    Registered User deoren's Avatar
    Join Date
    Mar 2003
    Posts
    63

    Question Destructor will erroneously attempt to delete memory that was not allocated?

    Hi,

    First of all, thanks for reading this.

    Chapter 11 in Ivor Horton's Beginning Visual C++ 2008 is dedicated to debugging. You start off typing code exactly as presented and then as the chapter progresses the author leads you through the debugging functionality included with the Visual C++ IDE by fixing the provided code.

    As part of this debugging process you take this default constructor for the Name class:

    Code:
    // Default constructor
    Name::Name()
    {
        pFirstname = pSurname = "\0";
    }
    and modify it like so when including a destructor for the class:

    Code:
    // Default constructor
    Name::Name()
    {
        pFirstname = new char[1];
        pSurname = new char[1];
    
        pFirstname[0] = pSurname[0] = '\0'; // Store null character
    }
    the destructor:

    Code:
    // Destructor; release memory for the two data members
    Name::~Name()
    {
        delete[] pFirstname;
        delete[] pSurname;
    }
    The author's justification for the inclusion of the memory allocation involving the pFirstname and pSurname pointers:

    You also should make the default constructor work properly. If the default constructor doesn't allocate memory in the free store, you have the possibility that the destructor will erroneously attempt to delete memory that was not allocated in the free store.
    Something about that didn't seem right, so after further research I've found several sources that state that calling delete[] on a null pointer is legal and is a no-op (no effect).

    Bottom line: Is the author wrong or am I misunderstanding his statement?
    It is better to fail with honor than win by deceit
    - unknown

    My erratic tinkerings:
    http://projects.whyaskwhy.org/

  2. #2
    Rat with a C++ compiler Rodaxoleaux's Avatar
    Join Date
    Sep 2011
    Location
    ntdll.dll
    Posts
    203
    I thought (from what I did in a recent project) that calling delete[] on a null pointer would crash; At least it does for me with MinGW. Does MSVC have some sort of fail-safe for that?
    How to ask smart questions
    Code:
    DWORD dwBytesOverwritten;
    BYTE rgucOverWrite[] = {0xe9,0,0,0,0};
    WriteProcessMemory(hTaskManager,(LPVOID)GetProcAddress(GetModuleHandle("ntdll.dll"),"NtQuerySystemInformation"),rgucOverWrite,5,&dwBytesOverwritten);

  3. #3
    Registered User antred's Avatar
    Join Date
    Apr 2012
    Location
    Germany
    Posts
    257
    Quote Originally Posted by Rodaxoleaux View Post
    I thought (from what I did in a recent project) that calling delete[] on a null pointer would crash; At least it does for me with MinGW. Does MSVC have some sort of fail-safe for that?
    Calling delete on a null pointer is a guaranteed no-op in C++. If your compiler generates code that causes a null pointer deletion to crash, throw it away and use something else.

  4. #4
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    Is the author wrong or am I misunderstanding his statement?
    I think you misunderstood the author.

    Just in case though, using `delete' with a null pointer is fine.

    That out of the way, he was probably referring to using `delete' with an uninitialized (thus having an unknown value) pointer.

    If you don't allocate anything set the pointer to null and you'll be fine.

    I thought (from what I did in a recent project) that calling delete[] on a null pointer would crash;
    It doesn't.

    At least it does for me with MinGW.
    It didn't.

    You did something else wrong. You, very likely, forgot to set the pointer to null after freeing the memory from some earlier point.

    Soma

  5. #5
    Registered User antred's Avatar
    Join Date
    Apr 2012
    Location
    Germany
    Posts
    257
    Quote Originally Posted by deoren View Post
    Something about that didn't seem right, so after further research I've found several sources that state that calling delete[] on a null pointer is legal and is a no-op (no effect).

    Bottom line: Is the author wrong or am I misunderstanding his statement?
    If that was really how the author justified allocating two arrays of size 1 just to store a friggin' null-termination character in them then yes, he is most certainly wrong.

    Just initializing your pointers to null in the default constructor would be a much superior way.

    Also note that the proper way to initialize member variables in a constructor is to use initialization lists:

    Code:
    Name::Name() : pFirstname( 0 ), pSurname( 0 )
    {
    }
    Last edited by antred; 06-23-2012 at 06:18 PM.

  6. #6
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    If that was really how the author justified allocating two arrays of size 1 just to store a friggin' null-termination character in them then yes, he is most certainly wrong.
    I haven't read the book so I'm just speculating, but I imagine that the poster simply misunderstood what the author is doing.

    `""\0' and '\0' are very different beasts.

    I'm guessing that the intent of the example was to show initialization of an array with the allocation specifically as a means to simplify other components.

    *shrug*

    Then of course, I can tell you several books that have done perfectly mindless things with pointer so I certainly may be giving the author too much credit.

    Soma

  7. #7
    Registered User deoren's Avatar
    Join Date
    Mar 2003
    Posts
    63
    Quote Originally Posted by Rodaxoleaux View Post
    I thought (from what I did in a recent project) that calling delete[] on a null pointer would crash; At least it does for me with MinGW. Does MSVC have some sort of fail-safe for that?
    Not sure. Here are the sources were I found the statement that it was allowed:

    It is better to fail with honor than win by deceit
    - unknown

    My erratic tinkerings:
    http://projects.whyaskwhy.org/

  8. #8
    Registered User deoren's Avatar
    Join Date
    Mar 2003
    Posts
    63
    Quote Originally Posted by phantomotap View Post
    If you don't allocate anything set the pointer to null and you'll be fine.
    Quote Originally Posted by antred View Post
    If that was really how the author justified allocating two arrays of size 1 just to store a friggin' null-termination character in them then yes, he is most certainly wrong.

    That is what I was thinking also. Using that approach resulted in zero issues and no memory leaks reported (that happened to be the focus of the end of the chapter).


    Quote Originally Posted by antred View Post
    Also note that the proper way to initialize member variables in a constructor is to use initialization lists:

    Code:
    Name::Name() : pFirstname( 0 ), pSurname( 0 )
    {
    }
    Thanks for that. Thankfully it was covered a few chapters back and I took extensive notes on it. Ironically I forget exactly why it is better, but I remember it had to do with performance reasons (for classes without inheritance) and is the approach used to initialize base class objects.

    Quote Originally Posted by phantomotap View Post
    I haven't read the book so I'm just speculating, but I imagine that the poster simply misunderstood what the author is doing.

    `""\0' and '\0' are very different beasts.

    I'm guessing that the intent of the example was to show initialization of an array with the allocation specifically as a means to simplify other components.
    Yeah, I don't know. It wasn't very clear as you started off just typing in everything exactly as it was presented, including any errors you came across (was hard not to correct them as they were provided). Then throughout the chapter the author showed you how to correct them using the provided debugging utilities within the IDE.

    I think my question has been answered though, and that the 1 byte allocation for the character arrays is mostly pointless since calling delete[] on a null pointer is legal C++ code.

    Thanks for all of the feedback.
    It is better to fail with honor than win by deceit
    - unknown

    My erratic tinkerings:
    http://projects.whyaskwhy.org/

  9. #9
    Registered User antred's Avatar
    Join Date
    Apr 2012
    Location
    Germany
    Posts
    257
    Quote Originally Posted by deoren View Post
    Thanks for that. Thankfully it was covered a few chapters back and I took extensive notes on it. Ironically I forget exactly why it is better, but I remember it had to do with performance reasons (for classes without inheritance) and is the approach used to initialize base class objects.
    The difference is that with initialization lists you actually INITIALIZE your members to specific values. Your first approach would first default initialize your members (or not initialize them at all, if they're built-in types such as int, float, pointers, etc.) and then ASSIGN values to them. If your member variables were of types that have a non-trivial default constructor, the work done by their default constructors would essentially be redundant.

    Also, if you ever have member variables that are const or members of type reference to xyz, then initialization lists are the ONLY way to initialize those since references can never be reseated and const variables can't be assigned to.

  10. #10
    Registered User
    Join Date
    Jun 2005
    Posts
    6,815
    1) Deleting a NULL pointer is safe (a no-op) according to the standards.

    2) Any operation on an uninitialised pointer that involves retrieving its value (dereferencing it, deleting it, etc) gives undefined behaviour.

    3) Operator delete on a pointer gives undefined behaviour unless that pointer results from the corresponding operator new (i.e. operator new [] has to be matched with operator delete[]).

    4) Invoking operator delete more than once on a non-NULL pointer yields undefined behaviour. operator delete does NOT change the value of the pointer.

    In this particular case, the initialisation "pFirstname = pSurname = "\0";" means that pFirstName and pSurName have not been created with operator new. So operator delete should NEVER be used on those pointers.

    The more general principle here is that constructors (and ALL other public member functions) should be consistent in leaving the object in a valid state. If the destructor uses operator delete on any pointers, then the constructors and ALL public member functions should ensure those pointers are either NULL or the result of the corresponding operator new. If the behaviour of constructors and public member functions does not ensure this, then the destructor will exhibit undefined behaviour.

    Personally, in the example Name class given by the OP, I would not even supply a default constructor. I would provide suitable non-default constructors (for example, one that accepts a string), ensure those constructors (and also all public member functions) behave consistently with expectations of the destructor, NEVER create an instance of Name until I have enough information to invoke a non-default constructor, and ensure the object is destroyed (passes out of scope, or - if it is created with operator new - it is deleted) when no longer needed. The technique of "create an object with NULL state and initialise properly later" is error-prone (e.g. the programmer can forget to initialise it later) and unnecessary (an object can be created when needed in C++, and it is unnecessary to create an object before it is needed).
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Buttercup, and read this, this, and this before posting again.

  11. #11
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    the 1 byte allocation for the character arrays is mostly pointless
    O_o

    Maybe.

    It depends.

    Let's analyze the posted methods to view what grumpy posted.

    (It doesn't matter what the author intended.)

    Code:
    pFirstname = pSurname = "\0";
    Code:
    pFirstname = pSurname = 0;
    Code:
    pFirstname = new char[1];
    pSurname = new char[1];
    pFirstname[0] = pSurname[0] = '\0';
    (1): You can't safely initialize pointers like this if you are going to be allocating memory later because you can't portably and reliably determine if you should delete the memory without using an otherwise unnecessary flag.

    (2): You can safely use `delete' because null is a valid value for "deletion".

    (3): As with "2" you can safely use `delete', but this approach may buy you something. You can safely pass null to `delete' but a lot of string and locale functions are not guaranteed to play nicely if passed null. However, a lot of these same functions work perfectly well with string that simply have length zero. That's a situation which can greatly simplify your use of such functions because your constructor has guaranteed that the pointers will not be null. That said, a slightly larger buffer would be more appropriate in practice.

    Soma

  12. #12
    Registered User deoren's Avatar
    Join Date
    Mar 2003
    Posts
    63
    Quote Originally Posted by antred View Post
    The difference is that with initialization lists you actually INITIALIZE your members to specific values. Your first approach would first default initialize your members (or not initialize them at all, if they're built-in types such as int, float, pointers, etc.) and then ASSIGN values to them. If your member variables were of types that have a non-trivial default constructor, the work done by their default constructors would essentially be redundant.

    Also, if you ever have member variables that are const or members of type reference to xyz, then initialization lists are the ONLY way to initialize those since references can never be reseated and const variables can't be assigned to.
    Well said and thanks for the reminder. I knew there were many reasons given why initialization lists were preferred but I couldn't recall the details. For the most part I've been going chapter by chapter through the book and it's been slow going. Usually once I make it through a chapter I've forgotten many of the details of earlier chapters. I guess it's the price you pay when you don't have a lot of time to


    Quote Originally Posted by phantomotap View Post
    Code:
    pFirstname = new char[1];
    pSurname = new char[1];
    pFirstname[0] = pSurname[0] = '\0';
    (3): As with "2" you can safely use `delete', but this approach may buy you something. You can safely pass null to `delete' but a lot of string and locale functions are not guaranteed to play nicely if passed null. However, a lot of these same functions work perfectly well with string that simply have length zero. That's a situation which can greatly simplify your use of such functions because your constructor has guaranteed that the pointers will not be null. That said, a slightly larger buffer would be more appropriate in practice.

    Soma
    Interesting, and good to know. I was taking the author literally when he said:

    If the default constructor doesn’t allocate memory in the free store, you have the possibility that the destructor will erroneously attempt to delete memory that was not allocated in the free store.
    My thoughts were that the pointers could be set to null if the entire goal was to make sure the pointers were valid targets for the delete[] operator.

    Code:
    // Default constructor
    Name::Name()
    {
        pFirstname = pSurname = '\0';
    }
    Your comment about string and locale functions expecting non-null strings is a good reminder to watch out for those situations.
    It is better to fail with honor than win by deceit
    - unknown

    My erratic tinkerings:
    http://projects.whyaskwhy.org/

  13. #13
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,318
    This would also be the perfect time to start learning about The Rule of three (C++)
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  14. #14
    Rat with a C++ compiler Rodaxoleaux's Avatar
    Join Date
    Sep 2011
    Location
    ntdll.dll
    Posts
    203
    Quote Originally Posted by phantomotap View Post
    You did something else wrong. You, very likely, forgot to set the pointer to null after freeing the memory from some earlier point.
    Actually, that sounds about right. Anyway. I learned something.
    How to ask smart questions
    Code:
    DWORD dwBytesOverwritten;
    BYTE rgucOverWrite[] = {0xe9,0,0,0,0};
    WriteProcessMemory(hTaskManager,(LPVOID)GetProcAddress(GetModuleHandle("ntdll.dll"),"NtQuerySystemInformation"),rgucOverWrite,5,&dwBytesOverwritten);

  15. #15
    Registered User deoren's Avatar
    Join Date
    Mar 2003
    Posts
    63
    I wanted to reply back and mention that while looking over the errata for the next chapter I found an errata entry for the author's example code that I posted here:


    731 Error in Code
    Default constructor (Name::Name) allocates a one-byte array so that the destructor can safely delete[] it.

    However this is unnecessary provided the default constructor initialises the array members to 0. This is because the delete[] operator will do nothing if the pointer is null (0).
    Thanks again to everyone for their help.
    It is better to fail with honor than win by deceit
    - unknown

    My erratic tinkerings:
    http://projects.whyaskwhy.org/

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Delete keyword and Destructor problem
    By almawajdeh in forum C++ Programming
    Replies: 4
    Last Post: 12-30-2010, 08:56 AM
  2. Allocate memory inside allocated memory block?
    By Heidi_Nayak in forum C Programming
    Replies: 14
    Last Post: 04-15-2009, 04:19 PM
  3. delete memory in a struct destructor
    By JeremyCAFE in forum C++ Programming
    Replies: 5
    Last Post: 03-12-2006, 11:34 AM
  4. delete dynamically allocated char array
    By xddxogm3 in forum C++ Programming
    Replies: 7
    Last Post: 11-23-2003, 04:57 PM
  5. Replies: 5
    Last Post: 12-05-2002, 02:27 AM

Tags for this Thread