malloc problem in C++

[Jason]

I've inherited some old C code that I've been tasked to convert to C++.
I've been running into some problems with memory allocation.

First of all, is there any problems using "malloc" in C++? I know using
"new" is preferable, but for now I have to use what is provided.

Second, do FILE pointers need to have memory allocated?

My specific problem is this: (all code examples are abbreviated for
clarity)

I have a class

class TObjectDef
{
public:
char *cpFileName;
int iRecordLength;
int iCurrentRecord;
long lRecordCount;
FILE *tpFilePointer
char cpRecBuf[2048];
}

which is initialized

gsPpk = new TObjectDef();

then, much later in the code (about 80k lines later), a float array is
initialized from this structure

(definition
struct DataSegments
{
char caDataId[6];
int iDataCount;
float *fpArr1;
float *fpArr2;
}
)

like so

void vSetFpArray(str uct DataSegments *spDataSeg, float *fpArr1, float
*fpArr2)
{
int iDataCount;
int iDataIndex;

iDataCount = spDataSeg->iDataCount;
spDataSeg->fpArr1 = (float *)malloc(sizeof (float) * iDataCount);
spDataSeg->fpArr1 = (float *)malloc(sizeof (float) * iDataCount);

for (iDataIndex = 0; iDataIndex < iDataCount; iDataIndex++)
{
spDataSeg->fpArr1[iDataIndex] = fpArr1[iDataIndex];
spDataSeg->fpArr1[iDataIndex] = fpArr1[iDataIndex];
}
return;
}

The function is called like

vSetFpArray(&sD ataSegments[currInd],floatArray1,fl oatArray2);

where the variables are defined as
static DataSegments sDataSegments[512];
float floatArray1[1024];
float floatArray2[1024];

The function above gets called just fine 318 times. On the 319th iteration,
when "malloc" is called, it is overwriting part of the previously defined
class (which is global to this function). The tpFilePointer goes from being
NULL to being "1" after the first malloc, and "2" after the second. (as in,
pointing to memory address :00000002).

Can anyone see any obvious problems with all this that I may have
overlooked? (Besides the obvious problems with global classes and such.) I
apologize if this question didn't make any sense, I'm happy to provide
clarity.

- Jason

[Victor Bazarov]

Jason wrote:

I've inherited some old C code that I've been tasked to convert to
C++. I've been running into some problems with memory allocation.

First of all, is there any problems using "malloc" in C++? I know
using "new" is preferable, but for now I have to use what is provided.

No, there are no problems with 'malloc', except that it does not
initialise the memory it allocates.

Second, do FILE pointers need to have memory allocated?

FILE pointers are those you obtain from the system by means of the
'fopen' function calls. You need not allocate anything.

My specific problem is this: (all code examples are abbreviated for
clarity)

I have a class

class TObjectDef
{
public:
char *cpFileName;
int iRecordLength;
int iCurrentRecord;
long lRecordCount;
FILE *tpFilePointer
char cpRecBuf[2048];
}

which is initialized

gsPpk = new TObjectDef();

Assuming that 'gsPpk' is declared as a pointer to TObjectDef.

then, much later in the code (about 80k lines later), a float array is
initialized from this structure

(definition
struct DataSegments
{
char caDataId[6];
int iDataCount;
float *fpArr1;
float *fpArr2;
}
)

like so

void vSetFpArray(str uct DataSegments *spDataSeg, float *fpArr1, float
*fpArr2)
{
int iDataCount;
int iDataIndex;

iDataCount = spDataSeg->iDataCount;
spDataSeg->fpArr1 = (float *)malloc(sizeof (float) * iDataCount);
spDataSeg->fpArr1 = (float *)malloc(sizeof (float) * iDataCount);

I am sure you meant

spDataSeg->fpArr2 =

here, didn't you? Otherwise you assign to the same variable twice,
which means you leak memory and leave 'fpArr2' unchanged.

>
for (iDataIndex = 0; iDataIndex < iDataCount; iDataIndex++)
{
spDataSeg->fpArr1[iDataIndex] = fpArr1[iDataIndex];
spDataSeg->fpArr1[iDataIndex] = fpArr1[iDataIndex];

Again, those two statements are _exactly_the_sa me_. Did you mean
to do

spDataSeg->fpArr2[iDataIndex] = fpArr2[iDataIndex];

on the second line?

}
return;
}

