Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C++ > check for deleted map entry -> crash ?

Reply
Thread Tools

check for deleted map entry -> crash ?

 
 
bolnvhuis@wanadoo.nl
Guest
Posts: n/a
 
      04-27-2006
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();cyclicRtdmI t!=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 ?

 
Reply With Quote
 
 
 
 
Rolf Magnus
Guest
Posts: n/a
 
      04-27-2006
http://www.velocityreviews.com/forums/(E-Mail Removed) 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();cyclicRtdmI t!=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().

 
Reply With Quote
 
 
 
 
Andrei Tarassov
Guest
Posts: n/a
 
      04-27-2006
> 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

 
Reply With Quote
 
Rolf Magnus
Guest
Posts: n/a
 
      04-27-2006
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.

 
Reply With Quote
 
Roland Pibinger
Guest
Posts: n/a
 
      04-27-2006
On Thu, 27 Apr 2006 12:34:27 +0300, Andrei Tarassov
<(E-Mail Removed)> 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

 
Reply With Quote
 
Axter
Guest
Posts: n/a
 
      04-27-2006

(E-Mail Removed) 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.

 
Reply With Quote
 
Roland Pibinger
Guest
Posts: n/a
 
      04-27-2006
On 27 Apr 2006 12:40:16 -0700, "Axter" <(E-Mail Removed)> 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
 
Reply With Quote
 
bolnvhuis@wanadoo.nl
Guest
Posts: n/a
 
      04-27-2006

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.

 
Reply With Quote
 
Axter
Guest
Posts: n/a
 
      04-28-2006

Roland Pibinger wrote:
> On 27 Apr 2006 12:40:16 -0700, "Axter" <(E-Mail Removed)> 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...b02795cf6a8c39

The smart pointer is specifically made to be used with the STL
containers, and it's far safer to use than using raw pointers.

 
Reply With Quote
 
Aman Angrish
Guest
Posts: n/a
 
      04-28-2006

> 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
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Re: Are deleted pictures REALLY deleted? khoshhon@gmail.com Computer Support 3 01-01-2009 08:53 AM
Re: Are deleted pictures REALLY deleted? John Holmes Computer Support 7 12-31-2008 04:46 PM
Re: Are deleted pictures REALLY deleted? Bucky Breeder Computer Support 0 12-31-2008 02:58 PM
Need Help Rebuilding Registry Deleted Userinit Entry flounderino Computer Support 0 12-31-2004 07:16 AM
Record not being deleted in dbase, even tho the display on datagrid is deleted.. Chumley the Walrus ASP .Net Web Controls 2 08-10-2004 02:23 PM



Advertisments