C++ char array copy to unsigned char array

Question

I am trying to read a file and store the information in unsigned char arrays. However, my program seems to be overwriting variables.

ClassA header:

...
public:
    ClassA(void);
    void LoadMemoryBlock(char* block, int bank);
....
private:
    unsigned char upperMemoryBank1[16384];
    unsigned char upperMemoryBank2[16384];
....

ClassA file:

ClassA::ClassA(void)
{
}
...
void ClassA::LoadMemoryBlock(char* block, int bank)
{
    if (bank == 1)
    {
        memcpy(upperMemoryBank1, block, 16384);
    }
    else if (bank == 2)
    {
        memcpy(upperMemoryBank2, block, 16384);
    }
}

ClassB header:

...
private:
    ClassA* classAobject;
...

ClassB file:

ClassB::ClassB()
{
    classAobject = &ClassA();
    ...
}
...
ClassB::StoreFile(ifstream &file)
{
    int position;

    char fileData[16384];

    position = file.tellg();
    file.seekg(HEADER_SIZE, ios::beg);
    position = file.tellg();
    file.read(fileData, 16384);
    position = file.tellg();
    classAobject->LoadMemoryBlock(fileData, 1);
    classAobject->LoadMemoryBlock(fileData, 2);

    position = file.tellg(); // Crashes here
    file.seekg(16384 + HEADER_SIZE, ios::beg);
    ...
}

Watching the position variable in my debugger shows that after the LoadMemoryBlock calls, it no longer shows 16400 like it did beforehand, but rather a random number that is different every time. Also, the file ifstream also is corrupted by the LoadMemoryBlock call. So I am guessing that memcpy is overwriting them.

I tried initializing my arrays differently, but now memcpy crashes!

ClassA header:

...
public:
    ClassA(void);
    void LoadMemoryBlock(char* block, int bank);
....
private:
    unsigned char* upperMemoryBank1;
    unsigned char* upperMemoryBank2;
....

ClassA file:

ClassA::ClassA(void)
{
    upperMemoryBank1 = new unsigned char[16384];
    upperMemoryBank2 = new unsigned char[16384];
}
...
void ClassA::LoadMemoryBlock(char* block, int bank)
{
    if (bank == 1)
    {
        memcpy(upperMemoryBank1, block, 16384); // Crashes here
    }
    else if (bank == 2)
    {
        memcpy(upperMemoryBank2, block, 16384);
    }
}

ClassB header:

...
private:
    ClassA* classAobject;
...

ClassB file:

ClassB::ClassB()
{
    classAobject = &ClassA();
    ...
}
...
ClassB::StoreFile(ifstream &file)
{
    int position;

    char* fileData = new char[16384];

    position = file.tellg();
    file.seekg(HEADER_SIZE, ios::beg);
    position = file.tellg();
    file.read(fileData, 16384);
    position = file.tellg();
    classAobject->LoadMemoryBlock(fileData, 1);
    classAobject->LoadMemoryBlock(fileData, 2);

    position = file.tellg();
    file.seekg(16384 + HEADER_SIZE, ios::beg);
    ...
}

I thought that at least one, if not both, of these methods should work. What am I doing wrong?

EDIT: I have included the ClassA initialization above.

This is how I call the StoreFile method:

bool ClassB::Load(char* filename)
{
    ifstream file(filename, ios::in|ios::binary);

    if(file.is_open())
    {
        if(!StoreFile(file))
        {
            return false;
        }

        file.close();
        return true;
    }

    printf("Could not open file: %s\n", filename);
    return false;
}

Please try to make a full example that we can use to reproduce the problem. Otherwise we're left with mostly guessing. (You'll probably find the bug yourself while narrowing it down to such an example) — R. Martinho Fernandes
– R. Martinho Fernandes, Commented Apr 1, 2012 at 21:50
Why do you have yet another intermediate local array, rather than reading into the relevant bank directly? — Kerrek SB
– Kerrek SB, Commented Apr 1, 2012 at 21:50
Did you check that classAobject points to a valid object of class ClassA at the beginning of that code? — celtschk
– celtschk, Commented Apr 1, 2012 at 21:50

David Schwartz · Accepted Answer · 2012-04-01 22:09:11Z

2

99% chance the bug is in whatever code initializes the value of the classAobject pointer. If it points to a legitimate instance of a ClassA object, the code should be fine.

Update: Yep. That was it.

classAobject = &ClassA();

This creates a new ClassA object and then stores a pointer to it. But at the end of the statement, it goes out of scope and is destroyed, leaving classAobject holding a pointer to a non-existing object. You want:

classAobject = new ClassA();

Don't forget the rule of three -- delete it in the destructor, allocate a new one on operator= and in the copy constructor. Or better yet, use a more C++ method like a smart pointer, depending on the semantics desired.

edited Apr 1, 2012 at 22:09

answered Apr 1, 2012 at 21:47

David Schwartz

184k18 gold badges229 silver badges293 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

pestilence669 Over a year ago

Consider "classAObject = new ClassA;" ... or better yet, just make it static and skip the new. If you insist on using the heap, think about boost::shared_ptr or another managed container to avoid leaking RAM.

Lubo Antonov · Accepted Answer · 2012-04-01 22:05:51Z

1

In the ClassB constructor you are initializing the classAobject pointer to the address of a temporary variable that becomes invalid as soon the constructor returns. This is the cause of the problem. Use new to create a proper heap object.

answered Apr 1, 2012 at 22:05

Lubo Antonov

2,33214 silver badges18 bronze badges

Collectives™ on Stack Overflow

C++ char array copy to unsigned char array

2 Answers 2

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related