The function is called like

vSetFpArray(&sD ataSegments[currInd],floatArray1,fl oatArray2);

where the variables are defined as
static DataSegments sDataSegments[512];
float floatArray1[1024];
float floatArray2[1024];

The function above gets called just fine 318 times. On the 319th
iteration, when "malloc" is called, it is overwriting part of the
previously defined class (which is global to this function). The
tpFilePointer goes from being NULL to being "1" after the first
malloc, and "2" after the second. (as in, pointing to memory address
:00000002).

<shrug Impossible to tell what that's due.

Can anyone see any obvious problems with all this that I may have
overlooked? (Besides the obvious problems with global classes and
such.) I apologize if this question didn't make any sense, I'm happy
to provide clarity.

Yes, the obvious problems I've pointed out. The non-obvious are
most likely beyond the posted code.

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask

[Jason]

"Victor Bazarov" <v.********@com Acast.netwrote in message
news:fa******** **@news.datemas .de...

>>[...]
First of all, is there any problems using "malloc" in C++? I know
using "new" is preferable, but for now I have to use what is provided.

No, there are no problems with 'malloc', except that it does not
initialise the memory it allocates.

But it _does_ allocate memory, as in it locks memory on the heap for that
specific pointer?

FILE pointers are those you obtain from the system by means of the
'fopen' function calls. You need not allocate anything.

Thanks, that's what I suspected.

>>[...]
gsPpk = new TObjectDef();

Assuming that 'gsPpk' is declared as a pointer to TObjectDef.

Correct.

>>[...]
spDataSeg->fpArr1 = (float *)malloc(sizeof (float) * iDataCount);
spDataSeg->fpArr1 = (float *)malloc(sizeof (float) * iDataCount);

I am sure you meant

spDataSeg->fpArr2 =

here, didn't you? Otherwise you assign to the same variable twice,
which means you leak memory and leave 'fpArr2' unchanged.

Yes, you are correct. I changed the variable names for this example, and
copy/pasted it into the code. Missed updating the second reference, I
apologize for the confusion. The old variable names are part of a
proprietary application, needed to change them for my own job security.

Again, those two statements are _exactly_the_sa me_. Did you mean
to do

spDataSeg->fpArr2[iDataIndex] = fpArr2[iDataIndex];

on the second line?

Again, I apoligize for the confusion.

>The function above gets called just fine 318 times. On the 319th
iteration, when "malloc" is called, it is overwriting part of the
previously defined class (which is global to this function). The
tpFilePointe r goes from being NULL to being "1" after the first
malloc, and "2" after the second. (as in, pointing to memory address
:00000002).

<shrug Impossible to tell what that's due.

Any suggestions on things I could be looking for while debugging? There are
no errors until it gets further down in the code, and an attempt is made to
initialize and use that file pointer.

>Can anyone see any obvious problems with all this that I may have
overlooked? (Besides the obvious problems with global classes and
such.) I apologize if this question didn't make any sense, I'm happy
to provide clarity.

Yes, the obvious problems I've pointed out. The non-obvious are
most likely beyond the posted code.

Besides my typos, everything looks fine? Thanks for your advice, that's
what I needed to know. I am still relatively new to the C++ environment,
being an old Delphi programmer, so outside perspective is always
appreciated.

- Jason

[Victor Bazarov]

Jason wrote:

"Victor Bazarov" <v.********@com Acast.netwrote in message
news:fa******** **@news.datemas .de...

>>[...]
First of all, is there any problems using "malloc" in C++? I know
using "new" is preferable, but for now I have to use what is
provided.

No, there are no problems with 'malloc', except that it does not
initialise the memory it allocates.

But it _does_ allocate memory, as in it locks memory on the heap for
that specific pointer?

Sure. If you look at some popular implementations of 'new' and
'new[]', you'll see that internally they use 'malloc', actually.

[..]

>>The function above gets called just fine 318 times. On the 319th
iteration, when "malloc" is called, it is overwriting part of the
previously defined class (which is global to this function). The
tpFilePoint er goes from being NULL to being "1" after the first
malloc, and "2" after the second. (as in, pointing to memory
address
00000002).

