Char Data Getting Corrupted

[Deluge]

I'm trying to create a type that has a char array and integer id members:

class ResourceObject {
public:
    ResourceObject(const unsigned char* data, const int id) : objectData(data), objectId(id) {}
    int getId() { return objectId; }
    unsigned char* getData() { return objectData; }
private:
    const unsigned char* objectData;
    const int objectId;
};

The arrays that I am using are actually PNG image data. Example:

const static unsigned char png1[] = {
    0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d,
...
...

But whenever I call the getData method, my char array seems to have become corrupted:

ResourceObject img1(png1, 1000);
img1.getData(); // corrupted data 

It appears I am storing it wrong in my class member.

Edit: Seems to work fine if I do something like the following to test it:

#include <iostream>
using namespace std;

int main(int argc, char** argv) {
	unsigned char text[] = { 'f', 'o', 'o', 't', 'b', 'a', 'l', 'l', '\0' };
	ResourceObject football(text, 1000);

	cout << "Selected sport: " << football.getData() << endl;

	return 0;
}

Outputs:

Selected sport: football

Not sure why my PNG data is getting corrupted.

[salem c]

You need to understand the scope of the memory your class ends up pointing to.

Having your class just hold a pointer to some bit of memory outside the class means you're tying the lifetimes of two different bits of data together.

If one is deleted (your PNG data) before the other (your ResourceObject), then the latter all of a sudden has a stale pointer (read: garbage).

Rather than a raw pointer, use one of the available smart pointers.
https://stackoverflow.com/questions/6876751/differences-between-unique-ptr-and-shared-ptr

On the plus side, you found this problem early.
Sometimes people go for months or years believing their 'working' code is good only to find their stale pointers are down to pure dumb luck.

[Deluge]

I believe I have found an answer with a little help from this answer on Stackoverflow ( https://stackoverflow.com/a/16645642/4677917 ). I can't use an assignment operator to copy the array data. Using a function like memcpy ( http://www.cplusplus.com/reference/cstring/memcpy/ ) allows me to copy the data to a new array properly.

I think this goes along with what you were saying about scope of memory salem c.

Going to do a few more tests.

Edit: Wasn't having any luck with memcpy. But found that I was able to use std::vector to store the data properly:

ResourceObject::ResourceObject(unsigned char* data, unsigned int data_size, const int id) : objectId(id) {
	objectData = vector<char>(data, data + data_size);
}

Still not sure why it wouldn't work with memcpy:

ResourceObject::ResourceObject(unsigned char* data, unsigned int data_size, const int id) : objectId(id) {
	objectData = new unsigned char[data_size];
	memcpy(objectData, data, data_size);
}

[jonnin]

the memcpy looks good. you changed types (unsigned char vs char) above; are you doing a comparison that fails due to signs when you think it was 'corrupted'?
Are you doing any string operations, which would work on a c-string (football) but not png data (has random zeros wherever).

the way it is written, objectData == data for the first data_size bytes. So, if data is a valid pointer, and has at least data_size bytes, the above should have worked, barring other snafus like threaded program(?) where maybe something else touched data while you were copying it.

[Deluge]

Here is some simplified sample code of what I was doing:

res/failsafe.png.h:

#ifndef RES_FAILSAFE_PNG_H
#define RES_FAILSAFE_PNG_H

static const unsigned char failsafe_png[] = {
	0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d,
	...
	...
};

#endif /* RES_FAILSAFE_PNG_H */ 

test.cpp:

#include "res/failsafe.png.h"

#include <fstream>
#include <iostream>
using namespace std;


class ResourceObject {
public:
	ResourceObject(unsigned char* data, unsigned int data_size);
	unsigned char* getData() { return objectData; }
	unsigned int getSize() { return sizeof(objectData); }
private:
	unsigned char* objectData;
};

ResourceObject::ResourceObject(unsigned char* data, unsigned int data_size) {
	objectData = new unsigned char[data_size];
	memcpy(objectData, data, data_size);

	// correct?
	delete[] objectData;

	cout << "\nIncoming size: " << data_size << "\n";
	cout << "Stored size: " << getSize() << "\n\n"; // 8
}


int main(int argc, char** argv) {
	int idx;
	fstream pngout;

	cout << "Writing original image ...\n";

	// creates working PNG image
	pngout.open("out_orig.png", fstream::out|fstream::binary);
	for (idx = 0; idx < sizeof(failsafe_png); idx++) {
		pngout << failsafe_png[idx];
	}
	pngout.close();

	ResourceObject ro((unsigned char*)failsafe_png, sizeof(failsafe_png));

	cout << "Writing copy image ...\n";

	// creates corrupt PNG image
	pngout.open("out_copy.png", fstream::out|fstream::binary);
	for (idx = 0; idx < ro.getSize(); idx++) {
		pngout << ro.getData()[idx];
	}
	pngout.close();

	return 0;
}

It was suggested I call delete[] on my data, though I don't really understand why:

alys666 wrote:

it's how you must implement this place, and don't use std::vector.

uint8 *_data = new uint8[datasize];
memcpy(_data, ext_data, datasize);
...
delete[] _data;

They recommended that I don't use std::vector. But, that's what I can get to work.

It's late again, so sorry if anyone replies & you don't hear from me for a while. Getting sleepy.

[Deluge]

Figured out that I had to store the data size along with the data, thanks to user doublemax over at wxWidgets forums ( https://forums.wxwidgets.org/viewtopic.php?f=1&t=46033#p192242 ):

class ResourceObject {
public:
	ResourceObject(unsigned char* data, unsigned int data_size);
	~ResourceObject() { delete[] objectData; }
	unsigned char* getData() { return objectData; }
	unsigned int getSize() { return objectSize; }
private:
	unsigned char* objectData;
	unsigned int objectSize;
};

ResourceObject::ResourceObject(unsigned char* data, unsigned int data_size) {
	objectData = new unsigned char[data_size];
	memcpy(objectData, data, data_size);
	objectSize = data_size;
}

[jonnin]

I don't get it... you had the size in the first version with memcpy, was it not set correctly?

that aside:
you need to call delete for every chunk of memory that you got with new. Every time. The where is what matters; here it looks like you want objectSize to live for a while after your copy, so when do you delete it? The destructor is a likely place to do that. Also, I highly recommend these changes:
constructor: set objectSize to null.
above code: if objectSize is not null, delete it first, before the new statement. You can skip the test and just delete it if you want, this is the *logic* but delete of null does not do anything and tests/conditions cost as much as just calling it, so you can leave the if off if you want.
proceed as above.

as it stands, if you call this routine more than once, you will leak memory, and if these are sizeable image files, you will actually be leaking LARGE amounts of memory. This is NOT good.
vectors are microscopically slower than your code, but if you don't understand pointers well, and really even if you do, you are introducing potential for bugs and problems by using pointers over vectors. Unless this is extreme high performance required code, I would use the vector... and if it is extreme performance, you have ... a lot of things yet to do to make this code efficient!

[Deluge]

In the first version, getting the data size wasn't working correctly:

ResourceObject ro(data, sizeof(data));
unsigned int ro_size = sizeof(ro.getData()); // only 8 bytes 

Made another version with getSize() method which simply returned sizeof(objectData). That didn't solve my problem because I ended up with the same result as originally:

public:
    unsigned int getSize() { return sizeof(objectData); }

...

ResourceObject ro(data, sizeof(data));
unsigned int ro_size = ro.getSize(); // still only 8 bites 

I was told that sizeof will not work on the objectData member because it is a pointer & not a static char array. So, I would have to store the data size separately. So, I created the member variable objectSize to store it. Now, getSize() method returns objectSize:

public:
    unsigned int getSize() { return objectSize; }
private:
    unsigned int objectSize;

I'm sorry, I really don't understand how I am creating memory leaks. I didn't think that objectSize was a pointer, so I wouldn't need to call delete on it. objectData is a pointer, so I call delete[] objectData; in the destructor.

--- Edit ---

Perhaps you mean I should be calling delete[] objectData in the constructor? A user over at the wxWidgets forums ( https://forums.wxwidgets.org/viewtopic.php?f=1&t=46033#p192245 ) wrote some code as follows:

if(_data){delete[] _data; _data=nullptr; _size=0;};

Which if translated into my variable names would be something like:

if (objectData) {
    delete[] objectData;
    objectData = NULL;
    objectSize = 0;
}

Is that what you are saying I should be doing in the constructor? So my constructor would look like this:

ResourceObject::ResourceObject(unsigned char* data, unsigned int data_size) {
	if (objectData) {
		delete[] objectData;
		objectData = NULL;
		objectSize = 0;
	}

	objectData = new unsigned char[data_size];
	memcpy(objectData, data, data_size);
	objectSize = data_size;
}

[jonnin]

sorry. I have a form of visual issue where front loaded variables all look the same to me, and I confused objectSize and objectData as you noticed.

Again, I don't know how you are using this stuff.
To leak memory would take the following scenario:

- you have some other method that is copying the data exactly the same way your constructor is doing it
- you call it more than once per object.

If you are keeping the memory to the constructor and destructor as shown, there is no problem! I just know how tempting it is to get something working and copy it... :)