check for deleted map entry -> crash ?

Discussion in 'C++' started by bolnvhuis@wanadoo.nl, Apr 27, 2006.

  1. Guest

    I'm using an STL map in my code.
    My application sometimes tries to delete things twice from the map.
    This leads to a crash in my current code. The problem is probably the
    way I check whether it is necessary to delete something (line 44 and 52
    in below code).
    In the below code the problem is demonstrated, line 65 will cause a
    segmentation fault.


    1 #include <iostream>
    2 #include <cstdlib>
    3 #include <map>
    4
    5 using namespace std;
    6
    7 class MyClass
    8 {
    9 public:
    10 MyClass(int);
    11 ~MyClass();
    12 void show();
    13 private:
    14 int nr_;
    15 };
    16
    17 MyClass::MyClass(int nr) : nr_(nr)
    18 {
    19 cout << "MyClass: " << nr_ << " constructed" << endl;
    20 }
    21
    22 MyClass::~MyClass()
    23 {
    24 cout << "MyClass: " << nr_ << " destructed" << endl;
    25 }
    26
    27 void MyClass::show()
    28 {
    29 cout << "Show this MyClass: " << nr_ << endl;
    30 }
    31
    32 typedef map<int, MyClass *> RtdmCyclicTimerMap;
    33
    34 int main(void)
    35 {
    36 RtdmCyclicTimerMap cyclicRtdmTimer_;
    37 map<int, MyClass *>::const_iterator cyclicRtdmIt;
    38
    39 // create 1 & 2
    40 cyclicRtdmTimer_[1] = new MyClass(11);
    41 cyclicRtdmTimer_[2] = new MyClass(22);
    42
    43 // delete 2
    44 if (cyclicRtdmTimer_[2]) {
    45 delete cyclicRtdmTimer_[2];
    46 cyclicRtdmTimer_.erase(2);
    47 } else {
    48 cout << "delete 2 failed" << endl;
    49 }
    50
    51 // delete 2 AGAIN
    52 if (cyclicRtdmTimer_[2]) {
    53 delete cyclicRtdmTimer_[2];
    54 cyclicRtdmTimer_.erase(2);
    55 } else {
    56 cout << "delete 2[2] failed" << endl;
    57 }
    58
    59 // show map -> NOW IT WILL CRASH
    60 cout << endl << "going to show map entries" << endl;
    61 for
    (cyclicRtdmIt=cyclicRtdmTimer_.begin();cyclicRtdmIt!=cyclicRtdmTimer_.end();
    ++cyclicRtdmIt) {
    62 int tKey = cyclicRtdmIt->first;
    63 MyClass *mcPtr = cyclicRtdmIt->second;
    64 cout << "map entry found: key=" << tKey << ", valP=" <<
    mcPtr << ", val=";
    65 mcPtr->show();
    66 }
    67 }
    68


    This is because line 52 somehow increases the internal map size from 1
    to 2 entries.
    Isn't it allowed to refer to deleted entries ?
    Must it be done with an iterator ?
     
    , Apr 27, 2006
    #1
    1. Advertising

  2. Rolf Magnus Guest

    wrote:

    > I'm using an STL map in my code.
    > My application sometimes tries to delete things twice from the map.
    > This leads to a crash in my current code. The problem is probably the
    > way I check whether it is necessary to delete something (line 44 and 52
    > in below code).


    Yes.

    > In the below code the problem is demonstrated, line 65 will cause a
    > segmentation fault.
    >
    >
    > 1 #include <iostream>
    > 2 #include <cstdlib>
    > 3 #include <map>
    > 4
    > 5 using namespace std;
    > 6
    > 7 class MyClass
    > 8 {
    > 9 public:
    > 10 MyClass(int);
    > 11 ~MyClass();
    > 12 void show();
    > 13 private:
    > 14 int nr_;
    > 15 };
    > 16
    > 17 MyClass::MyClass(int nr) : nr_(nr)
    > 18 {
    > 19 cout << "MyClass: " << nr_ << " constructed" << endl;
    > 20 }
    > 21
    > 22 MyClass::~MyClass()
    > 23 {
    > 24 cout << "MyClass: " << nr_ << " destructed" << endl;
    > 25 }
    > 26
    > 27 void MyClass::show()
    > 28 {
    > 29 cout << "Show this MyClass: " << nr_ << endl;
    > 30 }
    > 31
    > 32 typedef map<int, MyClass *> RtdmCyclicTimerMap;
    > 33
    > 34 int main(void)
    > 35 {
    > 36 RtdmCyclicTimerMap cyclicRtdmTimer_;
    > 37 map<int, MyClass *>::const_iterator cyclicRtdmIt;
    > 38
    > 39 // create 1 & 2
    > 40 cyclicRtdmTimer_[1] = new MyClass(11);
    > 41 cyclicRtdmTimer_[2] = new MyClass(22);
    > 42
    > 43 // delete 2
    > 44 if (cyclicRtdmTimer_[2]) {
    > 45 delete cyclicRtdmTimer_[2];
    > 46 cyclicRtdmTimer_.erase(2);
    > 47 } else {
    > 48 cout << "delete 2 failed" << endl;
    > 49 }
    > 50
    > 51 // delete 2 AGAIN
    > 52 if (cyclicRtdmTimer_[2]) {
    > 53 delete cyclicRtdmTimer_[2];
    > 54 cyclicRtdmTimer_.erase(2);
    > 55 } else {
    > 56 cout << "delete 2[2] failed" << endl;
    > 57 }
    > 58
    > 59 // show map -> NOW IT WILL CRASH
    > 60 cout << endl << "going to show map entries" << endl;
    > 61 for
    >

    (cyclicRtdmIt=cyclicRtdmTimer_.begin();cyclicRtdmIt!=cyclicRtdmTimer_.end();
    > ++cyclicRtdmIt) {
    > 62 int tKey = cyclicRtdmIt->first;
    > 63 MyClass *mcPtr = cyclicRtdmIt->second;
    > 64 cout << "map entry found: key=" << tKey << ", valP=" <<
    > mcPtr << ", val=";
    > 65 mcPtr->show();
    > 66 }
    > 67 }
    > 68
    >
    >
    > This is because line 52 somehow increases the internal map size from 1
    > to 2 entries.


    If operator[] doesn't find the element you are searching for, it creates
    that element. Otherwise, line 40 and 41 couldn't work the way they do.
    Since you don't assign anything to it in line 52, the value is probably
    indeterminate and cannot be used with delete.

    > Must it be done with an iterator ?


    Yes. Use the find() member function instead of operator[]. This will never
    create elements. If the element is found, you get an iterator to it,
    otherwise you get end().
     
    Rolf Magnus, Apr 27, 2006
    #2
    1. Advertising

  3. > 51 // delete 2 AGAIN
    > 52 if (cyclicRtdmTimer_[2]) {


    The problem is here. This implicitly adds an element to the map and
    initializes the pointer with NULL. When you try to dereference that pointer
    later, you get a segfault.

    You could use the following check instead:
    if (cyclicRtdmTimer_.find(2) != cyclicRtdmTimer_.end()) {

    Best wishes,
    --
    Andrei Tarassov
    software engineer
    Altiris, Inc.
    www.altiris.com
     
    Andrei Tarassov, Apr 27, 2006
    #3
  4. Rolf Magnus Guest

    Rolf Magnus wrote:

    > If operator[] doesn't find the element you are searching for, it creates
    > that element. Otherwise, line 40 and 41 couldn't work the way they do.
    > Since you don't assign anything to it in line 52, the value is probably
    > indeterminate and cannot be used with delete.


    Ok, forget that. Andrei is right. The newly added pointer is initialized to
    a null pointer, and since you only remove it if it's not a null pointer, it
    stays in your map. Later, when you try to dereference it, your program
    crashes.
     
    Rolf Magnus, Apr 27, 2006
    #4
  5. On Thu, 27 Apr 2006 12:34:27 +0300, Andrei Tarassov
    <> wrote:

    >> 51 // delete 2 AGAIN
    >> 52 if (cyclicRtdmTimer_[2]) {

    >
    >The problem is here. This implicitly adds an element to the map and
    >initializes the pointer with NULL. When you try to dereference that pointer
    >later, you get a segfault.


    Examples like the above demonstrate that STL is designed for values
    only, not for pointers. Unfortunately that fact is not conveyed in
    most STL books and tutorials.

    Best wishes,
    Roland Pibinger
     
    Roland Pibinger, Apr 27, 2006
    #5
  6. Axter Guest

    wrote:
    > I'm using an STL map in my code.
    > My application sometimes tries to delete things twice from the map.
    > This leads to a crash in my current code. The problem is probably the
    > way I check whether it is necessary to delete something (line 44 and 52
    > in below code).
    > In the below code the problem is demonstrated, line 65 will cause a
    > segmentation fault.
    >
    >
    > 1 #include <iostream>
    > 2 #include <cstdlib>
    > 3 #include <map>
    > 4
    > 5 using namespace std;
    > 6
    > 7 class MyClass
    > 8 {
    > 9 public:
    > 10 MyClass(int);
    > 11 ~MyClass();
    > 12 void show();
    > 13 private:
    > 14 int nr_;
    > 15 };
    > 16
    > 17 MyClass::MyClass(int nr) : nr_(nr)
    > 18 {
    > 19 cout << "MyClass: " << nr_ << " constructed" << endl;
    > 20 }
    > 21
    > 22 MyClass::~MyClass()
    > 23 {
    > 24 cout << "MyClass: " << nr_ << " destructed" << endl;
    > 25 }
    > 26
    > 27 void MyClass::show()
    > 28 {
    > 29 cout << "Show this MyClass: " << nr_ << endl;
    > 30 }
    > 31
    > 32 typedef map<int, MyClass *> RtdmCyclicTimerMap;
    > 33
    > 34 int main(void)
    > 35 {
    > 36 RtdmCyclicTimerMap cyclicRtdmTimer_;
    > 37 map<int, MyClass *>::const_iterator cyclicRtdmIt;


    I recommend against using raw dummy pointers in an STL container.
    I recommend you use a smart pointer like the following:
    http://axter.com/smartptr

    The above smart pointer can be used with std::map, and will
    automatically delete the pointee for you.
     
    Axter, Apr 27, 2006
    #6
  7. On 27 Apr 2006 12:40:16 -0700, "Axter" <> wrote:
    >I recommend against using raw dummy pointers in an STL container.
    >I recommend you use a smart pointer like the following:
    >http://axter.com/smartptr
    >
    >The above smart pointer can be used with std::map, and will
    >automatically delete the pointee for you.


    The problem in the above code is an 'automatically' created NULL
    pointer. Is your smart pointer smart enough to avoid that?

    Regards,
    Roland Pibinger
     
    Roland Pibinger, Apr 27, 2006
    #7
  8. Guest

    Andrei Tarassov schreef:

    > > 51 // delete 2 AGAIN
    > > 52 if (cyclicRtdmTimer_[2]) {

    >
    > The problem is here. This implicitly adds an element to the map and
    > initializes the pointer with NULL. When you try to dereference that pointer
    > later, you get a segfault.
    >
    > You could use the following check instead:
    > if (cyclicRtdmTimer_.find(2) != cyclicRtdmTimer_.end()) {
    >


    thx yes, now I see.
    indeed using find() everything works fine.

    It's just that I thought I could use the "operator[]" in a *read-only*
    way.
    I guess I assumed the operator[] usage in the if statement could never
    *write/change* something in the STL map.
    Maybe I'm still a bit too C-minded.

    thanks all for your insights.
     
    , Apr 27, 2006
    #8
  9. Axter Guest

    Roland Pibinger wrote:
    > On 27 Apr 2006 12:40:16 -0700, "Axter" <> wrote:
    > >I recommend against using raw dummy pointers in an STL container.
    > >I recommend you use a smart pointer like the following:
    > >http://axter.com/smartptr
    > >
    > >The above smart pointer can be used with std::map, and will
    > >automatically delete the pointee for you.

    >
    > The problem in the above code is an 'automatically' created NULL
    > pointer. Is your smart pointer smart enough to avoid that?


    Actually yes.

    The smart class has default logic that allows for assignment to a
    default object in such cases. See following link, and comments for
    smart_ptr<T>::set_default_object method:
    http://axter.com/smartptr/classsmart__ptr.htm#bc5f33826025855301b02795cf6a8c39

    The smart pointer is specifically made to be used with the STL
    containers, and it's far safer to use than using raw pointers.
     
    Axter, Apr 28, 2006
    #9
  10. Aman Angrish Guest


    > 44 if (cyclicRtdmTimer_[2]) {
    > 45 delete cyclicRtdmTimer_[2];

    It's good practice to think of operator[] as add/update operator for
    non-const maps.
    Not to be Not to be used as existence checker.

    Regards,
    Aman.



    --
    Posted via Mailgate.ORG Server - http://www.Mailgate.ORG
     
    Aman Angrish, Apr 28, 2006
    #10
  11. Rolf Magnus Guest

    Roland Pibinger wrote:

    > On Thu, 27 Apr 2006 12:34:27 +0300, Andrei Tarassov
    > <> wrote:
    >
    >>> 51 // delete 2 AGAIN
    >>> 52 if (cyclicRtdmTimer_[2]) {

    >>
    >>The problem is here. This implicitly adds an element to the map and
    >>initializes the pointer with NULL. When you try to dereference that
    >>pointer later, you get a segfault.

    >
    > Examples like the above demonstrate that STL is designed for values
    > only, not for pointers.


    No, it doesn't demostrate that. If the OP had stored values in the map, the
    behavior wouldn't be undefined, but the result would still be incorrect (a
    new value added to the map that wasn't supposed to be added).
     
    Rolf Magnus, Apr 28, 2006
    #11
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. AtomicBob
    Replies:
    14
    Views:
    940
    Toby Inkster
    May 2, 2006
  2. will551

    map elements being deleted

    will551, Dec 5, 2006, in forum: C++
    Replies:
    5
    Views:
    334
    will551
    Dec 6, 2006
  3. Mark

    Check If Object Deleted

    Mark, Sep 29, 2007, in forum: C++
    Replies:
    19
    Views:
    656
    James Kanze
    Oct 7, 2007
  4. Chumley the Walrus
    Replies:
    2
    Views:
    266
    Tom Gosselin
    Aug 10, 2004
  5. Mike Owen

    Allowing entry of a Carriage Return during data entry

    Mike Owen, Jul 27, 2006, in forum: ASP .Net Web Controls
    Replies:
    3
    Views:
    752
    Alessandro Zifiglio
    Jul 27, 2006
Loading...

Share This Page