JACK M JACK M - 23 days ago 10
C++ Question

is there any double free risk in this class

I write a simple Buffer class which holds a buffer and provides a function to reverse the content of the buffer.

Buffer.h

#ifndef __BUFFER_H__
#define __BUFFER_H__

#include <stdlib.h>
#include <cerrno>
#include <stdio.h>

class Buffer
{

private:
char * buffer;
int size;


public:
Buffer(int size);
~Buffer();
void reverse(int size);

};

#endif


Buffer.cc

#include "Buffer.h"


Buffer::Buffer(int size)
{
this -> size = size;
this -> buffer = (char *)malloc(size);
if(this -> buffer == NULL)
throw 1;
}

Buffer::~Buffer()
{
if(this -> buffer != NULL)
free(this -> buffer);
}

void Buffer::reverse(int size)
{
char tmp;
int i;
char * tmpb = this -> buffer;
for(i = 0; i < size / 2; i++)
{
tmp = (char)tmpb[i];
tmpb[i] = tmpb[size - i - 1];
// printf("exchange %x with %x\n", tmp & 0xff, tmpb[i] & 0xff);

tmpb[size - i - 1] = tmp;
}
}


There is a remote server to test my implementation using fault injection. That server gives me report that there is bug caused by double free or corruption. I have read my implementation many times, but no luck to find the bug. I dont have access to that server. Any help?

NOTE: I have to use C style code. Otherwise I would fail the server test. That is a hard requirement. Well, you may think this requirement is silly. But this is the requirement. Maybe one point is learning the bad while someone mixing C and C++.

The server provides a main function to test my implementation.

For anyone who wants to have a look at all the code, you could download a zip file from https://mega.nz/#!FhoHQD5Y!iD9tIZMNtKPpxfZTpL2KWoUJRedbw6wToh6QfVvzOjU. Just compile using
make
. The result is a program named
rcopy
which reverses content of a file byte by byte and then outputs to a new file.

Answer

Consider forbidding copying explicitly, if the compiler is allowed to generate an implicit copy constructor, then it could double free via two objects that share the buffer pointer. Otherwise, I see no way that a double free could happen. You can do this in C++11 like this:

class Buffer {
private:
    char *buffer;
    int size;

public:
    Buffer(int size);
    Buffer(const Buffer &) = delete;
    Buffer &operator=Buffer(const Buffer &) = delete;
    ~Buffer();
    void reverse(int size);

};

A few minor notes:

1.

free(NULL) is defined by the standard to be a non-operation. So

if(this -> buffer != NULL)
    free(this -> buffer);

Can just be:

free(this -> buffer);

2.

tmp = (char)tmpb[i];

Why cast here? tmpb[i] should already be a char. As a general rule of thumb, most of the times that you feel the need to cast, it probably means that there is a better way to do the task. Of course there are exceptions, but clean code should have minimal casting.

3.

Any reason not to just use std::swap(tmpb[size - i - 1], tmpb[i]) inside your reverse function?