malloc in c++ class, is it getting deallocated

[pdk5]

Hello,

I am trying to update a test program class, to my program

In that, I can see a buffer is allocated with malloc. But doesnot call free anywhere !. It looks like going to be memry leak. Am i right ?

#pragma once
#include <string>
#include <filesystem>
#include "algrithm/general/pnt2d.h"
#include "windows.h"
#include <math.h>
#include <iostream>
#include <fstream>
#include <iomanip> 
#include <boost/algorithm/string.hpp>

using namespace std;

class DumpBilHelper
{
	
public:
	float *buffer;
	
	float EastMin, EastMax, NorthMin, NorthMax, binSize, E, N;
	size_t nRows, nCols, iR, iC;
	size_t nBins;
	string BYTEORDER, LAYOUT, PIXELTYPE;
	string wktProjection;
	long nBands, nBits, BandRowBytes, TotalRowBytes, UTMZone;
	float NoData;
	bool HeaderOK;
	string fnBIL;

	DumpBilHelper();

	DumpBilHelper(float inEastMin, float inEastMax, float inNorthMin, float inNorthMax, int inUTMZone, float inBinSize);
	
	void Init(float inEastMin, float inEastMax, float inNorthMin, float inNorthMax, int inUTMZone, float inBinSize);
	void SaveBIL(string FileName);
	void LoadHeader(string FileName);
	
	void SetEN(float E, float N, float value);
	void SetEN(float E, float N);
	void SetRC(int R, int C, float value);
	void AddRC(int R, int C, float value);
	float GetEN(float E, float N);
	float GetRC(int R, int C);
	void Clear();
};

In the .cpp

#include "DumpBilHelper.h"

DumpBilHelper::DumpBilHelper()
{

}
DumpBilHelper::DumpBilHelper(float inEastMin, float inEastMax, float inNorthMin, float inNorthMax, int inUTMZone, float inBinSize)
{
	this->Init(inEastMin, inEastMax, inNorthMin, inNorthMax, inUTMZone, inBinSize);
}

void DumpBilHelper::Init(float inEastMin, float inEastMax, float inNorthMin, float inNorthMax, int inUTMZone, float inBinSize)
{	
	/// initialize ESRI BIL array and metadata
	binSize = inBinSize;
	UTMZone = inUTMZone;

	EastMin = floor(inEastMin / binSize) * binSize;
	EastMax = floor(inEastMax / binSize) * binSize;
	NorthMin = floor(inNorthMin / binSize) * binSize;
	NorthMax = floor(inNorthMax / binSize) * binSize;
	NoData = -9999.0f;

	nRows = (NorthMax - NorthMin) / binSize + 1;
	nCols = (EastMax - EastMin) / binSize + 1;
	BandRowBytes = nCols * sizeof(float);
	TotalRowBytes = BandRowBytes;
	nBits = sizeof(float)*8;
	nBins = nRows * nCols;
	
//	BOOST_LOG_TRIVIAL(debug) << "Malloc " << nBins << " " << nRows << "x" << nCols;
	buffer = (float *) malloc(nBins * sizeof(float));
	if (buffer == NULL)
	{ 
//		BOOST_LOG_TRIVIAL(error) << " Malloc failed for " << (size_t)(nBins * sizeof(float) / 1024 / 1024 / 1024) << " GB";
		throw("Malloc failed");
	}
	this->Clear();
}

void DumpBilHelper::SaveBIL(string Filename)
{
	ofstream HDR, fBIL;
	string sBIL, sHDR;
//	BOOST_LOG_TRIVIAL(debug) << "Range: " << fixed << setprecision(2) << EastMin << " " << EastMax << " " << NorthMin << " " << NorthMax << ", Size: " << nRows << "x" << nCols << ", Resolution " << binSize << ", NODATA "<< NoData;
	
	sHDR = Filename;
	sHDR.append(".hdr");
	//BOOST_LOG_TRIVIAL(debug) << "Creating ESRI BIL header file " << sHDR;
	HDR.open(sHDR);

	HDR << "BYTEORDER            I" << endl;
	HDR << "LAYOUT               BIL" << endl;
	HDR << "NROWS                " << nRows << endl;
	HDR << "NCOLS                " << nCols << endl;
	HDR << "NBANDS               1" << endl;
	HDR << "NBITS                32" << endl;
	HDR << "BANDROWBYTES         " << nCols * sizeof(float) << endl;
	HDR << "TOTALROWBYTES        " << nCols * sizeof(float) << endl;
	HDR << "PIXELTYPE            FLOAT" << endl;
	HDR << "ULXMAP               " << fixed << setprecision(2) << EastMin + binSize / 2 << endl;
	HDR << "ULYMAP               " << NorthMax - binSize / 2 << endl;
	HDR << "XDIM                 " << binSize << endl;
	HDR << "YDIM                 " << binSize << endl;
	HDR << "NODATA               " << NoData << endl;

	HDR.close();

	// saving raster data file
	sBIL = Filename;
	sBIL.append(".bil");
//	BOOST_LOG_TRIVIAL(debug) << "Creating ESRI BIL data file " << sBIL;

	fBIL.open(sBIL, ios::out | ios::binary);
	fBIL.write((char *)buffer, nRows*nCols * sizeof(float));
	fBIL.close();
}


