• Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 2470
  • Last Modified:

stl map insert problem

Environment:  Windows 2000, VS6 SP5, default MS stl implementation

I have  a trivial class, implemented in a DLL, as follows:
#ifndef TRIVIAL_H
#define TRIVIAL_H

class AFX_EXT_CLASS aff_data
{
public:
      aff_data();
      aff_data( const aff_data & rhs );
      const aff_data & operator =( const aff_data & rhs );

      int cb_index;
      std::string aff_name;
};

AFX_EXT_CLASS void dump_affiliates( const aff_map_t & aff );   // <- from the header file

typedef std::map<long, aff_data> aff_map_t;

#endif //TRIVIAL_H

In the same DLL is a diagnostic dump function for aff_map_t:

void dump_affiliates( const aff_map_t & aff )
{
      for( aff_map_t::const_iterator iter2 = aff.begin();
                  iter2 != aff.end();
                  iter2++ )
      {
            TRACE1( "aff_num = %ld  ", (*iter2).first );
            TRACE1( "name = '%s'  ", (*iter2).second.aff_name.substr(0,20).c_str() );
            TRACE1( "cb_index = %d\n", (*iter2).second.cb_index );
      }
}



In a separate module that links against the aff_data class DLL, I populate the map as follows:

aff_map_t affdata;

for( ; ; )
{
      CString aff_num;
      CString aff_name;
      if( ! create_aff( aff_num, aff_name ) )
            break;

      long x = atol( aff_num );
      aff_data a;
      a.cb_index = -1;
      a.aff_name = (LPCTSTR) aff_name;
      affdata[ x ] = a;
}


Immediately after the loop terminates, I dump the map container to debug output, and after the first map item is read, I get an access violation, apparently as a result of iter2 (see above) pointing to an invalid or corrupt map tree item.

dump_affiliates( affdata );

Any ideas ?  It seems like this should work.

0
xn12z9i
Asked:
xn12z9i
  • 9
  • 5
  • 4
  • +2
1 Solution
 
stefan73Commented:
Hi xn12z9i,


> TRACE1( "name = '%s'  ", (*iter2).second.aff_name.substr(0,20).c_str() );

Iterators and c_str() don't match nicely.

Try replacing this with
string tmp=(*iter2).second.aff_name;
TRACE1( "name = '%s'  ", tmp.substr(0,20).c_str() );
(or use
TRACE1( "name = '%.20s'  ", tmp.c_str() );
)



Cheers,
Stefan
0
 
MafaldaCommented:
1) what is create_aff doing ?

2) Did you try to trace the values when inserted to aff_map_t ?

3) I assume that the proble is in

a.aff_name = (LPCTSTR) aff_name;

change it to copy th contents of aff_name ... you copy here the pointer !


4) naming is really confusing ;o)
0
 
xn12z9iAuthor Commented:
Stefan73:

Thanks for the tip about iterators/.c_str(), but no go. The problem appears to be that the iterator itself is pointing to a completely invalid object after the first increment of the iterator. It almost appears as though the map [] operator inserts the new key/value pair, but my overloaded assignment operator is failing to copy the values from "a" to the pair<>.second reference returned by operator []. What I'll try to do next is save the reference to the returned pair<>.second and assign into that (possibly saving the address of the pair<>.second object as well for further debugging).

Mafalda:

1. create_aff() is a trivial function that populates the CString arguments.
2. Well, yes and no, it's slightly confusing stepping through the truncated variable names and inlines in the stl headers. Yes, immediately after the insert, a TRACE on :

TRACE1( "%s\n", (affdata[(newly inserted key)]).aff_name.c_str() );

works perfectly fine, but only immediately after the insert.  affdata.size() returns a count that indicates the item was inserted. I'll try to debug a little further.

3. a.aff_name = (LPCTSTR) aff_name is completely valid.  The std::string assignment operator supports assignment of "const char *" types.

Thanks for the replys. Anyone else have any ideas ?
0
What does it mean to be "Always On"?

Is your cloud always on? With an Always On cloud you won't have to worry about downtime for maintenance or software application code updates, ensuring that your bottom line isn't affected.

 
itsmeandnobodyelseCommented:
You use

>> aff_map_t::const_iterator iter2

try to use

   aff_map_t::iterator iter2

instead.

Regards, Alex
0
 
rstaveleyCommented:
This looks dubious to me:

--------8<--------
for( ; ; )
{
     // ..
     CString aff_name;
     // ...
     a.aff_name = (LPCTSTR) aff_name;
     // ...
}
--------8<--------

You are getting a const char* to a temporary object from local object aff_name. When aff_name goes out of scope, I'd expect the LPCTSTR to be a wild pointer.
0
 
itsmeandnobodyelseCommented:
>> You are getting a const char* to a temporary object from local object aff_name.

As a.aff_name is      

     std::string aff_name;  

std::string::operator =  () makes a copy of the CString (internal) char array. So, there is nothing to worry about.

Regards, Alex
0
 
itsmeandnobodyelseCommented:
As CString has  a - so-called - cast operator LPCTSTR() and there is a string constructor and assign operator that takes a right-hand LPCTSTR (or const char*) a really *good* compiler wouldn't need an explicit cast.

Regards, Alex
0
 
rstaveleyCommented:
Thanks, Alex, I hadn't looked at the class definition properly.
0
 
MafaldaCommented:
The LPCTSTR seems valid alright ...  although it is a temporary pointer.
It wouldn't hurt to try and copy its contents . . . I say so from experience . . .

