Memory leaks with realloc()

[Marlene Stebbins]

The bigint struct defines a big integer and represents it as a string of
characters:

typedef struct bigint {
int sign;
int size;
int initflag;
char *number;
} bigint;

In the code below I am initializing a bigint to zero with malloc() then
realloc()-ing in bigAlloc() and the two assignment functions. Do I have
a memory leak anywhere in this code? In particular, once the bigint has
been initialized do I need to free the currently allocated memory before
reallocating in bigAlloc() and the bigAssign()s?

/* initialize a bigint to zero */
void bigInit(bigint *a)
{
if((a->number = malloc(2))==NUL L)
{
fprintf(stderr, "malloc failed in bigInit()\n");
exit(EXIT_FAILU RE);
}
a->sign = POS;
a->size = 1;
a->initflag = 1;
a->number[0] = '0';
a->number[1] = '\0';
return;
}

/* allocate memory for a bigint */
void bigAlloc(bigint *a, int bytes)
{
if(a->initflag != 1)
{
bigInit(a);
}

if((a->number = realloc(a->number, bytes * sizeof(*a->number)))==NUL L)
{
fprintf(stderr, "bigAlloc failed\n");
exit(EXIT_FAILU RE);
}
return;
}

/* bigint assignment: a = b */
void bigAssign(bigin t *a, bigint b)
{
bigAlloc(a, b.size+1);
if(b.sign == POS) a->sign = POS;
if(b.sign == NEG) a->sign = NEG;
a->size = b.size;
strcpy(a->number, b.number);
return;
}

/* assignment */
void bigAssignByRef( bigint *a, bigint *b)
{
bigAlloc(a, b->size+1);
if(b->sign == POS) a->sign = POS;
if(b->sign == NEG) a->sign = NEG;
a->size = b->size;
strcpy(a->number, b->number);
return;
}

[Eric Sosman]

Marlene Stebbins wrote:

[...] Do I have
a memory leak anywhere in this code?
You have a potential leak in bigAlloc() at
if((a->number = realloc(a->number, ...))==NULL)
.... because if realloc() fails you will overwrite the still-
valid pointer `a->number' with NULL. Unless you have another
pointer somewhere that retains the former value of `a->number',
you have lost all ability to do anything with the memory that
`a->number' used to point to; you have "leaked" that memory.

However, in your case the leak is not serious, because if
realloc() fails you call `exit(EXIT_FAIL URE)'. The duration
of the leak will be short, and its effect on the subsequent
operation of the program practically nil ;-)
In particular, once the bigint has
been initialized do I need to free the currently allocated memory before
reallocating in bigAlloc() and the bigAssign()s?
You should *not* free() a memory area and then try to
realloc() it. The first argument to realloc() must be NULL
or must be a pointer to a currently-valid memory area as
allocated by malloc(), calloc(), or a previous realloc() --
if you hand realloc() a pointer to anything else, you get
undefined behavior.

In your case, though, it might make more sense to free()
and malloc() than to realloc() -- you do not need to preserve
the former contents of the memory `a->number' points to; you
are about to insert brand-new content. Thus, whatever extra
work realloc() undertakes to preserve the original bytes is
simply a waste. Indeed, free()/malloc() may succeed where a
realloc() would have failed; for your purposes realloc() is
not only less efficient, but less effective.

One additional remark: Why write
if(b.sign == POS) a->sign = POS;
if(b.sign == NEG) a->sign = NEG;

.... instead of `b.sign = a->sign;'?

--
Er*********@sun .com

[Krishanu Debnath]

Eric Sosman wrote:

[snip]

One additional remark: Why write

if(b.sign == POS) a->sign = POS;
if(b.sign == NEG) a->sign = NEG;

.... instead of `b.sign = a->sign;'?

You meant a->sign = b.sign, I bet.

Krishanu

[Marlene Stebbins]

Eric Sosman wrote:

One additional remark: Why write

if(b.sign == POS) a->sign = POS;
if(b.sign == NEG) a->sign = NEG;

... instead of `b.sign = a->sign;'?

Because I'm simple-minded. After I get everything working I'll try to
get rough edges like that cleaned up. Thanks for your help.

Marlene

[Villy Kruse]

On Wed, 23 Feb 2005 21:37:31 +0530,
Krishanu Debnath <kr******@cal.i nterrasystems.c om> wrote:

Eric Sosman wrote:

[snip]

One additional remark: Why write

if(b.sign == POS) a->sign = POS;
if(b.sign == NEG) a->sign = NEG;

.... instead of `b.sign = a->sign;'?

You meant a->sign = b.sign, I bet.

Is it even remotely possible that b.sign could be neither POS nor NEG,
and what should be done in this case?

Villy

[Eric Sosman]

Villy Kruse wrote:

On Wed, 23 Feb 2005 21:37:31 +0530,
Krishanu Debnath <kr******@cal.i nterrasystems.c om> wrote:

Eric Sosman wrote:

One additional remark: Why write
if(b.sign == POS) a->sign = POS;
if(b.sign == NEG) a->sign = NEG;

.... instead of `b.sign = a->sign;'?
You meant a->sign = b.sign, I bet.

Krishanu wins that bet ...
Is it even remotely possible that b.sign could be neither POS nor NEG,
and what should be done in this case?

I thought about that, but decided not to worry about
it for two reasons: First, the obvious intent of the function
was to make a new copy of the `b' struct and its contents,
and second, leaving `a->sign' uninitialized (freshly obtained
from malloc() and hence with indeterminate content) could not
possibly be "right."

--
Eric Sosman
es*****@acm-dot-org.invalid

[Villy Kruse]

On Thu, 24 Feb 2005 08:44:27 -0500,
Eric Sosman <es*****@acm-dot-org.invalid> wrote:

Is it even remotely possible that b.sign could be neither POS nor NEG,
and what should be done in this case?

I thought about that, but decided not to worry about
it for two reasons: First, the obvious intent of the function
was to make a new copy of the `b' struct and its contents,
and second, leaving `a->sign' uninitialized (freshly obtained
from malloc() and hence with indeterminate content) could not
possibly be "right."

So you could assert (b.sign == POS || b.sign == NET); Perhaps
that is already done earlier in the code. Defensive coding helps
find bugs.
Villy

[Marlene Stebbins]

Villy Kruse wrote:

On Thu, 24 Feb 2005 08:44:27 -0500,
Eric Sosman <es*****@acm-dot-org.invalid> wrote:

Is it even remotely possible that b.sign could be neither POS nor NEG,
and what should be done in this case?

I thought about that, but decided not to worry about
it for two reasons: First, the obvious intent of the function
was to make a new copy of the `b' struct and its contents,
and second, leaving `a->sign' uninitialized (freshly obtained
from malloc() and hence with indeterminate content) could not
possibly be "right."

So you could assert (b.sign == POS || b.sign == NET); Perhaps
that is already done earlier in the code. Defensive coding helps
find bugs.
Villy

Sign is set to POS by bigInit. If by some chance a bigint was not
initialized after it was declared, it would be initialialized by
bigAlloc which looks at initflag and calls bigInit if initflag != 1.
BTW, the assert statement you have written above wouldn't help much. :-)
Speaking of finding bugs, you can see the code for the entire big number
library at:

http://members.shaw.ca/bystander/bignum.html

If you have some time on your hands, take a look at it and see if you
can find the bug in division. Everything else works. If you would like
to email me, you can figure out my email address from the URL.

Marlene