Allocating Memory And Strings

[Mike]

Hi,

Can someone tell me where I'm going wrong please?

I'm trying to read 6 strings, allocate memory for each one and then print
them to screen before finally freeing the memory.

This is what I'm working with at the moment:

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

int main(void)
{

/*Array for holding pointers to the strings */
char *str_array[6];
/*Variable to keep track of which string we're working on*/
int string;
/*Temp string to hold value after it has been read */
char tempstr[40];

for (string=0; string<6; string++){
/*Read string */
scanf("%s", tempstr);
/*Allocate memory for the string, save address in str_array */
str_array[string]=(char*)(malloc(strlen(tempstr)*sizeof(char)));
/*Copy read string to newly created memory*/
*str_array[string] = tempstr;
}

for (string=0; string<6; string++){
/*Print the string*/
printf("%s\n", str_array[string]);
/*String has finished so free memory at that location*/
free(str_array[string]);
}
return 0;

}

At the moment it's currently displaying a random set of characters however
from stepping through the code I've noticed that the strings are being
stored in the str_array and not the actual space I'm allocating for them.
I'm presuming that the error is line:

*str_array[string] = tempstr;

However that to me looks right....

Any help appreciated,

Cheers,

Mike

 

[Jens Thoms Toerring]

Hi,

Can someone tell me where I'm going wrong please?

I'm trying to read 6 strings, allocate memory for each one and then print
them to screen before finally freeing the memory.

This is what I'm working with at the moment:

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