"In the simplest case, you can cast a CString object to be an LPCTSTR. The LPCTSTR type conversion operator returns a pointer to a read-only C-style null-terminated string from a CString object.
The pointer returned by LPCTSTR points into the data area used by the CString. If the CString goes out of scope and is automatically deleted or something else changes the contents of the CString, the LPCTSTR pointer will no longer be valid. Treat the string to which the pointer points as temporary. "

0
 
rstaveleyCommented:
I notice that aff_data has a copy constructor and assignment operator defined, which is probably not necessary bearing in mind the members. Could we see these in case there is something untoward happening in them?
0
 
rstaveleyCommented:
> Treat the string to which the pointer points as temporary

Yes, but the temporary will persist for the duration of the assignment.
0
 
xn12z9iAuthor Commented:
rstavely, copy/assignment implementation follows:

aff_data::aff_data() : aff_name( "" ), cb_index(-1)
{
}

aff_data::aff_data( const aff_data & rhs ) : aff_name(rhs.aff_name), cb_index(rhs.cb_index)
{
}

const aff_data & aff_data::operator =( const aff_data & rhs )
{
      if( this == &rhs )
            return *this;

      cb_index = rhs.cb_index;
      aff_name = rhs.aff_name;
      return *this;
}
0
 
xn12z9iAuthor Commented:
Looking at the debug output a little more closely, here is what I get at the point of the fault:

aff_num = 744  name = 'test string 1' cb_index = -1
aff_num = -842150451  

The fault message is that the memory at address '0x10489f21' refrerenced memory at '0xcdcdcdcc'.

As 0xcdcdcdcc looks pretty regularly formed, am I tripping over a section of previously initialized memory, or some kind of memory block boundary mark ?

Alex, re iterator vs. const_iterator, that did not have any noticeable effect. I still get the exception. Thanks, though.
0
 
MafaldaCommented:
So it is as I suspected either create_aff is doing something wrong or the LPCTSTR convertion is bad ...
0
 
xn12z9iAuthor Commented:
As a proof-of-concept, I converted the container to std::list<aff_data> instead of std::map<long,aff_data>. dump_affiliates() works properly now. Obviously, std::map<> is not playing nice with my aff_data class implementation. Does anyone have any insight into the std::map::insert() semantics ?

I also tried converting the map to std::map<long, aff_data*>, but still got the same access violation errors. Is the red/black tree in the std::map<> implementation getting corrupted somehow ?

Mafalda,

create_aff() is trivial for the code snippet I posted. It simply populates two CString objects passed as arguments.  As to your second point, the LPCTSTR conversion is no different than

std::string buf;
buf = "teststring";

in terms of rvalue object lifetime, scope, and assignment semantics.
0
 
rstaveleyCommented:
0xcdcdcdcd is uninitialised memory (see http://www.microsoft.com/msj/1198/c/c1198.aspx). It looks like you've decremented an integer in uninitialised memory, if you see 0xcdcdcdcc.
0
 
rstaveleyCommented:
> 0xcdcdcdcd is uninitialised memory

In debug mode on MSVC.
0
 
rstaveleyCommented:
> Does anyone have any insight into the std::map::insert() semantics ?

affdata[ x ] = a;

...should be OK. It takes a copy of a.
0
 
rstaveleyCommented:
What do you see if you display the aff_nums used for the index in the map entries?

for( ; ; )
{
     // ...
#ifdef _DEBUG
    afxDump << "Key " << x << '\n';
#endif
     affdata[ x ] = a;
     // ...
}
0
 
MafaldaCommented:
It is not that I do understand the code or your explanations I am just saying that something is malfunctioning  . . . I am sure it is in the allocation of memory for the aff_name variable or somewhere in the way. You also show this is the problem.
In my honest opinion it is usualy something in the programmers code ... even if he is sure his/her code is OK.
In other cases the bug is caused by a bug in the compiler ...
Maybe you should try to compile it in VS .NET ...
Everybody seems to say the code is OK so it seems like a VC++ 6 bug.

"0xcdcdcdcd is uninitialised memory" -> It's obvious that either you or Microsoft code is not initializing memory in the right way !!!!
You do not have to be Einstein to understand that ;o)
0
 
rstaveleyCommented:
One last thought (before I shoot off for the weekend)...

Your assignment operator implementation code is in the DLL, but you are assigning aff_name in your calling code.

That means that you are going to get free store memory for aff_name's data in the calling code (somewhere or other STL is going to call new), but you are releasing it in the DLL (somewhere or other STL is going to call delete).

The DLL and calling code have separate heap managers. I would expect that the heap manager in the DLL would choke at deleting a pointer allocated by the heap manager in the calling code.

To test this theory, can you try implementing the assignment operator code inline and see if that fixes it?

i.e. in your header:
--------8<--------
inline const aff_data & aff_data::operator =( const aff_data & rhs )
{
     if( this == &rhs )
          return *this;

     cb_index = rhs.cb_index;
     aff_name = rhs.aff_name;
     return *this;
}
--------8<--------

If that does the job, you should consider replacing the assignment...

>   a.aff_name = (LPCTSTR) aff_name;

with a setter which is implemented by the DLL, and make the data members private and of course remove all of the inlining.

Must go.... the weekend beckons.... hope this helps.
0
 
xn12z9iAuthor Commented:
Just an update - my implementation works, no modifications required, when using STLPort...

Thanks for all the help so far.
0

Featured Post

Independent Software Vendors: We Want Your Opinion

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

  • 9
  • 5
  • 4
  • +2
Tackle projects and never again get stuck behind a technical roadblock.
Join Now