Ytukyg

I have a server that sends raw binary data to print a "map" that a user must traverse through, however, I am having trouble clearing out my buffer after each line read and thus keep getting residual data printed at the end of the shorter lines. In the screenshot below you can see my output on the left, and what the output should be on the right. What is the best way to solve this? I feel like I am missing something but cant seem to find a solution.

And the code that is reading/printing this is below:

char* mapData = NULL;

string command = "command> ";

size_t dataSize = 0;

while(mapData != command.c_str()) {

    unsigned char* buffer = (unsigned char*) &dataSize;

    connection = read(mySocket, buffer, 8);

    if(connection == -1 || connection < 0) {

        cerr << "**Error: could not read text size" << endl;

        return 1;

    }



    mapData = (char*)malloc(dataSize);

    buffer = (unsigned char*) mapData;



    while((connection = read(mySocket, buffer, dataSize)) != -1) {

        if(connection == -1 || connection < 0) {

            cerr << "**Error: could not read text size" << endl;

        return 1;

        }

        if(dataSize != 1) {

            cout << buffer;

        }

        free(buffer);

        buffer = NULL;

    }



}

You are ignoring the return value of read() to know how many bytes are in the buffer.

read() returns the actual number of bytes that were read, which may be fewer than you requested. So you need to call read() in a loop until you have read all of the bytes you are expecting, eg:

int readAll(int sock, void *buffer, size_t buflen)

{

    unsigned char* pbuf = reinterpret_cast<unsigned char*>(buffer);

    while (buflen > 0) {

        int numRead = read(sock, pbuf, buflen);

        if (numRead < 0) return -1;

        if (numRead == 0) return 0;

        pbuf += numRead;

        buflen -= numRead;

    }

    return 1;

}

Also, after reading the buffer, you are treating it as if it were null-terminated, but it is not, which is why you get extra garbage in your output.

More importantly, mapData != command.c_str() will ALWAYS be true, so your while loop iterates indefinitely (until a socket error occurs), which is not what you want. You want the loop to end when you receive a "command> " string instead.

mapData is initially NULL, and c_str() NEVER returns NULL, so the loop ALWAYS iterates at least once.

Then you allocate and free mapData but don't reset it to NULL, so it is left pointing at invalid memory. Which doesn't really matter, since your while loop is just comparing pointers. c_str() will NEVER return a pointer to memory that mapData ever points to.

To end your loop correctly, you need to compare the contents of mapData after reading, not compare its memory address.

Try this instead:

char *mapData = NULL;

uint64_t dataSize = 0;

const string command = "command> ";

bool keepLooping = true;



do {

    if (readAll(mySocket, &dataSize, sizeof(dataSize)) <= 0) {

        cerr << "**Error: could not read text size" << endl;

        return 1;

    }



    if (dataSize == 0)

        continue;



    mapData = new char[dataSize];



    if (readAll(mySocket, mapData, dataSize) <= 0) {

        cerr << "**Error: could not read text" << endl;

        delete mapData;

        return 1;

    }



    cout.write(mapData, dataSize);



    keepLooping = (dataSize != command.size()) || (strncmp(mapData, command.c_str(), command.size()) != 0);



    delete mapData;

}

while (keepLooping);

Alternatively:

string mapData;

uint64_t dataSize = 0;

const string command = "command> ";



do {

    if (readAll(mySocket, &dataSize, sizeof(dataSize)) <= 0) {

        cerr << "**Error: could not read text size" << endl;

        return 1;

    }



    mapData.resize(dataSize);



    if (dataSize > 0) {

        if (readAll(mySocket, &mapData[0], dataSize) <= 0) {

            cerr << "**Error: could not read text" << endl;

            return 1;

        }



        cout << mapData;

    }

}

while (mapData != command);

like @eozd pointed out, calling malloc and free in your loop is a bad idea since you use return statements. Your code may leak memory. You should ensure you call free before returns. Even better, you could declare your buffer outside of while loop, and use break instead of return, and call free if there was en error

Looking at your solution, it seems that the communication protocol involves sending data size first, followed by the actual data. How is data size written to the wire? You may need to convert it from network byte order.

To debug, you could print out the value of dataSize before every read to make sure that it is what you expect

You should clear the buffer too. Add:

 memset(mapData, 0, dataSize);

after the malloc.