strcpy and the copy constructor

Discussion in 'C++' started by RonHiler, Oct 18, 2004.

  1. RonHiler

    RonHiler Guest

    My copy constructor is crashing my program, and I can't figure out
    why. I'll try to make the code listing as short as I can. Here are
    the two headers:

    class BreakthroughClass
    {
    public:
    BreakthroughClass();
    virtual ~BreakthroughClass();
    BreakthroughClass(const BreakthroughClass &source);
    BreakthroughClass& operator=(const BreakthroughClass &source);

    protected:
    char Name[256];
    char Description[1024];
    int LocationX;
    int LocationY;
    vector <unsigned int> Prereqs;

    friend class TechManagerClass;
    };

    class TechManagerClass
    {
    public:
    TechManagerClass();
    virtual ~TechManagerClass();

    void InitializeTechs();

    private:
    vector <BreakthroughClass> Breakthroughs;

    void BreakthroughLoadTitles(char *RCLabel);
    void BreakthroughLoadData(char *RCLabel);

    //single instance, do not implement these functions
    TechManagerClass(const TechManagerClass &source);
    TechManagerClass& operator=(const TechManagerClass &source);
    };

    TechManagerClass TechManager;

    So Breakthroughs are a vector of BreakthroughClass contained in the
    TechManagerClass, and TechManager is a global instance of
    TechManagerClass.

    When I call TechManager.InitializeTechs, I do this:

    void TechManagerClass::InitializeTechs()
    {
    BreakthroughLoadTitles("SCIENCE");
    BreakthroughLoadTitles("GOVERNMENT");
    //and so on
    }

    Then:

    void TechManagerClass::BreakthroughLoadTitles(char *RCLabel)
    {
    int i;
    char FullLabel[256];
    HRSRC ResourceLocation;
    HANDLE Resource;
    LPVOID ResourcePointer;
    char* pChar; //temporary tranversing variables
    BreakthroughClass NewBreakthrough;
    char OutputString[1024];

    i = 0;
    while (1)
    {
    sprintf (FullLabel, "%s_%d", RCLabel, i);
    sprintf(OutputString, "Loading %s Technology Breakthrough",
    FullLabel);
    OutputDebugString(OutputString);
    ResourceLocation = FindResourceEx (NULL, RT_RCDATA, FullLabel,
    MAKELANGID(LANG_NEUTRAL, SUBLANG_NEUTRAL));
    if (ResourceLocation == NULL)
    break;
    Resource = LoadResource (NULL, ResourceLocation);
    ResourcePointer = LockResource(Resource);

    pChar = (char*)ResourcePointer;
    strcpy(NewBreakthrough.Name, pChar);
    OutputDebugString(NewBreakthrough.Name);
    Breakthroughs.push_back(NewBreakthrough);
    i++;
    }
    sprintf(OutputString, "Loaded %i %s Technology Breakthroughs", i,
    RCLabel);
    OutputDebugString(OutputString);
    }

    So I'm finding the entry in my resource file (which are named
    SCIENCE_0, SCIENCE_1, GOVERNMENT_0, GOVERNMENT_1, and so on), copying
    the name into my temporary NewBreakthrough variable, then doing a
    push_back into Breakthroughs. This calls my copy constructor, and
    here's where I have problems. I took everything out (for now) except
    the one line, and this is the line causing the crash.

    BreakthroughClass::BreakthroughClass(const BreakthroughClass &source)
    {
    strcpy (Name, source.Name);
    }

    I've done OutputDebugString on the Name entries right into the copy
    constructor, and it seems to be coming out fine (I get the name of the
    techs), so I don't see why the strcpy is crashing. What am I doing
    wrong?

    Thanks,

    Ron
     
    RonHiler, Oct 18, 2004
    #1
    1. Advertising

  2. RonHiler wrote:
    > My copy constructor is crashing my program, and I can't figure out
    > why. [...]
    >
    > BreakthroughClass::BreakthroughClass(const BreakthroughClass &source)
    > {
    > strcpy (Name, source.Name);


    What's 'Name'? Where does it point to?

    > }
    >
    > I've done OutputDebugString on the Name entries right into the copy
    > constructor, and it seems to be coming out fine (I get the name of the
    > techs), so I don't see why the strcpy is crashing. What am I doing
    > wrong?


    You're using character pointers when you should use 'std::string'.

    V
     
    Victor Bazarov, Oct 18, 2004
    #2
    1. Advertising

  3. RonHiler

    Howard Guest

    "RonHiler" <> wrote in message
    news:...
    > Resource = LoadResource (NULL, ResourceLocation);
    > ResourcePointer = LockResource(Resource);
    >
    > pChar = (char*)ResourcePointer;
    > strcpy(NewBreakthrough.Name, pChar);
    > OutputDebugString(NewBreakthrough.Name);



    >
    > BreakthroughClass::BreakthroughClass(const BreakthroughClass &source)
    > {
    > strcpy (Name, source.Name);
    > }
    >
    > I've done OutputDebugString on the Name entries right into the copy
    > constructor, and it seems to be coming out fine (I get the name of the
    > techs), so I don't see why the strcpy is crashing. What am I doing
    > wrong?


    Is it possible that the string array of characters pointed to by pChar above
    isn't null-terminated, but has some garbage after it that isn't shown by
    your OutputDebugString call? If so, then there's a possibility that strcpy
    has trashed memory. Alternatively, have you checked what's in source.Name
    inside the copy constructor? Perhaps something is wrong with the string (or
    the object itself) at that point?

    -Howard
     
    Howard, Oct 18, 2004
    #3
  4. RonHiler

    Adrian Guest

    "RonHiler" <> wrote in message
    news:...
    > My copy constructor is crashing my program, and I can't figure out
    > why. I'll try to make the code listing as short as I can. Here are
    > the two headers:


    Since you delcare Name as a char array the default copy constructor will
    work fine, it you change name to pointer then all bets are off. Also as
    mentioned elsewhere use std::string

    #include <iostream>
    #include <cstring>

    class Test
    {
    public:
    Test(char *fred) { std::strncpy(data, fred, sizeof(data)-1); };
    void display() const { std::cout << data << std::endl; };
    void change(char *fred) { std::strncpy(data, fred, sizeof(data)-1); };
    private:
    char data[256];

    };

    int main()
    {
    Test t1("string 1");
    Test t2("string 2");

    t1.display();
    t2.display();
    t2=t1;

    t1.display();
    t2.display();

    t1.change("new string");
    t1.display();
    t2.display();

    return 0;
    }
     
    Adrian, Oct 18, 2004
    #4
  5. RonHiler

    RonHiler Guest

    "Howard" <> wrote in message news:<1PTcd.717506$>...
    >
    > Is it possible that the string array of characters pointed to by pChar above
    > isn't null-terminated, but has some garbage after it that isn't shown by
    > your OutputDebugString call? If so, then there's a possibility that strcpy
    > has trashed memory. Alternatively, have you checked what's in source.Name
    > inside the copy constructor? Perhaps something is wrong with the string (or
    > the object itself) at that point?


    Yeah, I looked at those possibilities, and it would seem to make
    sense, but I just don't see how it can be possible. Here is one entry
    from my resource file as an example:

    SCIENCE_0 RCDATA //BREAKTHROUGH
    BEGIN
    "Chemistry\0",
    "Chemistry breakthrough description to be written...\0",
    0, 0, //LOCATION IN TECH TREE
    1, //NUMBER OF PREREQS
    "Physics\0" //PREREQ LIST
    END

    Clearly my strings are null terminated. This seems to be born out by
    the OutputDebugStrings, which aren't showing any problems. I checked
    the strlen(), to see if there were characters not showing up with ODS,
    but the ODS output seemed perfectly normal. Here is a snippet of what
    I got:

    ODS: Loading SCIENCE_0 Technology Breakthrough Process MDestiny.exe
    (0xD34)
    ODS: Chemistry Process MDestiny.exe (0xD34)
    ODS: Chemistry 9 Process MDestiny.exe (0xD34)
    ODS: Loading SCIENCE_1 Technology Breakthrough Process MDestiny.exe
    (0xD34)
    ODS: Physics Process MDestiny.exe (0xD34)
    ODS: Physics 7 Process MDestiny.exe (0xD34)
    ODS: Loading SCIENCE_2 Technology Breakthrough Process MDestiny.exe
    (0xD34)
    ODS: Loaded 2 SCIENCE Technology Breakthroughs Process MDestiny.exe
    (0xD34)

    The first line shows the name of the resource entry being parsed
    The second line shows the technology name entry at the
    BreakthroughLoadTitles() level (in the temporary NewBreakthrough
    variable).
    The third line is actually inside the copy constructor, checking
    source.Name, with the string and its length being shown.
    This repeats for the next resource entry (SCIENCE_1). Then a third
    entry is parsed but not found (SCIENCE_2, which causes the break in
    the while statement), and the final line lets me know there were 2
    total entries parsed for that catagory.

    Everything SEEMS to be exactly as it should be, right into the copy
    constructor. I'm really pulling my hair out over this one :)

    Ron
     
    RonHiler, Oct 18, 2004
    #5
  6. "RonHiler" <> wrote in message
    news:...
    > "Howard" <> wrote in message
    > news:<1PTcd.717506$>...
    >>
    >> Is it possible that the string array of characters pointed to by pChar
    >> above
    >> isn't null-terminated, but has some garbage after it that isn't shown by
    >> your OutputDebugString call? If so, then there's a possibility that
    >> strcpy
    >> has trashed memory. Alternatively, have you checked what's in source.Name
    >> inside the copy constructor? Perhaps something is wrong with the string
    >> (or
    >> the object itself) at that point?

    >
    > Yeah, I looked at those possibilities, and it would seem to make
    > sense, but I just don't see how it can be possible. Here is one entry
    > from my resource file as an example:
    >
    > SCIENCE_0 RCDATA //BREAKTHROUGH
    > BEGIN
    > "Chemistry\0",
    > "Chemistry breakthrough description to be written...\0",
    > 0, 0, //LOCATION IN TECH TREE
    > 1, //NUMBER OF PREREQS
    > "Physics\0" //PREREQ LIST
    > END
    >
    > Clearly my strings are null terminated. This seems to be born out by
    > the OutputDebugStrings, which aren't showing any problems. I checked
    > the strlen(), to see if there were characters not showing up with ODS,
    > but the ODS output seemed perfectly normal. Here is a snippet of what
    > I got:
    >
    > ODS: Loading SCIENCE_0 Technology Breakthrough Process MDestiny.exe
    > (0xD34)
    > ODS: Chemistry Process MDestiny.exe (0xD34)
    > ODS: Chemistry 9 Process MDestiny.exe (0xD34)
    > ODS: Loading SCIENCE_1 Technology Breakthrough Process MDestiny.exe
    > (0xD34)
    > ODS: Physics Process MDestiny.exe (0xD34)
    > ODS: Physics 7 Process MDestiny.exe (0xD34)
    > ODS: Loading SCIENCE_2 Technology Breakthrough Process MDestiny.exe
    > (0xD34)
    > ODS: Loaded 2 SCIENCE Technology Breakthroughs Process MDestiny.exe
    > (0xD34)
    >
    > The first line shows the name of the resource entry being parsed
    > The second line shows the technology name entry at the
    > BreakthroughLoadTitles() level (in the temporary NewBreakthrough
    > variable).
    > The third line is actually inside the copy constructor, checking
    > source.Name, with the string and its length being shown.
    > This repeats for the next resource entry (SCIENCE_1). Then a third
    > entry is parsed but not found (SCIENCE_2, which causes the break in
    > the while statement), and the final line lets me know there were 2
    > total entries parsed for that catagory.
    >
    > Everything SEEMS to be exactly as it should be, right into the copy
    > constructor. I'm really pulling my hair out over this one :)
    >
    > Ron


    Unless I missed it you never said where you are calling InitializeTechs
    from. Is it possible that you are running into the so called 'static
    initialization order fiasco'. In other words because you are calling
    InitializeTechs from the constructor of another global variable you are
    inadvertently using the Breakthroughs vector before it has been constructed?

    If this sounds at all likely check out the FAQ

    http://www.parashift.com/c -faq-lite/ctors.html#faq-10.11

    john
     
    John Harrison, Oct 18, 2004
    #6
  7. RonHiler

    RonHiler Guest

    "John Harrison" <> wrote in message news:<>...
    >
    > Unless I missed it you never said where you are calling InitializeTechs
    > from. Is it possible that you are running into the so called 'static
    > initialization order fiasco'. In other words because you are calling
    > InitializeTechs from the constructor of another global variable you are
    > inadvertently using the Breakthroughs vector before it has been constructed?
    >


    Thanks for the thought, but no, I call InitializeTechs from inside my
    WinMain function, so that's not it :)
     
    RonHiler, Oct 19, 2004
    #7
  8. RonHiler

    RonHiler Guest

    "Adrian" <> wrote in message news:<cl14kp$ofv$>...
    >
    > Since you delcare Name as a char array the default copy constructor will
    > work fine, it you change name to pointer then all bets are off.


    It does not. The defualt constructor gives the same crash. And I'm
    not using the default constructor for three reasons
    1) I will be changing the char arrays to pointers and using 'new' to
    allocate memory for them in the copy constructor (otherwise I'll be
    wasting a lot of space, as I will have about 800 techs by the end).
    For now they are arrays just to keep things simple until I get this
    part debugged :)
    2) The class in question contains vectors (see the header for
    BreakthroughClass), and it's been my experience that default copy
    constructors do not handle vectors well.
    3) I consider it bad form to use default copy constructors in ANY
    case, even when it seems perfectly safe. I will either prototype them
    as private (and not define the function) if the class is meant to be
    singly instanced, or, when I HAVE to have a copy constructor (such as
    this case where the class is used as a vector member), I create my
    own.

    > Also as
    > mentioned elsewhere use std::string
    >


    for purposes of performance, I dislike std::string.

    I've discovered an odd thing. Having decided the source data was fine
    (based on the ODS output), I turned my attention to where the data was
    going. And, as it turns out, the code seems to work just fine when I
    add a single line to the constructor for TechManagerClass

    TechManagerClass::TechManagerClass()
    {
    Breakthroughs.clear();
    Breakthroughs.reserve(100);
    }

    The addition of the reserve command seems to do the trick, though I'm
    not sure why that should be. It appears as though the memory for the
    vectors is not being allocated before the strcpy command in the CC.
    Which seems very strange to me, as what I know about vectors suggests
    that ought not be the case (I thought the first thing push_back() did
    was allocate memory if needed).

    Does someone know the reason for this? Is it because of the large
    memory footprint of the class (1296 bytes per member if I calculate
    that right)?

    I've also considered the possibility that I'm just hiding the problem,
    that in fact strcpy is still leaking (somehow) but now it has memory
    space to leak into (because of the reserved space), and I'll run right
    back into the same issue once I get more techs input into the resource
    file. Can someone comment? :)

    Thanks guys for the responses. I know it's hard to debug someone
    else's code, and I appreciate the efforts.

    Ron
     
    RonHiler, Oct 19, 2004
    #8
  9. "RonHiler" <> wrote in message
    news:...
    > "Adrian" <> wrote in message
    > news:<cl14kp$ofv$>...
    >>
    >> Since you delcare Name as a char array the default copy constructor will
    >> work fine, it you change name to pointer then all bets are off.

    >
    > It does not. The defualt constructor gives the same crash. And I'm
    > not using the default constructor for three reasons
    > 1) I will be changing the char arrays to pointers and using 'new' to
    > allocate memory for them in the copy constructor (otherwise I'll be
    > wasting a lot of space, as I will have about 800 techs by the end).
    > For now they are arrays just to keep things simple until I get this
    > part debugged :)
    > 2) The class in question contains vectors (see the header for
    > BreakthroughClass), and it's been my experience that default copy
    > constructors do not handle vectors well.
    > 3) I consider it bad form to use default copy constructors in ANY
    > case, even when it seems perfectly safe. I will either prototype them
    > as private (and not define the function) if the class is meant to be
    > singly instanced, or, when I HAVE to have a copy constructor (such as
    > this case where the class is used as a vector member), I create my
    > own.
    >
    >> Also as
    >> mentioned elsewhere use std::string
    >>

    >
    > for purposes of performance, I dislike std::string.
    >
    > I've discovered an odd thing. Having decided the source data was fine
    > (based on the ODS output), I turned my attention to where the data was
    > going. And, as it turns out, the code seems to work just fine when I
    > add a single line to the constructor for TechManagerClass
    >
    > TechManagerClass::TechManagerClass()
    > {
    > Breakthroughs.clear();
    > Breakthroughs.reserve(100);
    > }
    >
    > The addition of the reserve command seems to do the trick, though I'm
    > not sure why that should be. It appears as though the memory for the
    > vectors is not being allocated before the strcpy command in the CC.
    > Which seems very strange to me, as what I know about vectors suggests
    > that ought not be the case (I thought the first thing push_back() did
    > was allocate memory if needed).
    >
    > Does someone know the reason for this? Is it because of the large
    > memory footprint of the class (1296 bytes per member if I calculate
    > that right)?
    >
    > I've also considered the possibility that I'm just hiding the problem,
    > that in fact strcpy is still leaking (somehow) but now it has memory
    > space to leak into (because of the reserved space), and I'll run right
    > back into the same issue once I get more techs input into the resource
    > file. Can someone comment? :)
    >
    > Thanks guys for the responses. I know it's hard to debug someone
    > else's code, and I appreciate the efforts.
    >


    I think this all points to heap corruption occurring somewhere else in your
    code. push_back will allocate memory, but when you call reserve it no longer
    needs to (at least not for a while). Heap corruption problems are notorious
    being hard to find because they cause a crash sometime after the corruption
    has actually occurred.

    Is it possible for you to post a complete program that has this behaviour?
    Try and cut out everything extraneous and then post the reduced program
    here. Often going through this process will help you find the problem
    because you'll remove some piece of code that you think has no bearing and
    suddenly the problem goes away, therefore you've a pretty good idea that the
    code just removed was the cause of the problem.

    John
     
    John Harrison, Oct 19, 2004
    #9
    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. Aire
    Replies:
    3
    Views:
    486
    Mike Wahler
    Jan 25, 2004
  2. ali
    Replies:
    4
    Views:
    618
    David Harmon
    Mar 5, 2007
  3. Alok  Kumar

    using strcpy to copy from a char

    Alok Kumar, Mar 24, 2007, in forum: C Programming
    Replies:
    15
    Views:
    824
    CBFalconer
    Mar 25, 2007
  4. Generic Usenet Account
    Replies:
    10
    Views:
    2,341
  5. cinsk
    Replies:
    35
    Views:
    2,725
    James Kanze
    Oct 11, 2010
Loading...

Share This Page