0

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.

JACK M
  • 2,627
  • 3
  • 25
  • 43
  • 2
    What is this supposed to accomplish that `std::vector` with `std::reverse` wouldn't do at least as well? – Jerry Coffin Nov 15 '16 at 20:49
  • 1
    Not quite related but why are you using `malloc`, `free` and C-style casts in C++ code? – UnholySheep Nov 15 '16 at 20:49
  • 3
    Sure there are double free risks. You didn't define copy-constructor. Have a read about [What is The Rule of Three?](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). In addition to that, why are you using `malloc`, and `free` instead of `new`, and `delete` in your C++ code? – Algirdas Preidžius Nov 15 '16 at 20:49
  • 1
    also the `reverse` method takes a parameter whereas you aleady know the size. If you pass too big a size, there is your memory corruption. What does the server test? your class? or a main program that you provide? – Jean-François Fabre Nov 15 '16 at 20:51
  • *" I have to use C style code."* Who comes up with requirements like that? – UnholySheep Nov 15 '16 at 20:52
  • If you have to use C style code, then why are you using C++? Use C. – Algirdas Preidžius Nov 15 '16 at 20:54
  • 1
    *I have read my implementation many times, but no luck to find the bug* -- Here is your "test suite": `{ Buffer b1(10); Buffer b2 = b1; }` There's your double free error. – PaulMcKenzie Nov 15 '16 at 21:03

1 Answers1

1

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?

Evan Teran
  • 87,561
  • 32
  • 179
  • 238