C++ possible memory leak reading file

Question

I'm relatively new to C++ and I'm trying to read a proprietary file format. I know the header format, and I've created a struct RTIHeader with the necessary fields.

My test code reads bytes from the file and copies them into the same memory space as the header struct, effectively reconstituting it. My problem is that every time I run the test code (just calling the constructor) I get a different value! My theory is that I do not fully understand memcpy.

struct RTIHeader decode(char* memblock){
    struct RTIHeader header;
    memcpy(&header,&memblock,sizeof(RTIHeader));
    return header;
}

RTIFile::RTIFile(const char* filename){
    // open the file in binary input mode with the pointer at the end
    std::ifstream file(filename,
                       std::ios::in |
                       std::ios::binary |
                       std::ios::ate);

    std::ifstream::pos_type size;
    char * memblock;
    RTIHeader header;

    // if the file didn't open, throw an error
    if(!file.is_open()){
        //TODO learn about C++ error handling
        return;
    }
    // use pointer position to determine file size
    size = file.tellg();
    // read file
    memblock = new char [sizeof(RTIHeader)];
    file.seekg(0,std::ios::beg);
    file.read(memblock,sizeof(RTIHeader));

    header = decode(memblock);

    std::cout << (unsigned int)header.signature[0] << "\n";

    // still need to read the rest of the file
    file.close();
}

We need the definition of RTIHeader. – Edward Strange Sep 12 '12 at 01:32 — Edward Strange, Sep 12 '12 at 01:32

score 2 · Accepted Answer · answered Sep 12 '12 at 01:34

2

You are passing the address of memblock as the second argument of memcpy. Since memblock is a parameter to the function, you will just be copying chunks of memory off your stack.

To fix, just pass the pointer directly:

memcpy(&header,memblock,sizeof(RTIHeader));

By the way, since you are returning the structure by value, you will incur an extra memcpy as the returned value is copied in full (unless your compiler optimizes it). To avoid this, you should consider passing the target structure as a pointer to decode, as in

struct RTIHeader *decode(struct RTIHeader *header, char* memblock){
    memcpy(header, memblock, sizeof(RTIHeader));
    return header;
}

Then you just have to do

decode(&header, memblock);

which is more efficient.

answered Sep 12 '12 at 01:34

nneonneo

171,345
36
312
383

Thank you, that was fast! SO tells me I need to wait 7 minutes before accepting your answer but that was the exact problem. – Ryan Kennedy Sep 12 '12 at 01:36
Thanks for the tip on optimization as well, very insightful! – Ryan Kennedy Sep 12 '12 at 01:45
3

You should also be careful with packing. File formats usually assume packing of 1 (no gaps between fields), but the struct in the code is declared with the default packing (depending on the tools you use) and might not be of the same size/layout as file format declares. – Zdeslav Vojkovic Sep 12 '12 at 01:52
Okay, so would I just add `#pragma pack(1)` before `RTIHeader`? – Ryan Kennedy Sep 12 '12 at 02:35
It's compiler dependent. See http://stackoverflow.com/questions/4274055/portable-way-of-writing-a-c-struct-to-a-file-serialisation-for-c. – nneonneo Sep 12 '12 at 02:37
@Ryan Do not this pointer stuff that's being recommended in this answer. This is exactly the kind of stuff you should NOT do unless the profiler tells you. Not only will ALL modern compilers optimize away the copies, the more recent ones will use move semantics when appropriate. By passing the return in by address you limit your ability to initialize to useful values. Furthermore, if you do in fact find that you're making unnecessary and slow copies you should be passing by reference, NOT by pointer. This person is giving you really bad advice. – Edward Strange Sep 12 '12 at 02:41
This is a *POD struct*, not a class object. None of the bits about move semantics or initialization values matters. – nneonneo Sep 12 '12 at 02:50

C++ possible memory leak reading file

1 Answers1