C++ cout corruption

Question

I am reading a file header using ifstream. Edit: I was asked to put the full minimal program, so here it is.

#include <iostream>
#include <fstream>


using namespace std;

#pragma pack(push,2)

struct Header
{
    char label[20];
    char st[11];
    char co[7];
    char plusXExtends[9];
    char minusXExtends[9];
    char plusYExtends[9];
};

#pragma pack(pop)

int main(int argc,char* argv[])
{
    string fileName;
    fileName = "test";

    string fileInName = fileName + ".dst";

    ifstream fileIn(fileInName.c_str(), ios_base::binary|ios_base::in);
    if (!fileIn)
    {
       cout << "File Not Found" << endl;
       return 0;
    }

    Header h={};
    if (fileIn.is_open()) {
                cout << "\n" << endl;
                fileIn.read(reinterpret_cast<char *>(&h.label), sizeof(h.label));
                cout << "Label: " << h.label << endl;
                fileIn.read(reinterpret_cast<char *>(&h.st), sizeof(h.st));
                cout << "Stitches: " << h.st << endl;
                fileIn.read(reinterpret_cast<char *>(&h.co), sizeof(h.co));
                cout << "Colour Count: "  << h.co << endl;
                fileIn.read(reinterpret_cast<char *>(&h.plusXExtends),sizeof(h.plusXExtends));
                cout << "Extends: "  << h.plusXExtends << endl;
                fileIn.read(reinterpret_cast<char *>(&h.minusXExtends),sizeof(h.minusXExtends));
                cout << "Extends: "  << h.minusXExtends << endl;
                fileIn.read(reinterpret_cast<char *>(&h.plusYExtends),sizeof(h.plusYExtends));
                cout << "Extends: "  << h.plusYExtends << endl;

// This will output corrupted
                cout << endl << endl;
                cout << "Label: " << h.label << endl;
                cout << "Stitches: " << h.st << endl;
                cout << "Colour Count: "  << h.co << endl;
                cout << "Extends: "  << h.plusXExtends << endl;
                cout << "Extends: "  << h.minusXExtends << endl;
                cout << "Extends: "  << h.plusYExtends << endl;
    }


    fileIn.close();

    cout << "\n";
    //cin.get();
    return 0;
}


ifstream fileIn(fileInName.c_str(), ios_base::binary|ios_base::in);

Then I use a struct to store the header items

The actual struct is longer than this. I shortened it because I didn't need the whole struct for the question. Anyway as I read the struct I do a cout to see what I am getting. This part is fine.

As expected my cout shows the Label, Stitches, Colour Count no problem. The problem is that if I want to do another cout after it has read the header I am getting corruption in the output. For instance if I put the following lines right after the above code eg

Instead of seeing Label, Stitches and Colour Count I get strange symbols, and corrupt output. Sometimes you can see the output of the h.label, with some corruption, but the labels are Stitches are written over. Sometimes with strange symbols, but sometimes with text from the previous cout. I think either the data in the struct is getting corrupted, or the cout output is getting corrupted, and I don't know why. The longer the header the more the problem becomes apparent. I would really like to do all the couts at the end of the header, but if I do that I see a big mess instead of what should be outputting.

My question is why is my cout becoming corrupted?

Answer 1

Using arrays to store strings is dangerous because if you allocate 20 characters to store the label and the label happens to be 20 characters long, then there is no room to store a NUL (0) terminating character. Once the bytes are stored in the array there's nothing to tell functions that are expecting null-terminated strings (like cout) where the end of the string is.

Your label has 20 chars. That's enough to store the first 20 letters of the alphabet: ABCDEFGHIJKLMNOPQRST

But this is not a null-terminated string. This is just an array of characters. In fact, in memory, the byte right after the T will be the first byte of the next field, which happens to be your 11-character st array. Let's say those 11 characters are: abcdefghijk.

Now the bytes in memory look like this: ABCDEFGHIJKLMNOPQRSTabcdefghijk

There's no way to tell where label ends and st begins. When you pass a pointer to the first byte of the array that is intended to be interpreted as a null-terminated string by convention, the implementation will happily start scanning until it finds a null terminating character (0). Which, on subsequent reuses of the structure, it may not! There's a serious risk of overrunning the buffer (reading past the end of the buffer), and potentially even the end of your virtual memory block, ultimately causing an access violation / segmentation fault.

When your program first ran, the memory of the header structure was all zeros (because you initialized with {}) and so after reading the label field from disk, the bytes after the T were already zero, so your first cout worked correctly. There happened to be a terminating null character at st[0]. You then overwrite this when you read the st field from disk. When you come back to output label again, the terminator is gone, and some characters of st will get interpreted as belonging to the string.

To fix the problem you probably want to use a different, more practical data structure to store your strings that allows for convenient string functions. And use your raw header structure just to represent the file format.

You can still read the data from disk into memory using fixed sized buffers, this is just for staging purposes (to get it into memory) but then store the data into a different structure that uses std::string variables for convenience and later use by your program.

For this you'll want these two structures:

#pragma pack(push,2)
struct RawHeader  // only for file IO
{
    char label[20];
    char st[11];
    char co[7];
    char plusXExtends[9];
    char minusXExtends[9];
    char plusYExtends[9];
};
#pragma pack(pop)

struct Header  // A much more practical Header struct than the raw one
{
    std::string label;
    std::string st;
    std::string co;
    std::string plusXExtends;
    std::string minusXExtends;
    std::string plusYExtends;
};

After you read the first structure, you'll transfer the fields by assigning the variables. Here's a helper function to do it.

#include <string>
#include <string.h>

template <int n> std::string arrayToString(const char(&raw)[n]) {
    return std::string(raw, strnlen_s(raw, n));
}

In your function:

Header h;
RawHeader raw;

fileIn.read((char*)&raw, sizeof(raw));
// Now marshal all the fields from the raw header over to the practical header.
h.label         = arrayToString(raw.label);
h.st            = arrayToString(raw.st);
h.st            = arrayToString(raw.st);
h.co            = arrayToString(raw.co);
h.plusXExtends  = arrayToString(raw.plusXExtends);
h.minusXExtends = arrayToString(raw.minusXExtends);
h.plusYExtends  = arrayToString(raw.plusYExtends);

It's worth mentioning that you also have the option of keeping the raw structure around and not copying your raw char arrays to std::strings when you read the file. But you must then be certain that when you want to use the data, you always to compute and pass lengths of the strings to functions that will deal with those buffers as string data. (Similar to what my arrayToString helper does anyway.)