memory leaks

[THEARTOFWEB]

Hi,

do you think the following code could lead to memory leaks?

#define _CRT_SECURE_NO_WARNINGS
#include <windows.h>
#include <vector>
#include <cstdlib>
#include <ctime>
#include <cstdio>
#include <boost/circular_buffer.hpp>
using namespace std;
using namespace boost;

char * getDateTime(void);
const short numbuff = 5;
const short buflen  = 30;

typedef struct
{
	unsigned char * pData;
	unsigned short bufferLength;
	unsigned short bytesRecorded;
	bool flag;
} Buffer;

int main()
{
	circular_buffer<Buffer*> cb(numbuff);
	circular_buffer<Buffer*>::const_iterator it;

	// fill buffer
	for(int i = 0; i<numbuff; i++)
	{
		// set up buffer
		Buffer *buff       = new Buffer;
		ZeroMemory(buff, sizeof(Buffer));

		buff->bufferLength  = buflen;
		buff->bytesRecorded = buflen;
		buff->flag          = true;
		buff->pData         = new unsigned char[buflen];
		buff->pData         = reinterpret_cast<unsigned char *>(getDateTime());

		// push buffer
		cb.push_back(buff);

		Sleep(1000);
	}

	// show elements

	for(int i = 0; i<(int)cb.size(); i++)
	{
		printf("%s\n", cb[i]->pData);
	}

	system("pause");
	return EXIT_SUCCESS;
}

// getDateTime (Fri, 10 Oct 2008 14:41:59 GMT)
char * getDateTime(void)
{
	time_t rawtime;
	struct tm * timeinfo;
	time(&rawtime);
	timeinfo = gmtime(&rawtime);
	char * buffer = new char[30];
	strftime(buffer,30,"%a, %d %b %Y %X GMT",timeinfo);
	return buffer;
}

I cannot delete "buff" since its scope is inside the first for loop...

[THEARTOFWEB]

Hi,

do you think the following code could lead to memory leaks? it creates a
circualr buffer made up of 5 element then it pushes 10 elements (overwriting
the first 5) when overwriting , do you think the eldest pointed memory will
be overwritten, or a new pointer in memory will be written?

#define _CRT_SECURE_NO_WARNINGS
#include <windows.h>
#include <vector>
#include <cstdlib>
#include <ctime>
#include <cstdio>
#include <boost/circular_buffer.hpp>
using namespace std;
using namespace boost;

char * getDateTime(void);
const short numbuff = 5;
const short buflen  = 30;

typedef struct
{
 unsigned char * pData;
 unsigned short bufferLength;
 unsigned short bytesRecorded;
 bool flag;
} Buffer;

int main()
{
 circular_buffer<Buffer*> cb(numbuff);
 circular_buffer<Buffer*>::const_iterator it;

 printf("Push elements:\n");
 // fill buffer
 for(int i = 0; i<10; i++)
 {
  // set up buffer
  Buffer *buff       = new Buffer;
  ZeroMemory(buff, sizeof(Buffer));

  buff->bufferLength  = buflen;
  buff->bytesRecorded = buflen;
  buff->flag          = true;
  buff->pData         = new unsigned char[buflen];
  buff->pData         = reinterpret_cast<unsigned char *>(getDateTime());

  printf("%s\n", buff->pData);

  // push buffer
  cb.push_back(buff);

  Sleep(1000);
 }

 printf("\nShow elements:\n");

 // show elements
 for(int i = 0; i<static_cast<int>(cb.size()); i++)
 {
  printf("%s\n", cb[i]->pData);
 }

 system("pause");
 return EXIT_SUCCESS;
}

// getDateTime (Fri, 10 Oct 2008 14:41:59 GMT)
char * getDateTime(void)
{
 time_t rawtime;
 struct tm * timeinfo;
 time(&rawtime);
 timeinfo = gmtime(&rawtime);
 char * buffer = new char[30];
 strftime(buffer,30,"%a, %d %b %Y %X GMT",timeinfo);
 return buffer;
}

