Is there a possible memory leak in my C++ file?

Question

I'm using an example code given to me by another C++ coder for a project. I'm a new student of C++ language and I wondered is there a possible memory leak / bugs in this class file given to me (PlacementHead.cpp):

#include "PlacementHead.h"
#include <string>
#include <iostream>
#include <string.h>

PlacementHead::PlacementHead(int width, int height, int gap, char* s) {
    width_ = width;
    height_ = height;
    gap_ = gap;
    size_ = (width*height)+1;
    set_ = new char[size_ + 1];
    from_ = new int[size_ + 1];
    original_ = new char[size_ + 1];
    strcpy(set_,s);
    strcpy(original_,s);
}

PlacementHead::~PlacementHead() {

}

int PlacementHead::getSize() { return size_; }
int PlacementHead::getHeight() { return height_; }
int PlacementHead::getWidth() { return width_; }
int PlacementHead::getGap() { return gap_; }

// Palauttaa indeksissä i olevan suuttimen
char PlacementHead::getNozzle(int i) {
    return set_[i-1];
}

// Asettaa indeksissä i olevan suuttimen
void PlacementHead::setNozzle(int i, char c) {
    set_[i-1] = c;
}

// Merkitsee suuttimen poimituksi poistamalla sen listasta
void PlacementHead::markNozzle(int i, int bankPos) {
    set_[i-1] = ' ';
    from_[i-1] = bankPos;
}

// Palauttaa seuraavan poimimattoman suuttimen indeksin
int PlacementHead::getNextUnmarkedPos() {
    for (int i=0; i<size_; i++) {
        if (set_[i]!=' ') {
            return i+1;
        }
    }
    return 0;
}

// Palauttaa suuttimen alkuperäisen sijainnin pankissa
int PlacementHead::getBankPos(int i) {
    return from_[i-1];
}

// Plauttaa alkuperäisen ladontapaan suutinjärjestyksen
void PlacementHead::reset() {
    //for (int i=0; i<size_; i++) {
    //  set_[i] = original_[i];
    //}
    strcpy(set_,original_);
}

// Tulostusmetodi
void PlacementHead::print() {
    std::cout << "ladontapaa:\n";
    for (int h=height_; h>0; h--) {
        for (int w=width_; w>0; w--) {
            int i = ((h-1)*width_)+w;
            std::cout << getNozzle(i);
        }
        std::cout << "\n";
    }
}

PlacementHead.h:

#ifndef PLACEMENTHEAD_H
#define PLACEMENTHEAD_H

class PlacementHead {
    public:
        PlacementHead(int size, int rows, int gap, char* s);
        ~PlacementHead();
        int getSize();
        int getHeight();
        int getWidth();
        int getGap();
        char getNozzle(int i);
        void setNozzle(int i, char c);
        void markNozzle(int i, int bankPos);
        int getNextUnmarkedPos();
        int getBankPos(int i);
        void reset();
        void print();
    private:
        char* set_;
        int* from_;
        char* original_;
        int size_;
        int width_;
        int height_;
        int gap_;
};

#endif

I notice that there is dynamic allocation of memory, but I don't see a delete anywhere...is this a problem? How could I fix this if it is a problem?

Thnx for any help!

P.S.

I noticed there is no keyword class used in this example?...Can you define a class like this?

Looks like 3 memory leaks to me! – Paul R Jan 17 '14 at 11:37 — Paul R, Jan 17 '14 at 11:37
+1 I thought so as well :D thnx for the confirmation! – jjepsuomi Jan 17 '14 at 11:37 — jjepsuomi, Jan 17 '14 at 11:37
You need 3 x `delete [] ...` in the destructor. – Paul R Jan 17 '14 at 11:39 — Paul R, Jan 17 '14 at 11:39
+1 Thnx @PaulR much appreciated! – jjepsuomi Jan 17 '14 at 11:40 — jjepsuomi, Jan 17 '14 at 11:40

score 5 · Accepted Answer · answered Jan 17 '14 at 11:48

5

It's impossible to say without seeing the class definition (the header); if size_, etc. are something like boost::shared_array, or std::unique_ptr, there is no leak. If they are simply int*, there is a leak.

Of course, no C++ programmer would write this sort of code anyway. The class would contain std::vector<int> and std::string. Judging from what we see here, the author doesn't know C++.

answered Jan 17 '14 at 11:48

James Kanze

150,581
18
184
329

+1 @JamesKanze thank you for your help, I included the code's header-file. Is this what you meant? =) – jjepsuomi Jan 17 '14 at 11:52
1

@jjepsuomi Yes. And as I suspected, the variables in question are raw pointers, so it does leak. marcin_j's point about not obeying the rule of three is basically right too, but if you use `std::vector` and `std::string` (or smart pointer), you'll get the deep copy semantics automatically (or you may want to disable copy and assignment---if the class represents some external entity, you usually don't want to support them). – James Kanze Jan 17 '14 at 12:19

score 1 · Answer 2 · edited May 23 '17 at 12:11

Another problem is that your code does not obey Rule of three (links here and here)

once you will write code like:

{
PlacementHead a(0,0,0,"asdsa");
PlacementHead b(0,0,0,"asdsa");
a = b; // line 1
} // here segfault

you will get segfault, in line 1, pointers will be copied from b to a, and once you will finally have destructors, pointers will be deleted twice, which is wrong. This is called shallow copy, you need deep copy, where new array will be allocated.

score 1 · Answer 3 · answered Jan 17 '14 at 12:02

1

the code has leak . the constructor allocates the memory .Destructor or some other function have to clean that before the object gets destroyed

answered Jan 17 '14 at 12:02

Simal Haneef

179
5

Is there a possible memory leak in my C++ file?

3 Answers3