malloc experiments

[Steve Zimmerman]

This program compiles fine, but are there any hidden dangers
in it, or is it ok?

Experiment 1 ##################################################

#include <stdio.h>
#include <stdlib.h>
#include <malloc.h>
#include <string.h>

int main()
{
struct pres {
char name[25];
struct pres *next;
};

struct pres *president;

president = (struct pres *)malloc(sizeof(struct pres));

strcpy(president->name, "George Washington");
president->next = (struct pres *)malloc(sizeof(struct pres));

printf("The first structure has been created:\n");
printf("president->name = %s\n", president->name);
printf("next structure address = %i\n", president->next);

return 0;
}

###################################################################

--Steve

 

[Mark A. Odell]

This program compiles fine, but are there any hidden dangers
in it, or is it ok?

Experiment 1 ##################################################

#include <stdio.h>
#include <stdlib.h>
#include <malloc.h>
#include <string.h>

int main()
{
struct pres {
char name[25];
struct pres *next;
};

struct pres *president;

president = (struct pres *)malloc(sizeof(struct pres));

This is a problem. If you had neglected to include <stdlib.h> there would
be no prototype for malloc(), thus it would return an integer. Your cast
would have hidden this mistake. In C (but not C++), we do not cast the
return value of malloc(). Second, it is best to specify the size of the
object not its type. That way, if the type of the object changes you don't
have to find the malloc() call and change that line too. E.g.

president = malloc(sizeof *president);

Next, be sure you were handed a valid pointer, malloc() can fail. If it
does, maybe you could return EXIT_FAILURE.

strcpy(president->name, "George Washington");

Use strncpy() and tell strncpy not to exceed sizeof president->name - 1.

president->next = (struct pres *)malloc(sizeof(struct pres));

(Same malloc() comments here too).

 

[Mikael Thorgren]

This program compiles fine, but are there any hidden dangers
in it, or is it ok?

Experiment 1 ##################################################

#include <stdio.h>
#include <stdlib.h>
#include <malloc.h>
#include <string.h>

Add this definition to avoid magic numbers
#define PRES_NAME_LEN 25

int main()
{
struct pres {
char name[25];
struct pres *next;
};

Use PRES_NAME_LEN instead of magic 25.

struct pres *president;

president = (struct pres *)malloc(sizeof(struct pres));

You have to check if malloc was successful.
The condition to continue execution is (president != NULL)
Just a minor remark;
You don't have to cast the result from malloc as long as stdlib.h is
included (and it should be!).

strcpy(president->name, "George Washington");

Oops.
What happens if the string exceeds the size of "name" field in the struct?
Something will be destroyed and overwritten!
Protect the field from overflow by using
strncpy(president->name, "George Washington", PRES_NAME_LEN);

president->next = (struct pres *)malloc(sizeof(struct pres));

printf("The first structure has been created:\n");
printf("president->name = %s\n", president->name);
printf("next structure address = %i\n", president->next);

return 0;
}

###################################################################

--Steve

Finally, you must free the memory you've allocated when this program will
transform from prototype to a real application.

Mikael T

 

[Eric Sosman]

This program compiles fine, but are there any hidden dangers
in it, or is it ok?

Experiment 1 ##################################################

#include <stdio.h>
#include <stdlib.h>
#include <malloc.h>

Non-Standard header. It's anybody's guess what it might do.

#include <string.h>

int main()
{
struct pres {
char name[25];
struct pres *next;
};

struct pres *president;

president = (struct pres *)malloc(sizeof(struct pres));

Two minor points: the cast is unnecessary, and it would be
better to write `sizeof *president'. One major gaffe: malloc()
can fail and return NULL, and you should check for this before
trying to use the possibly-not-allocated memory.

strcpy(president->name, "George Washington");
president->next = (struct pres *)malloc(sizeof(struct pres));

printf("The first structure has been created:\n");
printf("president->name = %s\n", president->name);
printf("next structure address = %i\n", president->next);

"%i" is for converting integers, not pointer values. You
need to use "%p", and you need to convert the pointer value
from a `struct pres*' to a `void*'.

 

[Fred L. Kleinschmidt]

This program compiles fine, but are there any hidden dangers
in it, or is it ok?

Experiment 1 ##################################################

#include <stdio.h>
#include <stdlib.h>
#include <malloc.h>
#include <string.h>

int main()
{
struct pres {
char name[25];
struct pres *next;
};

struct pres *president;

president = (struct pres *)malloc(sizeof(struct pres));

strcpy(president->name, "George Washington");

Danger - what if the name is longer than 24 characters?

 

[Daniel Haude]

On Wed, 03 Sep 2003 14:23:55 GMT,

This program compiles fine, but are there any hidden dangers
in it, or is it ok?

president = (struct pres *)malloc(sizeof(struct pres));

Who taught you to cast malloc()'s return value? Others have pointed out
the reasons why this is bad; I just want to know where you got the idea
from.

--Daniel

 

[Simon Biber]

Use strncpy() and tell strncpy not to exceed sizeof president->name - 1.

strncpy is the wrong tool for this job. If the string supplied is
too large it will leave the array unterminated, so there is not a
valid string. Use strncat instead, by zeroing the string first:
*president->name = 0; /* or president->name[0] = '\0'; */
strncat(president->name, "George Washington", sizeof president->name - 1);

This is undefined behaviour. The printf is looking for an int, but you
never supplied an int, you supplied a pointer to struct pres, which
may be stored in an entirely different place.

You should print pointers with the %p conversion, and remember to
cast the pointer to (void *), as that is the type expected by %p.

printf("next structure address = %p\n", (void*)president->next);

 

[Simon Biber]

Oops.
What happens if the string exceeds the size of "name" field in the struct?
Something will be destroyed and overwritten!
Protect the field from overflow by using
strncpy(president->name, "George Washington", PRES_NAME_LEN);

You want strncat instead, because strncpy doesn't null-terminate
the string if the length argument is exhausted. You also want to
specify PRES_NAME_LEN - 1 here, to leave room for the null char.

 

[Martin Dickopp]

struct pres {
char name[25];
struct pres *next;
};

Use PRES_NAME_LEN instead of magic 25.

strcpy(president->name, "George Washington");

What happens if the string exceeds the size of "name" field in the struct?
Something will be destroyed and overwritten!
Protect the field from overflow by using
strncpy(president->name, "George Washington", PRES_NAME_LEN);

This is still problematic, as `strncpy' does not write a '\0' character if
the source string has length equal to or greater than the length specified
in the third argument.

A better solution:

strncpy (president->name, "George Washington", PRES_NAME_LEN - 1);
president->name [PRES_NAME_LEN - 1] = '\0';

Martin

 

[bd]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

This program compiles fine, but are there any hidden dangers
in it, or is it ok?

Experiment 1 ##################################################

#include <stdio.h>
#include <stdlib.h>
#include <malloc.h>

No such header. malloc(), calloc(), and free() are in stdlib.h

#include <string.h>

int main()
{
struct pres {
char name[25];
struct pres *next;
};

struct pres *president;

president = (struct pres *)malloc(sizeof(struct pres));

The cast is unnecessary, and could prevent the compiler warning you if you
accidentally forget to include stdlib.h.

strcpy(president->name, "George Washington");
president->next = (struct pres *)malloc(sizeof(struct pres));

See above.

printf("The first structure has been created:\n");
printf("president->name = %s\n", president->name);
printf("next structure address = %i\n", president->next);

%i is for an int. president->next is a struct pres *. Try;
printf("next structure address = %p\n", (void *)president->next);

return 0;
}

- --
There's another way to survive. Mutual trust -- and help.
-- Kirk, "Day of the Dove", stardate unknown

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.2 (GNU/Linux)

iD4DBQE/VjfEx533NjVSos4RAnHNAJd0IU66hKYlV4fDyPlNCSTGqaOeAJ9oWWaS
BDUTzHsSo6ZqFeR3WZR8AA==
=msbg
-----END PGP SIGNATURE-----

 

[bd]

This program compiles fine, but are there any hidden dangers
in it, or is it ok?

Experiment 1 ##################################################

#include <stdio.h>
#include <stdlib.h>
#include <malloc.h>
#include <string.h>

int main()
{
struct pres {
char name[25];
struct pres *next;
};

struct pres *president;

president = (struct pres *)malloc(sizeof(struct pres));

strcpy(president->name, "George Washington");

Danger - what if the name is longer than 24 characters?

strcpy() like this is ok - the string literal requires 18 bytes. There's
little point in checking the size of a string if you know it beforehand,
after all.

 

[Matt Gregory]

You want strncat instead, because strncpy doesn't null-terminate
the string if the length argument is exhausted. You also want to
specify PRES_NAME_LEN - 1 here, to leave room for the null char.

Not really, because the memory returned from malloc is going to
contain garbage. In either case, you're still going to have to
insert a null somewhere.

 

[Simon Biber]

Not really, because the memory returned from malloc is
going to contain garbage. In either case, you're still
going to have to insert a null somewhere.

Yes, the choice is between the equivalent
president->name[0] = '\0';
strncat(president->name, "George Washington", PRES_NAME_LEN - 1);
and
strncpy (president->name, "George Washington", PRES_NAME_LEN - 1);
president->name[PRES_NAME_LEN - 1] = '\0';

Of those I would take the first, as strncpy makes me
instinctively suspicious when reading code.

 

[August Derleth]

On Wed, 03 Sep 2003 14:23:55 GMT,



Who taught you to cast malloc()'s return value? Others have pointed out
the reasons why this is bad; I just want to know where you got the idea
from.

It's common practice in C++, one of the differences between that
language and C and one of the reasons experienced C programmers blanch
at the meaningless term 'C/C++'.

 

[Martin Ambuhl]

This program compiles fine, but are there any hidden dangers
in it, or is it ok?

Experiment 1 ##################################################

#include <stdio.h>
#include <stdlib.h>
#include <malloc.h>

^^^^^^^^
Here's one obvious danger: that it might not compile with C compiler.
<malloc.h> is not one of the standard headers. malloc() and friends are
handled by <stdlib.h>

 

[Ravi Uday]

[snip]

struct pres *president;

This is a problem. If you had neglected to include <stdlib.h> there would
be no prototype for malloc(), thus it would return an integer. Your cast
would have hidden this mistake.

Can you elaborate on this. Isnt the prototype of malloc:
void *malloc( size_t size );

malloc returns a void pointer to the allocated space, or NULL if there is
insufficient memory available.
Since it returns a pointer to void if casted to the variables type thats in
use, that seems correct ! than not casting at all !.

In C (but not C++), we do not cast the

return value of malloc(). Second, it is best to specify the size of the
object not its type. That way, if the type of the object changes you don't
have to find the malloc() call and change that line too. E.g.

president = malloc(sizeof *president);

[ snip ]

 

[Arthur J. O'Dwyer]

Can you elaborate on this. Isnt the prototype of malloc:
void *malloc( size_t size );

malloc returns a void pointer to the allocated space,
or NULL if there is insufficient memory available.
Since it returns a pointer to void if casted to the
variables type thats in use, that seems correct !
than not casting at all !.

If you cast malloc's return value, that *is* correct C.
It *will* compile, and, if you haven't made any foolish
errors, it *will* do exactly what you expect. It's just
not the nicest possible way of doing things.

Consider the following program fragment:


#include <stdio.h>

int main(void)
{
int *p;
...
p = (int *) malloc(sizeof (int));
...
}

Do you see the bug?

If not, here it is: The programmer forgot to #include <stdlib.h>.
So there's no prototype for 'malloc' in scope inside 'main'.
So, when the compiler sees the call to 'malloc', it just *assumes*
that 'malloc' is a function taking one argument (of type 'size_t',
because that's what 'sizeof' yields), and returning an 'int'
(because that's just the traditional return type in C). So we
effectively have a program that looks like this:

int main(void)
{
int wrong_return_type_malloc(size_t);
int *p;
...
p = (int *) wrong_return_type_malloc( foo );
...
}

Now, look at that cast. In the current program, it's *hiding*
the error from the compiler. (Remember that a cast always says
to the compiler, "Be quiet and let me do this my way, right or
wrong.") So this program will compile without diagnostics, and
run, and crash, because we're expecting 'malloc' to return an
'int' (by mechanism A) when really it's returning a 'void *' (by
mechanism B). Really, some machines will actually use different
registers for different types, and so on.
So our program crashes at runtime -- or, insidiously, doesn't
crash until the day of the big demo, when the boss is around.

Now look at the same program, minus the cast (but *still* missing
the <stdlib.h> header file).

int main(void)
{
int wrong_return_type_malloc(size_t);
int *p;
...
p = wrong_return_type_malloc( foo );
...
}

*Now*, minus the cast, we have an assignment of an
'int' to an 'int *'. That's a problem that the
compiler is required to warn us about. So, instead
of the big-demo-day crash, we get a nice error message
from the compiler, along the lines of

Line xxx: Incompatible types in assignment

and we add the missing header, and everything is fine!

So *that's* why most people here will tell you not to
cast 'malloc'.

Tak-Shing Chan came up with an example or two of places
where it's actually more foolproof to cast malloc than
not, involving 'extern' declarations. Look them up on
Google Groups if you're interested. But the main point
stands: Don't cast malloc, and don't cast *anything*
unless you can abundantly justify the cast. Because
casts are blunt instruments, capable of doing great
damage when used indiscriminately.

HTH,
-Arthur

 

[Mark A. Odell]

strncat(president->name, "George Washington", PRES_NAME_LEN - 1);

Question on style here. How does PRES_NAME_LEN improve readability versus
the orignal

struct pres {
char name[25];
struct pres *next;
};

if we use sizeof president->name instead of PRES_NAME_LEN? I see the
manifest constant as extra work without real additional clarity.

 

[Martin Dickopp]

strncat(president->name, "George Washington", PRES_NAME_LEN - 1);

Question on style here. How does PRES_NAME_LEN improve readability versus
the orignal

struct pres {
char name[25];
struct pres *next;
};

if we use sizeof president->name instead of PRES_NAME_LEN? I see the
manifest constant as extra work without real additional clarity.

Although I'm not the person who suggested `PRES_NAME_LEN' originally in
this thread, I still like the idea. The reason is that I prefer to #define
/all/ magic numbers in one place per file, so if this were my code, I
would have used the macro in the structure declaration, but still referred
to the size of the member as `sizeof president->name' later.

Martin

 

[Steve Zimmerman]

On Wed, 03 Sep 2003 14:23:55 GMT,



Who taught you to cast malloc()'s return value? Others have pointed out
the reasons why this is bad; I just want to know where you got the idea
from.

--Daniel

The honest answer is, "No one taught me to cast malloc()'s return value.
I've never written any code using malloc." See

http://www.c-for-dummies.com/lessons/chapter.17

Thank you for your post.

--Steve