// thanks

[Skizmo]

buff->pData = new unsigned char[buflen];
buff->pData = reinterpret_cast<unsigned char *>(getDateTime());

This leaks. pData is overwritten the 2nd time so the memory you created the 1st time is gone.
Also... who frees the memory that 'getDateTime' has created.

[THEARTOFWEB]

This leaks. pData is overwritten the 2nd time so the memory you created the 1st time is gone.
Also... who frees the memory that 'getDateTime' has created.

by the way, how can I change getDateTime() to return char[30] ?? I mean the return type...

thanks

[VictorN]

by the way, how can I change getDateTime() to return char[30] ?? I mean the return type...

thanks

For example:

 void getDateTime( char *buffer)
{
 time_t rawtime;
 struct tm * timeinfo;
 time(&rawtime);
 timeinfo = gmtime(&rawtime);

 strftime(buffer,30,"%a, %d %b %Y %X GMT",timeinfo);
}

Now you can:

char buffer[30];
getDateTime(buffer);

[VictorN]

All that you newed you *must* delete. Period.
If you don't delete what you have newed then you produce memory leaks.

[THEARTOFWEB]

All that you newed you *must* delete. Period.
If you don't delete what you have newed then you produce memory leaks.

Ok. As I was suggested in this very forum...I am going to use vector to collect chars...is that ok? and no pointers anymore...

[VictorN]

...I am going to use vector to collect chars...is that ok? and no pointers anymore...

Yes, it is definetly a very good idea!

[THEARTOFWEB]

For instance...do you think the following could be better approach?

struct Buffer
{
public:
	vector<unsigned char> vChar;
	unsigned int bufferLength;
	unsigned int bytesRecorded;
	Buffer() : bytesRecorded(0), bufferLength(0), vChar(NULL) { };
};

int main()
{
	circular_buffer<Buffer> cb(numbuff);
	circular_buffer<Buffer>::const_iterator it;

	(...)
}

thanks

[hoxsiew]

No memory leaks that I see, but certainly undefined behavior as your "char szTime" goes out of scope when your for loop finishes.

Why not declare your "struct Buffer" with a fixed size "payload":

struct Buffer
{
public:
	char payload[30];
	int bufferLength;
	int bytesRecorded;
	int user;
	Buffer() : bytesRecorded(0), bufferLength(0), user(0), payload(NULL) { };
};

Edit: remove the "payload(NULL)" form your constructor too.

[THEARTOFWEB]

Ok, so all I can do is the following:

#include <windows.h>
#include <vector>
#include <cstdlib>
#include <ctime>
#include <cstdio>
#include <boost/circular_buffer.hpp>
using namespace std;
using namespace boost;

void getDateTime(char * szTime);
const int numbuff = 3;
const int buflen  = 30;

struct Buffer
{
public:
	char payload[30];
	int bufferLength;
	int bytesRecorded;
	int user;
	Buffer() : bufferLength(0), bytesRecorded(0), user(0) { }
};

int main()
{
	circular_buffer<Buffer> cb(numbuff);

	// Insert elements
	printf("Push elements:\n");
	for(int i = 0; i<5; i++)
	{
		// Get time
		char szTime[30]; getDateTime(szTime);

		// Init Buff
		Buffer buff;
		ZeroMemory(&buff, sizeof(Buffer));

		memcpy((void*)buff.payload, szTime, buflen);
		buff.user          = i;
		buff.bufferLength  = buflen;
		buff.bytesRecorded = buflen;

		cb.push_back(buff);

		printf("%s\n", buff.payload);
		Sleep(1000);
	}

	// Show elements:
	printf("Show elements:\n");
	for(int i = 0; i<(int)cb.size(); i++)
	{
		printf("%s\n", cb[i].payload);
	}

	system("pause");
	return EXIT_SUCCESS;
}

