Avoiding memory leak in C++?

Question

I'm beginning at coding in C++, and I recently encountered a problem that I can't really find a solution for it, because I don't understand my error.

My program is made to give the conversion in Kelvin to Celsius, but it isn't able to do it for more than 3 temperatures in a row. As soon as I pass 4, it gives me the right conversion with a memory problem (out of range?).

Error in "xxx": Double free or corruption (out): 0x0000000001c94c40

And when I enter more than four temperatures it gives some random numbers, that are probably values of other adresses.

Here is my program.

#include <iostream>

using namespace std;

unsigned int NumberT {0};           //Definition of the global variables
double* Kelvin=new double[NumberT];
double* Celsius=new double[NumberT];
unsigned int i=0;


double* Conversion (double, double, unsigned int)   //Conversion function
{
    for(unsigned int i=0;i<NumberT;++i)
    {
        Celsius[i]=Kelvin[i]-273.15;
    }
    return Celsius;
}


void printTemperatures (double, double, unsigned int)   //Print function
{
    for(unsigned int i=0;i<NumberT;++i)
    {
        cout<<"The temperature is "<< Kelvin[i] <<" [K], which is "<< Celsius[i] <<" [C]"<<endl;
    }
    return;
}


int main ()                     //Main
{   
    cout<<"How many temperatures do you want to enter?"<<"\n";
    cin>>NumberT;

    cout<<"What are the temperatures?"<<"\n";
    for (unsigned int i=0;i<NumberT;++i)
    {
        cin>>Kelvin[i];
    }

    Conversion (Kelvin[i], Celsius[i], NumberT);
    printTemperatures (Kelvin[i], Celsius[i], NumberT);


    delete [] Celsius;
    delete [] Kelvin;
    return 0;
}

So I would be really happy to know what is wrong with my code, and why is it like this. I heard that we shouldn't use global variables, and this can maybe help me to understand why.

By the way I'm interested to have some advices on how to write a proper code with a good syntax, without having scope problems. Because I would like to learn how to write a function that is the most "universal" possible, so that I can pull it and use it in another program with other variables.

Thank you in advance for your support

P.S.: I'm using g++ as compiler and the 2011 C++ standard

`NumberT` is 0 when you do your memory allocation - move the globals and memory allocation inside `main`, read the value of `NumberT` first, and only then do the actual memory allocations. You should also format your code properly - you have a number of other errors which will become more obvious once the code is formatted. — Paul R, Mar 27 '17 at 13:29
You really should be using a `std::vector` instead of `double*`. — NathanOliver, Mar 27 '17 at 13:29
Your arrays, that you allocated, have a size of 0. I don't remember if such an action is an undefined behavior in of itself, but trying to write data to such an array - definitively is. — Algirdas Preidžius, Mar 27 '17 at 13:30
You should never use `new` which will make your code much easier and memory leaks very unlikely. Stick to values, `std::vector` and if you absolutely cannot avoid it use `std::unique_ptr`. — nwp, Mar 27 '17 at 13:31

score -2 · Answer 1 · answered Mar 27 '17 at 13:31

-2

A good habit in modern C++ is to never use manual memory management - if you see new or delete in the c++11 or later code it's a sign of code smell.

Use std::make_shared/std::make_unique instead, use value types and std::move.

See Herb Sutter's latest talk "Leak-Freedom in C++ by default".

For your particular example

unsigned int NumberT {0}; //Definition of the global variables double* Kelvin=new double[NumberT];

You create an array with 0 elements here, then you do a lot of unallocated memory overwrites and then try to free it. This won't work.

Move array allocations to AFTER you've read the NumberT from std::cin and it should improve considerably.

Next improvement would be to switch to more functional style without global variables (you don't need them).

Then switch to using std::shared_ptrs.

answered Mar 27 '17 at 13:31

Berkus

345
1
8

Shame `std::make_unique` doesn't exist in C++11, so you're going to have to see "`new`" in your code at least once ;) – Lightness Races in Orbit Mar 27 '17 at 13:33
You're still using `new` even in that which could cause a leak. the real `std::make_unique` will not leak on failed construction. – NathanOliver Mar 27 '17 at 13:38
True, but this can be fixed. I switched to c++14 so it's not a problem for me anymore. – Berkus Mar 27 '17 at 13:44
@Berkus: Exactly; that has a `new` in it and (as Nathan points out) a broken one. This is precisely my point! – Lightness Races in Orbit Mar 27 '17 at 14:55
@BoundaryImposition I don't see what you imply at. `make_unique` is part of c++14 and you don't have to see any `new`, broken or not. – Berkus Mar 28 '17 at 16:31
@Berkus: Your replacement for `make_unique` in C++11, posted in the answer above, has a `new` in it, and it is broken. – Lightness Races in Orbit Mar 28 '17 at 16:32
@BoundaryImposition It's an example. I do not want you to use it. JUST AN EXAMPLE. Write your own or use c++14. – Berkus Mar 28 '17 at 16:33
@Berkus: You said you didn't understand my comment. I simply explained it. Please don't shout at me. – Lightness Races in Orbit Mar 28 '17 at 16:33
It does not invalidate anything I said in the post. Watch the video. – Berkus Mar 28 '17 at 16:34
_"I do not want you to use it"_ People might use it when they see your answer; you should write a notice on it with a disclaimer so that they realise it's broken! – Lightness Races in Orbit Mar 28 '17 at 16:34
Are you happy now? – Berkus Mar 28 '17 at 16:35

Rama · Answer 2 · 2017-03-29T18:26:27.783

Your problem is that NumberT is 0 when you do your memory allocation.
When you do:

double* Kelvin=new double[0];

No initialization is performed, so the pointer has indeterminate value. Writing and reading this memory has undetermined behavior.

When you use delete[] on an pointer with indeterminate value your code probably will crash.

This issue is perfect for using a vector<double> instead of an double[] array, there is no reason not to use the dynamic memory handling facilities provided by this container.

Use as this example:

//Replace arrays with vectors
vector<double> Kelvin;
vector<double> Celsius;

//in your main to read user input
for (unsigned int i=0;i<NumberT;++i)
{
    double read;
    cin >> read;
    Kelvin.push_back(read);
}

//In your conversion function
for (std::vector<double>::iterator it = Kelvin.begin(); it != Kelvin.end(); ++it)
{
  Celsius.push_back((*it)-273.15);
}

Other tips:

-You dont need to declare global variables (you can declare it at the begining of your main()for example) and pass variables by referece, like This:

void Conversion (vector<double> & Kelvin, vector<double> & Celsius);

-When the vectors goes out of scope(at the end of your main()), they will do the delete work for you.

score -2 · Answer 3 · edited May 23 '17 at 11:54

As the other said. Your arrays initialization is the issue. In general, since you don't know the initial size of your elements I would immediately think about vectors. In that case, you could remove the "How many elements.." and introduce an exit character or parse X separated values.

Of course, using arrays is not wrong but you have to (re)initialize them with the correct size. Thus after you get the NumberT. Nevertheless, for array vs vector check this Arrays Vs Vectors

I have posted an implementation using vectors but still, is based on your approach.

(NOTE!: This code is not efficient since it does a lot of vector copies and that is to not introduce new concepts. Please search on your own for arguments by reference and std::move)

#include <iostream>
#include <vector>
#include <algorithm>

std::vector<double> ToCelcius(std::vector<double> kelvinValues)   //Conversion function
{
    std::vector<double> celciusValues;
    for (auto value : kelvinValues)
    {
        celciusValues.push_back(value - 273.15);
    }
    return celciusValues;
}


void PrintTemperatures(std::vector<double> kelvinSource, std::vector<double> celsiusSource)   //Print function
{
        for (uint16_t valueIndex = 0; valueIndex < kelvinSource.size(); valueIndex++)
        {
            std::cout << "The temperature is " << kelvinSource.at(valueIndex) << " [K], which is " << celsiusSource.at(valueIndex)<< " [C]" << std::endl;
        }
}


int main()                     //Main
{
    uint16_t numberOfValues;
    std::cout << "How many temperatures do you want to enter?" << "\n";
    std::cin >> numberOfValues;

    std::cout << "What are the temperatures?" << "\n";
    std::vector<double> kelvinValues;

    double value;
    for (uint16_t i = 0; i< numberOfValues; ++i)
    {
        if (std::cin >> value) {
            kelvinValues.push_back(value);
        }
        continue;
    }
    auto celciusValues = ToCelcius(kelvinValues);
    PrintTemperatures(kelvinValues, celciusValues);

    return 0;
}

Avoiding memory leak in C++?

3 Answers3