void DumpBilHelper::Clear()
{
	for (iC = 0; iC < nRows*nCols; iC++)
		buffer[iC] = 0;

}
void DumpBilHelper::SetEN(float E, float N, float value)
{
	SetEN(E, N);
	// cout << E << '\t' << N << '\t' << iR << '\t' << iC << endl;
	SetRC(iR, iC, value);	
}
void DumpBilHelper::SetEN(float inE, float inN)
{
// just calculating row and column, positioning "cursor" on it, no change
	iR = (NorthMax - inN ) / binSize;
	iC = (inE - EastMin ) / binSize;
	E = floor(inE / binSize) * binSize + binSize / 2;
	N = floor(inN / binSize) * binSize + binSize / 2;

}

void DumpBilHelper::SetRC(int R, int C, float value)
{
	if ((R <= nRows) && (C <=nCols) && (R>=0) && (C>=0) )
		buffer[R*nCols + C] = value;
}

void DumpBilHelper::AddRC(int R, int C, float value)
{
	if ((R <= nRows) && (C <= nCols) && (R >= 0) && (C >= 0))
		buffer[R*nCols + C] += value;
}

float DumpBilHelper::GetEN(float E, float N)
{
	iR = (NorthMax - N) / binSize;
	iC = (E - EastMin) / binSize;
	return GetRC(iR, iC);
}

float DumpBilHelper::GetRC(int R, int C)
{
	if ((R <= nRows) && (C <= nCols))
	{
		E = EastMin + C * binSize + binSize / 2;
		N = NorthMax - R * binSize - binSize / 2;
		return buffer[R * nCols + C];
	}
	else
		return NAN;
		
}

void DumpBilHelper::LoadHeader(string FileName)
{
	filesystem::path p(FileName);
	string hdrFileName, Line;
	vector<string> tokens;
	
	fnBIL = FileName;

	

//	BOOST_LOG_TRIVIAL(info) << "Using coverage file " << FileName;

	hdrFileName = p.replace_extension(".hdr").string();
	
	ifstream HDR;
	HDR.open(hdrFileName);
	while (getline(HDR, Line))
	{
		// convert to the uppercase, and split into attribute/value pair
		transform(Line.begin(), Line.end(), Line.begin(), ::toupper);
		boost::split(tokens, Line, boost::is_any_of("\t "), boost::token_compress_on);
		
		if (tokens.size() == 2)
		{
			// as expected
			if (tokens[0] == "BYTEORDER")
			{
				BYTEORDER = tokens[1];
				continue;
			}
			if (tokens[0] == "LAYOUT")
			{
				LAYOUT = tokens[1];
				continue;
			}
			if (tokens[0] == "NROWS")
			{
				nRows = stol(tokens[1]);
				continue;
			}
			if (tokens[0] == "NCOLS")
			{
				nCols = stol(tokens[1]);
				continue;
			}
			if (tokens[0] == "NBITS")
			{
				nBits = stol(tokens[1]);
				continue;
			}
			if (tokens[0] == "PIXELTYPE")
			{
				PIXELTYPE = tokens[1];
				continue;
			}
			if (tokens[0] == "ULXMAP")
			{
				EastMin= stol(tokens[1]);
				continue;
			}
			if (tokens[0] == "ULYMAP")
			{
				NorthMax = stol(tokens[1]);
				continue;
			}
			if (tokens[0] == "XDIM")
			{
				binSize = stoi(tokens[1]);
				continue;
			}
			if (tokens[0] == "NODATA")
			{
				NoData = stof(tokens[1]);
				continue;
			}
			if (tokens[0] == "TOTALROWBYTES")
			{
				TotalRowBytes = stol(tokens[1]);
				continue;
			}
			if (tokens[0] == "BANDROWBYTES")
			{
				BandRowBytes = stol(tokens[1]);
				continue;
			}
		}
		else
		{
		}
//			BOOST_LOG_TRIVIAL(error) << "Invalid ESRI HDR line " << Line;
	}
	//cout << "DBG " << EastMin << "," << NorthMax << "," << nRows << "," << nCols << "," << binSize << endl;
	HDR.close();
	HeaderOK = true;

}