<shrug Impossible to tell what that's due.

Any suggestions on things I could be looking for while debugging?

If 'malloc' somehow overrides (steps onto) some memore you're still
using for some other object, it could be only two things: the heap
is corrupt (your program writes beyond the bounds of a dynamically
allocated object, thus crossing over to memory that doesn't really
belong to it, OR your "part of previously defined class" was somehow
transferred from under your control and it's now part of the heap
that 'malloc' is free to reuse.

What you should do is to get the tool that would allow you to debug
memory allocations and access. Rational Purify, BoundsChecker, come
to mind.

There are no errors until it gets further down in the code, and an
attempt is made to initialize and use that file pointer.

Memory access troubles are the worst, and if you get them under your
control, you're going to be very happy.

>>Can anyone see any obvious problems with all this that I may have
overlooked? (Besides the obvious problems with global classes and
such.) I apologize if this question didn't make any sense, I'm
happy to provide clarity.

Yes, the obvious problems I've pointed out. The non-obvious are
most likely beyond the posted code.

Besides my typos, everything looks fine? Thanks for your advice,
that's what I needed to know. I am still relatively new to the C++
environment, being an old Delphi programmer, so outside perspective
is always appreciated.

Here is something you might want to consider: do NOT use dynamic
memory allocated directly by you, instead use standard containers.
Switch to using 'std::vector<fl oat>' instead of 'malloc'ed 'float*'.
Easier to debug, for sure.

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask

[Victor Bazarov]

Jason wrote:

"red floyd" <no*****@here.d udewrote in message
news:wd******** ***********@new ssvr12.news.pro digy.net...

>Also, malloc'ed memory is uninitialized, until you fill it with
something, it's UB to read that memory.

UB?

"Undefined behaviour"

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask

[Jason]

"Victor Bazarov" <v.********@com Acast.netwrote in message
news:fa******** **@news.datemas .de...

Jason wrote:

>UB?

"Undefined behaviour"

Well that makes a lot more sense than "Undistribu ted Budget".

*sigh*

- Jason

[Default User]

Jason wrote:

I've inherited some old C code that I've been tasked to convert to
C++. I've been running into some problems with memory allocation.

First of all, is there any problems using "malloc" in C++? I know
using "new" is preferable, but for now I have to use what is provided.

As the others have mentioned, malloc() will work for allocating raw
buffers. One difference between C and C++ is that C has automatic
conversion of void pointers to object pointers, while C++ does not.
It's possible that the old code could be written like:

int *p = malloc(8 * sizeof(int));

That won't work in C++. That being said, it's extremely common for C
code to cast the pointer from malloc() anyway, even though it's not
needed and not really a good idea in C, as it can hide a nasty error.

Brian

[Jason]

"Victor Bazarov" <v.********@com Acast.netwrote in message
news:fa******** **@news.datemas .de...

Jason wrote:

>But it _does_ allocate memory, as in it locks memory on the heap for
that specific pointer?

Sure. If you look at some popular implementations of 'new' and
'new[]', you'll see that internally they use 'malloc', actually.

Well that explains why I'm still getting the same error using "new" and
"delete".

If 'malloc' somehow overrides (steps onto) some memore you're still
using for some other object, it could be only two things: the heap
is corrupt (your program writes beyond the bounds of a dynamically
allocated object, thus crossing over to memory that doesn't really
belong to it, OR your "part of previously defined class" was somehow
transferred from under your control and it's now part of the heap
that 'malloc' is free to reuse.

The old code is horrendous, with objects being used before they exist, and
other inappropriate actions. I'm really not surprised that this is
happening, just confused about how to solve it with no *apparent* errors. I
was hoping that this was going to be more of an oversight on my part than
anything. The object that is being overwritten is most likely being
re-initialized somewhere. If I re-allocate an object doing:

gsPpk = new ObjectDef();

then later in the code do

gsPpk = new ObjectDef();

again, what happens? Is the original instance of the object lost and that
memory locked?

What you should do is to get the tool that would allow you to debug
memory allocations and access. Rational Purify, BoundsChecker, come
to mind.

Thanks, I will definitely check into some options.

Memory access troubles are the worst, and if you get them under your
control, you're going to be very happy.

c:\stupidprojec t>del /f /s *.*

would make me happy.

>Besides my typos, everything looks fine? Thanks for your advice,
that's what I needed to know. I am still relatively new to the C++
environment, being an old Delphi programmer, so outside perspective
is always appreciated.

Here is something you might want to consider: do NOT use dynamic
memory allocated directly by you, instead use standard containers.
Switch to using 'std::vector<fl oat>' instead of 'malloc'ed 'float*'.
Easier to debug, for sure.

If only it were that simple. If I could merely do a gloabl search/replace,
my life would be so much simpler.

- Jason

[Jason]

"Default User" <de***********@ yahoo.comwrote in message
news:5j******** *****@mid.indiv idual.net...

As the others have mentioned, malloc() will work for allocating raw
buffers. One difference between C and C++ is that C has automatic
conversion of void pointers to object pointers, while C++ does not.
It's possible that the old code could be written like:

int *p = malloc(8 * sizeof(int));

That won't work in C++. That being said, it's extremely common for C
code to cast the pointer from malloc() anyway, even though it's not
needed and not really a good idea in C, as it can hide a nasty error.

There were a few places in the old code like this, but things like that
throw compilation errors. The code now compiles fine, but I'm getting
memory problems at run-time.

- Jason

[Victor Bazarov]

Jason wrote:

"Victor Bazarov" <v.********@com Acast.netwrote in message
news:fa******** **@news.datemas .de...

>Jason wrote:

>>But it _does_ allocate memory, as in it locks memory on the heap for
that specific pointer?

Sure. If you look at some popular implementations of 'new' and
'new[]', you'll see that internally they use 'malloc', actually.

Well that explains why I'm still getting the same error using "new"
and "delete".

Make sure you do the right 'delete' -- "delete[]" for arrays.

>If 'malloc' somehow overrides (steps onto) some memore you're still
using for some other object, it could be only two things: the heap
is corrupt (your program writes beyond the bounds of a dynamically
allocated object, thus crossing over to memory that doesn't really
belong to it, OR your "part of previously defined class" was somehow
transferred from under your control and it's now part of the heap
that 'malloc' is free to reuse.

The old code is horrendous, with objects being used before they
exist, and other inappropriate actions. I'm really not surprised
that this is happening, just confused about how to solve it with no
*apparent* errors. I was hoping that this was going to be more of an
oversight on my part than anything. The object that is being
overwritten is most likely being re-initialized somewhere. If I
re-allocate an object doing:
gsPpk = new ObjectDef();

then later in the code do

gsPpk = new ObjectDef();

again, what happens?

Nothing awful, most likely.

Is the original instance of the object lost and
that memory locked?

The original instance is "lost" in terms of gaining access to it, but
the memory is still allocated. This is what's known to be "a memory
leak", since the memory is (and cannot be) deallocated because the
pointer to that memory has been lost when you override the value in
the 'gsPpk'. Unless, of course, somebody holds onto the value of the
pointer, which in itself may be a BAD IDEA(tm).

[..]

>Memory access troubles are the worst, and if you get them under your
control, you're going to be very happy.

c:\stupidprojec t>del /f /s *.*

would make me happy.

I hear you.

>>Besides my typos, everything looks fine? Thanks for your advice,
that's what I needed to know. I am still relatively new to the C++
environment , being an old Delphi programmer, so outside perspective
is always appreciated.

Here is something you might want to consider: do NOT use dynamic
memory allocated directly by you, instead use standard containers.
Switch to using 'std::vector<fl oat>' instead of 'malloc'ed 'float*'.
Easier to debug, for sure.

If only it were that simple. If I could merely do a gloabl
search/replace, my life would be so much simpler.

Refactoring old C code is not much fun. But it's often what you have
to do and it's better done right. That includes stopping the use of
"naked" pointers for arrays and switching to using proper standard
containers. While it looks daunting at first, you will save more time
later if you have done that.

Talk you your boss, ask for an extension of couple of days, back your
project up in a reliable location, then comb those "naked" pointers
out and replace them with vectors. You'll be glad you do.

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask