problems with string struct members

[flipjoebanana]

Hello,
I am having problem with strings within structs and was wondering if someone could help.

I have a struct, i.e.

#include <stdio.h>
#include <strings.h>

struct feeling
{
    char *emotion;
    char *emotion_options[20];
    int num;
};

which i then pass to a function via reference, i.e.

int main()
{
    struct feeling ex_1 {"happy",{"yes","no","maybe"},-1};
    function(&ex_1);

    return 0;
}

and in the function I use string functions on its members and they aren't being preserved/accessed correctly?

void function(struct emotion *test) 
{
    int size;
    printf("%s", test->emotion); /*prints out happy */
    size = strlen(test->emotion); 
    printf("%s",test->emotion); /* prints out random characters */
}

I was wondering why the 'emotion' member would be modified after the strlen call, and what can be done to prevent it? Or really just how I should go about doing this. Tried different string function calls like strcat, strcmp, and have same problem.

[Dave Sinkula]

I'm not sure what you're seeing.

#include <stdio.h>
#include <strings.h>

struct feeling
{
    char *emotion;
    char *emotion_options[20];
    int num;
};

void function(struct feeling *test) 
{
    int size;
    printf("%s", test->emotion); /*prints out happy */
    size = strlen(test->emotion); 
    printf("%s",test->emotion); /* prints out random characters */
}

int main()
{
    struct feeling ex_1 = {"happy",{"yes","no","maybe"},-1};
    function(&ex_1);

    return 0;
} 

/* my output
happyhappy
*/

[flipjoebanana]

Thanks for replying. I wanted to make sure I was understanding that last part right. I also need to parse a command line input and store the items into the struct, I think this is the part where I'm doing something wrong. Updated code:

#include <stdio.h>
#include <strings.h>

typedef struct
{
    char *emotion;
    char *emotion_options[20];
    int num;
}FEELING;

void function(FEELING *test)
{
    int size;
    printf("%s\n", test->emotion); 
    size = strlen(test->emotion);
    printf("%s\n",test->emotion); 
}

void process_input(FEELING *ex_1)
{
    char line[200];
    char *pstr;
    int count=0;

    fgets(line,201,stdin);

    ex_1->num = -1;

    pstr = strtok(line," \n\r");
    while(pstr != NULL)
    {
        if(ex_1->num == -1)
            ex_1->emotion = pstr;
        else
            ex_1->emotion_options[ex_1->num] = pstr;

        pstr = strtok(NULL," \n\r");
    }
}

int main()
{
    FEELING ex_1;

    /* get emotional input */
    process_input(&ex_1);
    function(&ex_1);

    return 0;
}

Any thoughts why this doesn't work?

[Dave Sinkula]

You don't allocate memory into which to read the strings.

[Aia]

Regardless of any other logic flaws, I would like to point to some of them.

void process_input(FEELING *ex_1)
{
    char line[200];
    char *pstr;
    int count=0;

    fgets(line,201,stdin); /* line can only hold a buffer of 200, there's opportunity for overflow because of that 201 */

    ex_1->num = -1;

    pstr = strtok(line," \n\r");
    while(pstr != NULL)
    {
        if(ex_1->num == -1) /* never has any opportunity of be something else */
            ex_1->emotion = pstr; /* Keeps overwriting the variable to pointing to different tokens every time through the loop */
        else
            ex_1->emotion_options[ex_1->num] = pstr; /* never happens */

        pstr = strtok(NULL," \n\r");
    }
}

[flipjoebanana]

Thanks for the help guys! Here's final:

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

#define MAX_EMOTION_SIZE 50

typedef struct
{
    char *emotion;
    char **emotion_options;
    int num;
}FEELING;

void function(FEELING *test)
{
    int size, count;
    size = strlen(test->emotion);

    /* prints out emotion */
    printf("%s\n",test->emotion);

    /* print out emotion options */
    for(count = 0; count < test->num; count++)
        printf("%s\n", test->emotion_options[count]);
}


void process_input(FEELING *ex_1)
{
    char line[200];
    char *pstr;

    printf("Enter emotion with options: ");
    fgets(line,200,stdin);

    pstr = strtok(line," \n\r");
    while(pstr != NULL)
    {
        if(ex_1->num == -1)
            strcpy(ex_1->emotion,pstr);
        else
        {
            ex_1->emotion_options[ex_1->num] = (char *)malloc(MAX_EMOTION_SIZE);
            strcpy(ex_1->emotion_options[ex_1->num],pstr);
        }

        (ex_1->num)++;
        pstr = strtok(NULL," \n\r");
    }
}

int main()
{
    FEELING ex_1;
    ex_1.emotion_options = (char **)malloc(sizeof(char *[MAX_EMOTION_SIZE]));
    ex_1.emotion = (char *)malloc(MAX_EMOTION_SIZE);
    ex_1.num = -1;


    /* get emotional input */
    process_input(&ex_1);
    function(&ex_1);

    int i;
    if(ex_1.num > 0)
        for(i=0; i < (ex_1.num); i++)
                free(ex_1.emotion_options[i]);

    free(ex_1.emotion);
    free(ex_1.emotion_options);
    return 0;
}

[dwks]

ex_1.emotion_options = (char **)malloc(sizeof(char *[MAX_EMOTION_SIZE]));

That's uninitialized, so if you don't fill in MAX_EMOTION_SIZE elements of that array, then this will be freeing random values:

for(i=0; i < (ex_1.num); i++)
                free(ex_1.emotion_options[i]);

The solution is to memset() the memory from the original call to zero, or easier yet, use calloc() instead of malloc(). (calloc() is meant to be used for arrays anyway, and this is an array.)

Your process_input() function could use sscanf() if you wanted -- but the way you have it is fine too (and probably what I would have done anyway).

(ex_1->num)++;

Those parentheses aren't required, but they're fine, of course. What isn't fine is that you don't check if you're incrementing ex_1->name above or equal to MAX_EMOTION_SIZE, so you have a possible buffer overrun here.

In main():

int i;
    if(ex_1.num > 0)
        for(i=0; i < (ex_1.num); i++)
                free(ex_1.emotion_options[i]);

Declaring variables in the middle of a block like that is C99. If you want C89-compatible code (which I suspect), put the variable declaration at the beginning of the function -- or perhaps refactor the freeing code into a separate function.

If you don't mind C99-specific code, you might as well declare the variable in the for loop.

for(int i=0; i < (ex_1.num); i++)

Casting malloc() isn't always a good idea, because it can hide changes to the type of the variable and so on.

[flipjoebanana]

Thanks for the comments dwks. Yes I do desire c89 (after I just figured out what that meant). I made all the fixes. I have a question about the freeing random values and was wondering why that needed to be corrected.

Does that mean that a pointer upon creation randomly points to some address initially and potentially toward memory used by another program? Calloc/memset address this issue by setting the pointer values to null?

If so, then why is the default for the pointer to point to some random address and not null upon declaration?

[dwks]

Thanks for the comments dwks. Yes I do desire c89 (after I just figured out what that meant).

Yeah, most of the time it's a good idea. Sorry I didn't explain it further.

Does that mean that a pointer upon creation randomly points to some address initially and potentially toward memory used by another program?

Yes. Accessing such memory could lead to data corruption or at the very least segmentation faults.

Calloc/memset address this issue by setting the pointer values to null?

Correct.

If so, then why is the default for the pointer to point to some random address and not null upon declaration?

Why? Because initializing things to zero takes time. C is an efficient language. It does as little as possible. C figures that if you need the memory initialized to zero, you will use calloc or memset. :)

[edit] About casting malloc():

In the old days of K&C C (before C89!), there was no such thing as void *. So malloc() had to return char * instead. Which meant that you had to cast the return value of malloc() to whatever you were using, say, int *.

Now, however, void * does exist, and that is what malloc() returns. C (even C89) says that you can assign a void pointer to any other pointer without a cast. Hence, casting malloc() is no longer required. And in fact, it's possible to make an error in a cast, so you might as well leave out the cast.

Casting malloc() can also hide the fact that stdlib.h is not included. Without a cast, the compiler will use the implicit int rule and assume that malloc() returns int. Without a cast, an assignment from int to, say, int * will cause an error or warning; casting the value hides this warning. You can get some really nasty errors this way. (malloc(10) is passing an int for the parameter, but malloc() takes a size_t, which may not be the same size as an int . . . .)

This issue is compounded by the fact that in C++, you do need a cast. But in C++, you're better off using new and delete anyway.

You're best off not casting malloc() at all. :) [/edit]

[edit=2] Another thing:

Hard-coding types like this

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

is doable, but it makes more sense to use the variable itself:

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

That way the code still works when you change the type of p. sizeof is a compile-time value in this case, so don't worry, it's just as efficient. [/edit]

[edit=3] In other words, instead of this

ex_1.emotion_options = (char **)malloc(sizeof(char *[MAX_EMOTION_SIZE]));

I'd use this:

ex_1.emotion_options = malloc(sizeof(*ex_1.emotion_options) * MAX_EMOTION_SIZE);

[/edit]