I am thinking of adding the explicit destructor with the free to buffer ?

Any comments are very helpful.

Thankyou
pdk

[salem_c]

> I am thinking of adding the explicit destructor with the free to buffer ?
Absolutely yes, you need this.

Your default constructor should make sure the pointer is NULL.

Why can't you used std::array or std::vector ?
Both would solve the problem of having to write an explicit destructor.

Both allow you to get at the data for use in your write call.
https://en.cppreference.com/w/cpp/container/array/data
https://en.cppreference.com/w/cpp/container/vector/data

[pdk5]

Thankyou very much Salem.

Yes, thankyou for suggestion on std:array and std::vector. Not sure why it was not used there in the sample code I received. The buffer can be huge size. So will it be problem having big vector or array)

Btw, Is unique_ptr good idea when using on member variable? I am not sure whether there will be issues in copy constructor and may need to move operator ?

[salem_c]

> So will it be problem having big vector or array)
You can specify the initial size of the container, just like you specify the initial size of your malloc.

> Btw, Is unique_ptr good idea when using on member variable? I am not sure whether there will be issues in copy constructor
https://en.cppreference.com/w/cpp/la.../rule_of_three

If you use the std containers, you don't have to do anything and the problem solves itself.

If you "do your own thing" with malloc, then you probably have to implement all the ctors/dtors yourself.

[pdk5]

Thankyou very much Salem.

Yes using stl is better way.

But trying to explore the option of unique_ptr.. I am not sure, if i initialise in the Init function, it goes out of scope, when the SaveBIL(string Filename) is called.

[salem_c]

https://en.cppreference.com/w/cpp/memory/unique_ptr

Well the scope will be the lifetime of the class instance in which it resides.

The object is disposed of, using a potentially user-supplied deleter, by calling get_deleter()(ptr). The default deleter (std::default_delete) uses the delete operator, which destroys the object and deallocates the memory.

So in addition, you'd need to stop calling malloc, and use new[] instead.

[2kaud]

In C++ don't use malloc/calloc and free. Use new and delete. free/delete should be in the class destructor. The constructor(s) should set the pointer variable to either a valid value (from new) or nullptr. Note that nullptr is valid for delete.

But as has been mentioned, consider using std::unique_ptr. You then don't need to release memory in a destructor. When the class object goes out of scope the unique_ptr memory will automatically be released.

However, as pointed out, use std::vector as the size isn't known at compile time. The type of buffer will have to be changed but unless you're doing pointer arithmetic usage of buffer shouldn't need to be changed. Note that vector has a .data member that returns a pointer to the underlying data.

[pdk5]

Thankyou very much kaud for valuable inputs. I have incorporated the comments here. (currently using the unique_ptr)

[2kaud]

Also note that if you allocate memory in a class (new/malloc etc) then you need to provide a copy constructor and assignment - which may be simply =delete. If not then if a copy or assignment is done then a shallow memory copy is undertaken (just copy the pointer) rather than the required deep copy (allocate new memory and copy from the existing memory) - and chaos will ensure!. =delete will cause a compiler error if these operations are attempted.

[pdk5]

Also note that if you allocate memory in a class (new/malloc etc) then you need to provide a copy constructor and assignment - which may be simply =delete. If not then if a copy or assignment is done then a shallow memory copy is undertaken (just copy the pointer) rather than the required deep copy (allocate new memory and copy from the existing memory) - and chaos will ensure!. =delete will cause a compiler error if these operations are attempted.

Yes, of course. Thankyou very much. Yes, it will result in double delete (with the chaos). Thanks a lot kaud for explaining and taking time to help me.