void getDateTime(char * szTime)
{
	time_t rawtime = time(NULL);
	struct tm timeinfo;
	gmtime_s(&timeinfo, &rawtime);
	strftime(szTime, 30, "%a, %d %b %Y %X GMT", &timeinfo);
}

so I will set payload to contains a loto of chars like:

char payload[4096]

then if payload is less the 4096 bytes long I will zero fill or read only a part of that

[Joeman]

Looks like you aren't taking advantage of c++ at all. this looks like c with classes approach with boost?

#include <windows.h>

#include <ctime>
#include <string>
#include <iostream>
#include <boost/circular_buffer.hpp>
#include <boost/foreach.hpp>

using namespace std;
using namespace boost;

string getDateTime()
{
	time_t rawtime = time(NULL);
	tm* timeinfo = gmtime(&rawtime);
	char szTime[30];
	strftime(szTime, 30, "%a, %d %b %Y %X GMT", timeinfo);
	return szTime;
}
 
struct Buffer 
{
	string payload;
	int user;
	Buffer() : payload(), user() { }
	Buffer( const string& payload, int user ) : payload(payload), user(user) { }
};
 
int main()
{
	const int numbuff = 3;
	circular_buffer<Buffer> cb(numbuff);
 
	// Insert elements
	cout << "Push elements:" << endl;
 
	for( int i = 0; i < 5; i++ )
	{
		cb.push_back( Buffer( getDateTime(), i ) );
		Sleep(1000);
	}
 
	// Show elements:
	cout << "Show elements:" << endl;
 
	BOOST_FOREACH( Buffer Record, cb )
	cout << Record.payload << endl;
 
	system("pause");
	return 0;
}

This is more c++ than the c approach , but understanding how and why to use string, constructors, containers and etc will take time. If this isn't what you require, just state what is wrong with the above code.

[THEARTOFWEB]

This is more c++ than the c approach , but understanding how and why to use string, constructors, containers and etc will take time. If this isn't what you require, just state what is wrong with the above code.

I might deal with /char/ (or unsigned char) data returned by some other functions....

I know I could cast to char to string...Yet, I'd like to stick with char for the moment

[THEARTOFWEB]

I mean, binary string mostly (unsigned char) the getDateTime() is just a way to fill the buffer with something for the moment...in the near future I'll be dealing with binary string only in my application

[THEARTOFWEB]

This is more c++ than the c approach , but understanding how and why to use string, constructors, containers and etc will take time. If this isn't what you require, just state what is wrong with the above code.

I think for the moment I'll use this:

#include <windows.h>
#include <cstdlib>
#include <ctime>
#include <cstdio>
#include <cstring>
#include <boost/circular_buffer.hpp>
using namespace std;
using namespace boost;

void getDateTime(char * szTime);
const int numbuff = 3;
const int buflen  = 30;

struct cBuffer
{
	char data[1024];
	int bytesRecorded;
	bool flag;
	cBuffer(char * data_, int bytesRecorded_, bool flag_) :
	bytesRecorded(bytesRecorded_), flag(flag_)
	{
		memcpy(static_cast<void *>(data), static_cast<void *>(data_), bytesRecorded);
	}
};


int main()
{
	circular_buffer<cBuffer> cb(numbuff);

	// Insert elements
	printf("Push elements:\n");
	for(int i = 0; i < 5; i++)
	{
		char szTime[30]; getDateTime(szTime);

		cb.push_back(cBuffer(szTime, 30, true));

		printf("%s\n", szTime);

		Sleep(1000);
	}

	// Show elements:
	printf("Show elements:\n");
	for(int i = 0; i<(int)cb.size(); i++)
	{
		printf("%s\n", cb[i].data);
	}

	system("pause");
	return EXIT_SUCCESS;
}

void getDateTime(char * szTime)
{
	time_t rawtime = time(NULL);
	struct tm timeinfo;
	gmtime_s(&timeinfo, &rawtime);
	strftime(szTime, 30, "%a, %d %b %Y %X GMT", &timeinfo);
}

this time I think I won't be having any memory leaks!