C++ memory leak (valgrind)

Question

Valgrind claims I'm indirectly losing memory; what vexes me, is I have no idea why that would be the case.

Not sure if this is a false positive or if I just don't understand some pointer assignment or something.

Am I losing memory here? If so, why?

Valgrind report:

==24392== 21 bytes in 2 blocks are indirectly lost in loss record 1 of 3
==24392==    at 0x4028699: operator new[](unsigned int) (in        /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==24392==    by 0x804B41D: Perishable::load(std::basic_fstream<char,        std::char_traits<char> >&) (Perishable.cpp:42)
==24392==    by 0x804C504: sict::PosApp::loadRecs() (PosApp.cpp:139)
==24392==    by 0x804D58E: sict::PosApp::run() (PosApp.cpp:393)
==24392==    by 0x8049337: main (milestone4.cpp:8)

Here is the line 42:

std::fstream& Perishable::load(std::fstream& stream) {
    char    mysku[MAX_SKU_LEN + 1];
    char    mynam[100];
    bool    mytax;
    double  myprice;
    int     myqty;
    int     date[5]; 

    stream.getline(mysku, 100, ',');
    sku(mysku);
    stream.getline(mynam, 100, ',');

    name(mynam);//bytes indirectly lost

Here is name() [fully commented]

void Item::name(char *name){
    delete[] _name; //..............Just in case it points to something.
                    //..............Note: How could I possibly be losing memory with this line here??
    int x = strlen(name); //........Grab the length of the new input

    if (_name == '\0') {//..........If it was empty, ie. its the first assignment
        _name = new char[x + 1];//..Allocate the space, +1 for null char
    }

    for (int i = 0; i < x; i++) {//.Copy
        _name[i] = name[i];
    }

    _name[x]   = '\0';//............Yeah, its manual termination. I should maybe use strcpy
}

Edit>>> here is the destructor

Item::~Item() {
    std::cout << "called destructor";
    delete[] _name;
    _name = '\0';
}

Edit>> copy constructor and assignment operator

//Copy Constructor
Item::Item(const Item& copyfrom){
    (*this) = copyfrom;

}

//Member operators
Item& Item::operator=(const Item &myitem) {
    if (!myitem.isEmpty()){
        init((*this), myitem._sku, myitem._name, myitem._price, myitem._taxed);
        this->_quantity = myitem._quantity;

    }
    return (*this);
}

void Item::init(Item &obj, char const sku[], char const name[], double priced, bool taxed) {

    int length = strlen(name);
    int skulength = strlen(sku);

    obj._price = priced;
    obj._taxed = taxed;
    obj._quantity = 0;
    obj._name = new char[length+1]; //+1 for the null which wasn't counted. Huge pain debugging that.


    for (int i = 0; i < length; i++) {

        obj._name[i] = name[i];

        if (i < skulength) {//redundanc


            obj._sku[i] = sku[i];
        }
    }

    obj._name[length]   = '\0';
    obj._sku[skulength] = '\0';

}

This is the compiler screaming in pain, trying to tell you to use `std::string`. — Jerry Coffin, Dec 09 '15 at 18:17
Haha. Probably. Unfortunately its a school assignment and I don't get to be shwifty like that heh. — bigcodeszzer, Dec 09 '15 at 18:18
If you can't use `std::string`, write a (minimal) imitation of it, and use that. Simple test: if you have `new [xxx]` or `delete [] foo;` if your code, you're doing something wrong. — Jerry Coffin, Dec 09 '15 at 18:19
It's school: They make you do things ancient-style to make sure you understand the concept. If you don't know /how/ to use it without string, that's not a good thing. — bigcodeszzer, Dec 09 '15 at 18:21
The problem is that this is teaching you the *wrong* concepts--ones you'll need to un-learn before you can write code decently at all. — Jerry Coffin, Dec 09 '15 at 18:23
@IIIIIIIIIIIIIIIIIIIIIIII I believe it is the first line of the method `Item::name` — Algirdas Preidžius, Dec 09 '15 at 18:24
where is _name defined? Also, the `delete [] _name` looks scary, especially when you use it afterwards (possible double frees etc.). The `_name == '\0' looks verry wrong also (should this be == nullptr? or _name[0] == '\0'? Which would clearly be undefined behaviour after delete...) — Fabian Schmitthenner, Dec 09 '15 at 18:25
@AlgirdasPreidžius, no, that is a delete for a _name that maybe has not even been allocated. the fact is after this new, that has to be a function to free the memory, it could be the destructor for Item — ForeverStudent, Dec 09 '15 at 18:25
@bigcodeszzer If these institutions insist on teaching you "the concepts", then why not have you write a string class and be done with it?. This one-off manipulation of `new char []` honestly doesn't teach anything except how to write bad code. Writing a real string class, yes, may not be necessary in the real world, but at the very least, teaches you something beneficial. — PaulMcKenzie, Dec 09 '15 at 18:27
@AlgirdasPreidžius I'll print the destructor... just a sec. But I've been looking at it and I think it is the x+1, the math doesn't add up right, I think there is an extra index. — bigcodeszzer, Dec 09 '15 at 18:27
@PaulMcKenzie Well, society won't give me the /chance/ to write good code without a degree. So don't look at me. — bigcodeszzer, Dec 09 '15 at 18:28
Ok, what I see here is that after you freed `_name`, it isn't reset to `nullptr`, and holds its old value. So when you are checking for `_name == '\0'`, it wouldn't be true, and you would not allocate the new buffer for your string (if it was set to something different, before calling `Item::name`, that is). Not sure how is that related to losing memory, though. — Algirdas Preidžius, Dec 09 '15 at 18:29
@bigcodeszzer That doesn't stop you from writing a string class. If you did that, and used it in your Item class, you wouldn't have the issues you have now. Be a little ambitious -- it takes a half hour, maybe an hour, if you know what a copy constructor, assignment operator and destructor are. — PaulMcKenzie, Dec 09 '15 at 18:29
@AlgirdasPreidžius The destructor is actually virtual. That could be my problem, it should be in the derived class. — bigcodeszzer, Dec 09 '15 at 18:29
@PaulMcKenzie I have to use their format. It's part of the assignment specs. — bigcodeszzer, Dec 09 '15 at 18:30
@bigcodeszzer: Your professor doesn't understand modern C++ at all if that's the sort of code he considers right. Looks like he's trying to write C. — Vincent Savard, Dec 09 '15 at 18:32
@bigcodeszzer I bet there is no restriction on you creating your own class, using the same rules given to you. You wouldn't be using the `std::string` class, and you're still using `new []` and `delete []` only in a different part of the code. Again, if you were taught what a copy constructor is, what an assignment operator, and what a destructor does, you can still play by the rules and write the code correctly. — PaulMcKenzie, Dec 09 '15 at 18:33
@bigcodeszzer I can't imagine any mature and seasoned programmer "hating" a langugage for a fanboy reason. Bjarne stroustrup who is the creator of C++ mentiones in his book "The C++ programming language" that is was comfortable with 25 languages before he embarked on his journey that would become C++ — ForeverStudent, Dec 09 '15 at 18:33
@AlgirdasPreidžius edit: the destructor is now at the bottom — bigcodeszzer, Dec 09 '15 at 18:38
"Well, society won't give me the /chance/ to write good code without a degree." Doable, but I admit it's freaking hard to get in the door without a ticket. Wrote embedded systems for five years before getting a degree. Good investment, though. Wage tripled for the same work. I recommend a software engineering degree over computer science if you want to write good code. — user4581301, Dec 09 '15 at 18:48
@bigcodeszzer If you are copying or assigning `Item` objects, then your code will not work correctly without a user-defined copy constructor and assignment operator. You need to show us in what context you're using the Item objects.. And it doesn't matter what the teacher says, your code *cannot* work correctly if you're doing any of these operations (assignment, copy construction). So if you were not taught these operations, then complain. I can easily cause a leak with your class doing basically very little (again, if you don't have the required operations coded). — PaulMcKenzie, Dec 09 '15 at 18:50
Yep. Recommend a read: [What is the Rule of Three?](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) — user4581301, Dec 09 '15 at 18:52
Not using copy or assignment in the main, but I'm pretty sure I have them. One sec. — bigcodeszzer, Dec 09 '15 at 18:57
@PaulMcKenzie Added. I assure you, neither are being called from main. The issue here is valgrind telling me I have indirect mem loss. It's probably pointer related. — bigcodeszzer, Dec 09 '15 at 19:00
@bigcodeszzer The compiler can also copy / assign. For example, returning or passing `Item` objects by value. — PaulMcKenzie, Dec 09 '15 at 19:01
The kicker is it's really easy to do a copy or assignment without meaning to. Call a function and don't use a pointer or reference, that's a copy (and almost always fatal because the copy's destructor will be called when the function exits) . Stuff the object in an array, that's an assignment. Put it into a vector, that's usually a copy or an assignment, depending on how you did it. — user4581301, Dec 09 '15 at 19:01
Item is just the polymorphic pointer in my case, I'm using derived classes Perishable and NonPerishable which have virtual destructors. And... no copy constructors or assignment operators I don't think. — bigcodeszzer, Dec 09 '15 at 19:02
Your assignment operator is incorrect. Simple question -- what happened to the old string in `this` in the assignment operator? From what I see, it was never `delete`d. Also, your class is not exception safe -- if those calls to `new` fail in your assignment operator or in `name`, your object is messed up. — PaulMcKenzie, Dec 09 '15 at 19:05
Your copy constructor won't work. It will copy the pointer to `_name`, not the data at `_name`, leaving you with two objects pointing to one memory allocation. When one is destroyed both `_name`'s data will go with it.. What it will do is allow you to place a breakpoint or a print statement so you can see if it ever is called. — user4581301, Dec 09 '15 at 19:05
Also, I will point out: This program does not crash. It is fully functional. I put it through valgrind to see what's up there.... now I'm getting small memory losses. A lot of the problems you are suggesting would crash at runtime. — bigcodeszzer, Dec 09 '15 at 19:11
@bigcodeszzer -- *This program does not crash.* In C++, -- There is no guarantee how a program will run with the types of bugs we're pointing out to you. Your code invokes undefined behavior. Saying that "it would crash at runtime" is incorrect. — PaulMcKenzie, Dec 09 '15 at 19:13
In `Item::init`: `obj._name = new char[length+1]` occurs without checking to ensure `_name` is not occupied. May be your leak. Instead of an init function, use a constructor. [Practice RAII](https://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization) and save the init functions for rare occasions. RAII can save a lot of debugging time. — user4581301, Dec 09 '15 at 19:17
Non-crashing example: Say you overrun an array and write over a variable outside the array that you aren't using. It's good memory, so no crash and you dodged a bullet... until days later you enter a case where the overwritten variable matters. So a bug that manifested long in the past results in odd behaviour. Those are really really fun to trace. — user4581301, Dec 09 '15 at 19:23
I got one of those with valgrind also. Said invalid write of size 4. I've managed to decrease that to 1 so far. — bigcodeszzer, Dec 09 '15 at 19:36
@bigcodeszzer -- The other issue with your assignment operator is that it assigns a `quantity` of 0 when it should be assigning whatever the `quantity` is of the object that is being assigned from. I also don't get why your `init` function is this complex. Just `strcpy` whatever is passed to you. Take it or leave it, here is a better implementation of all of the functions you're having trouble with: http://ideone.com/mvGEGx — PaulMcKenzie, Dec 09 '15 at 19:51

score 0 · Answer 1 · answered Dec 09 '15 at 18:29

If you really must use new and bare pointers (hint: almost nobody does), then you need to ensure that the owner of _name releases it when it's no longer needed.

The right place for that is probably the ~Item() destructor, but it's hard to be sure without the rest of your code.

AlexB · Accepted Answer · 2015-12-09T19:48:17.720

0

I only see:

Item::~Item() {
    std::cout << "called destructor";
    delete[] _name;
    // Don't reassign
}

and

void Item::init(Item &obj, char const sku[], char const name[], double priced, bool taxed) {
    // ...
    if (obj._name == nullptr /* 0 */) {
        obj._name = new char[length+1]; 
        // What if it's not null?
    } else {
        // Do you want to delete and new up a char?
    }
}

Also, using a non-const reference is scary, to me. I see the new-ing up of obj._name in init to be the biggest problem, without any pre-emptive checks.

edited Dec 09 '15 at 19:48

answered Dec 09 '15 at 19:37

AlexB

78
4

Checking for what specifically? – bigcodeszzer Dec 09 '15 at 19:45
I edited the answer. If it's not null and you new it up, you're not deallocating that old block before allocating new space in the free store. I found [this](http://stackoverflow.com/questions/9979776/does-calling-new-twice-on-the-same-pointer-without-calling-delete-in-betwe) that I think is relevant. – AlexB Dec 09 '15 at 19:52

C++ memory leak (valgrind)

2 Answers2