Allocating Memory And Strings

Discussion in 'C Programming' started by Mike, Feb 28, 2009.

  1. Mike

    Mike Guest

    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
     
    Mike, Feb 28, 2009
    #1
    1. Advertising

  2. Mike <> wrote:
    > 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
    <stdlib.h>, it might keep the compiler from warning you about
    that.

    > /*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
    --
    \ Jens Thoms Toerring ___
    \__________________________ http://toerring.de
     
    Jens Thoms Toerring, Feb 28, 2009
    #2
    1. Advertising

  3. Mike

    Eric Sosman Guest

    Mike wrote:
    > 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().

    > }
    > return 0;
    >
    > }



    --
    Eric Sosman
    lid
     
    Eric Sosman, Feb 28, 2009
    #3
  4. Mike

    Flash Gordon Guest

    Mike wrote:
    > 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.

    <snip>

    > 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
     
    Flash Gordon, Feb 28, 2009
    #4
  5. Mike

    Flash Gordon Guest

    Eric Sosman wrote:
    > Mike wrote:


    <snip>

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


    <snip>

    >> /*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.
    --
    Flash Gordon
     
    Flash Gordon, Feb 28, 2009
    #5
  6. Mike

    Mike Guest

    "Flash Gordon" <> wrote in message
    news:-gordon.me.uk...
    > Mike wrote:
    >> 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.
    >
    > <snip>
    >
    >> 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


    Thanks alot for the replies and links,

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

    Mike
     
    Mike, Feb 28, 2009
    #6
  7. Mike

    Guest

    On Feb 28, 2:01 pm, "Mike" <> wrote:
    > 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
     
    , Feb 28, 2009
    #7
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Joe Estock
    Replies:
    7
    Views:
    381
    Karl Heinz Buchegger
    Nov 18, 2003
  2. Yodai
    Replies:
    1
    Views:
    296
    Yodai
    Aug 28, 2003
  3. Ben

    Strings, Strings and Damned Strings

    Ben, Jun 22, 2006, in forum: C Programming
    Replies:
    14
    Views:
    814
    Malcolm
    Jun 24, 2006
  4. Win Sock

    Allocating memory for strings

    Win Sock, Oct 6, 2007, in forum: C Programming
    Replies:
    20
    Views:
    829
    David Thompson
    Oct 22, 2007
  5. Rakesh Kumar
    Replies:
    5
    Views:
    695
    James Kanze
    Dec 21, 2007
Loading...

Share This Page