int main(void)
{

/*Array for holding pointers to the strings */
char *str_array[6];
/*Variable to keep track of which string we're working on*/
int string;
/*Temp string to hold value after it has been read */
char tempstr[40];

for (string=0; string<6; string++){
/*Read string */
scanf("%s", tempstr);

I hope you are aware that this is unsafe. 'tempstr' has only room
for 39 chars (plus the trailing '\0') but the user can easily enter
a longer string, which the results in a write past the end of
'tempstr'. To void that use

scanf("%39s", tempstr);

/*Allocate memory for the string, save address in str_array */
str_array[string]=(char*)(malloc(strlen(tempstr)*sizeof(char)));

The bad problem is that you allocate one char less that is
needed. strlen() returns the number of chars before the
trailing '\0' and you need one more char for that.

It is usually recommended here not o cast the return value
of malloc(). Unless you want to compile the program with a
C++ compiler it's not needed and, if you forgot to include

/*Copy read string to newly created memory*/
*str_array[string] = tempstr;

That's not a string copy. It sets the first element of the array
'str_array[string]' points to to the address of the first element
of the array 'tempstr' (after converting that address to a char,
resulting in a non-sense value). '*str_array[string] could also
be written as 'str_array[string][0]', which hopefully makes it
clearer what you're really doing here.

To copy strings use the strcpy() function

strcpy( str_array[string], tempstr )

Regards, Jens

 

[Eric Sosman]

Hi,

Can someone tell me where I'm going wrong please?

Several places; see remarks in-line.

I'm trying to read 6 strings, allocate memory for each one and then
print them to screen before finally freeing the memory.

This is what I'm working with at the moment:

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

int main(void)
{

/*Array for holding pointers to the strings */
char *str_array[6];
/*Variable to keep track of which string we're working on*/
int string;
/*Temp string to hold value after it has been read */
char tempstr[40];

for (string=0; string<6; string++){
/*Read string */
scanf("%s", tempstr);

This is as dangerous as gets(), because if the input
string is 40 characters or longer it will not fit in the
tempstr[] array. See Questions 12.20 and 12.23 in the
comp.lang.c Frequently Asked Questions (FAQ) list at
<http://www.c-faq.com/>. Also, every call to a function
that can succeed or fail should be accompanied by a test
to see which happened. scanf() can fail (for example, if
it encounters end-of-file).

/*Allocate memory for the string, save address in str_array */
str_array[string]=(char*)(malloc(strlen(tempstr)*sizeof(char)));

You're allocating less space than the string needs,
because you've forgotten about the trailing `\0' character.
Also, see Question 7.8 in the FAQ. Also, see the remarks
above about testing for success or failure; malloc() can
fail.

/*Copy read string to newly created memory*/
*str_array[string] = tempstr;

This "copies" no string at all. Rather, it causes the
pointer variable str_array[string] to point at the first
character in the array tempstr[]. That's bad because it
discards the only record you have of the memory malloc()
allocated; this is known as a "memory leak." Also, the
contents of tempstr[] will be overwritten next time around
the loop, meaning that that the "identity" of what is pointed
at will change. Read up on the strcpy() function.

}

for (string=0; string<6; string++){
/*Print the string*/
printf("%s\n", str_array[string]);
/*String has finished so free memory at that location*/
free(str_array[string]);

This would be right if you hadn't clobbered str_array[string]
already. But as things stand, you are trying to free() the
tempstr[] array itself, which is a no-no because it was not
obtained from malloc().

 

[Flash Gordon]

Hi,

Can someone tell me where I'm going wrong please?

I'm trying to read 6 strings, allocate memory for each one and then
print them to screen before finally freeing the memory.

This is what I'm working with at the moment:

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

int main(void)
{

/*Array for holding pointers to the strings */
char *str_array[6];
/*Variable to keep track of which string we're working on*/
int string;
/*Temp string to hold value after it has been read */
char tempstr[40];

This is large enough for a 39 character string + the null termination.

for (string=0; string<6; string++){
/*Read string */
scanf("%s", tempstr);

What happens if the user types in a 40 character or longer string? What
if there is a space in the string the user types? What if there is an
error since you do not check the value returned by scanf? You would
almost certainly be better off using fgets, but if you use scanf you
need to specify the maximum length and check the value it returns.

/*Allocate memory for the string, save address in str_array */
str_array[string]=(char*)(malloc(strlen(tempstr)*sizeof(char)));

sizeof(char) is 1 be *definition* and the cast is not (and in this
instance never has been) required. That gets us down to the more readable:
str_array[string]=malloc(strlen(tempstr));

Now it is easy to see that you have not allowed space for the null
termination of your string. So what you actually need is:
str_array[string] = malloc(strlen(tempstr) + 1);

The extra space is just to make it easier to read!

You should also check to see if malloc succeeded.

/*Copy read string to newly created memory*/
*str_array[string] = tempstr;

<snip>

This does not copy a string. Your compiler should have told you it was
wrong. To copy strings you (normally) use strcpy.

At the moment it's currently displaying a random set of characters
however from stepping through the code I've noticed that the strings are
being stored in the str_array and not the actual space I'm allocating
for them. I'm presuming that the error is line:

*str_array[string] = tempstr;

However that to me looks right....

Your compiler will not have thought it looked right. You need to re-read
your C text book, the parts where it tells you about strings. You may
also find useful information in the C faq at http://c-faq.com/

 

[Flash Gordon]

Mike wrote:

/*Array for holding pointers to the strings */
char *str_array[6];

/*Copy read string to newly created memory*/
*str_array[string] = tempstr;

This "copies" no string at all. Rather, it causes the
pointer variable str_array[string] to point at the first
character in the array tempstr[]. That's bad because it

<snip>

Read it again. It's bad Jim, but not as you know it...
He is attempting to assign a pointer to a char! Required diagnostic.

 

[Mike]

Hi,

Can someone tell me where I'm going wrong please?

I'm trying to read 6 strings, allocate memory for each one and then print
them to screen before finally freeing the memory.

This is what I'm working with at the moment:

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

int main(void)
{

/*Array for holding pointers to the strings */
char *str_array[6];
/*Variable to keep track of which string we're working on*/
int string;
/*Temp string to hold value after it has been read */
char tempstr[40];

This is large enough for a 39 character string + the null termination.

for (string=0; string<6; string++){
/*Read string */
scanf("%s", tempstr);

What happens if the user types in a 40 character or longer string? What if
there is a space in the string the user types? What if there is an error
since you do not check the value returned by scanf? You would almost
certainly be better off using fgets, but if you use scanf you need to
specify the maximum length and check the value it returns.

/*Allocate memory for the string, save address in str_array */
str_array[string]=(char*)(malloc(strlen(tempstr)*sizeof(char)));

sizeof(char) is 1 be *definition* and the cast is not (and in this
instance never has been) required. That gets us down to the more readable:
str_array[string]=malloc(strlen(tempstr));

Now it is easy to see that you have not allowed space for the null
termination of your string. So what you actually need is:
str_array[string] = malloc(strlen(tempstr) + 1);

The extra space is just to make it easier to read!

You should also check to see if malloc succeeded.

/*Copy read string to newly created memory*/
*str_array[string] = tempstr;

<snip>

This does not copy a string. Your compiler should have told you it was
wrong. To copy strings you (normally) use strcpy.

At the moment it's currently displaying a random set of characters
however from stepping through the code I've noticed that the strings are
being stored in the str_array and not the actual space I'm allocating for
them. I'm presuming that the error is line:

*str_array[string] = tempstr;

However that to me looks right....

Your compiler will not have thought it looked right. You need to re-read
your C text book, the parts where it tells you about strings. You may also
find useful information in the C faq at http://c-faq.com/

Thanks alot for the replies and links,

I've realised my biggest mistake was approaching this still with my VB head
on.

Mike

 

[jfbode1029]

Hi,

Can someone tell me where I'm going wrong please?

I'm trying to read 6 strings, allocate memory for each one and then print
them to screen before finally freeing the memory.

This is what I'm working with at the moment:

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

int main(void)
{

 /*Array for holding pointers to the strings */
 char *str_array[6];
 /*Variable to keep track of which string we're working on*/
 int string;
 /*Temp string to hold value after it has been read */
 char tempstr[40];

 for (string=0; string<6; string++){
  /*Read string */
  scanf("%s", tempstr);
  /*Allocate memory for the string, save address in str_array */
  str_array[string]=(char*)(malloc(strlen(tempstr)*sizeof(char)));

By definition, sizeof(char) == 1, so it's not necessary to use it in
this case. Also, remember that strlen() gives you the number of
character before the terminating 0 character; you'll have to add 1 to
properly size the array. Finally, you don't need to cast the result
of malloc() (unless you're working in C++ or you're working with a
compiler that predates the 1989 standard). Rewrite that as

str_array[string] = malloc(strlen(tempstr) + 1);

You will also want to add a check to make sure malloc() returned a
valid (i.e., != NULL) pointer.

  /*Copy read string to newly created memory*/
  *str_array[string] = tempstr;

Here's you're problem; you cannot copy the string contents using the
"=" operator. Here's what you need to do:

strcpy(str_array[string], tempstr);

 for (string=0; string<6; string++){
  /*Print the string*/
  printf("%s\n", str_array[string]);
  /*String has finished so free memory at that location*/
  free(str_array[string]);
 }
 return 0;

}

At the moment it's currently displaying a random set of characters however
from stepping through the code I've noticed that the strings are being
stored in the str_array and not the actual space I'm allocating for them.
I'm presuming that the error is line:

  *str_array[string] = tempstr;

However that to me looks right....

Any help appreciated,

Cheers,

Mike