Function changes data on other variables

Question

i have structure Trip and i make linked list for this Trip, when i add trip to the list i dont have problem, but when add the second trip it overrides the first one. The input I do is adding two trips with id 1 and 2 and when i iterate them i see there is two times id 2

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

char date[10];
char date2[10];
char newdate[10];
char newdate2[10];
int i,t,i2,t2;
int ret;

int option;



typedef struct trip
{
    int code;
    char startdate[10];
    int duration;
    double price;
} Trip;

typedef struct list
{
    Trip* Trip;
    struct list* next;
} List;


List* create_List(Trip* trip)
{
    List* newList = malloc(sizeof(List));
    if (NULL != newList)
    {
        newList->Trip = trip;
        newList->next = NULL;
    }
    return newList;
}

void delete_List(List* oldList)
{
    if (NULL != oldList->next)
    {
        delete_List(oldList->next);
    }
    free(oldList);
}

List* add_List(List* wordList, Trip* trip)
{
    List* newList = create_List(trip);
    if (NULL != newList)
    {
        newList->next = wordList;
    }
    return newList;
}

void createTripData(Trip *trip);

int main(int argc, char *argv[])
{

    List* trips;
    int first = 1;

    while(1)
    {
        printf("Select an option: \n 1-add new trip \n 2-Iterate\n");
        scanf("%d", &option);

        if (option == 1)
        {
            Trip newTrip;

            createTripData(&newTrip);

            if(first == 1)
            {
                first = -1;
                trips = create_List(&newTrip);
            }
            else
            {
                trips = add_List(trips, &newTrip);
            }
        }

        else if (option == 2)
        {
            system("@cls||clear");
            List* iter;
            for (iter = trips; NULL != iter; iter = iter->next)
            {
                printf("Id %d \n", iter->Trip->code);
            }
        }


    }
    return (0);
}

void createTripData(Trip *trip)
{
            fflush(stdin);
            printf("Enter id: ");
            scanf("%d", &trip->code);//
}

`fflush(stdin)` is undefined behavior on all but a very few implementations and is non-portable. Whenever a "Function changes data of other variables", that generally means you are writing beyond the bounds of a block of memory, happily smashing the memory of neighboring variables. Double-check your use of pointers and the dereferences you make. Avoid the use of *global variables* unless absolutely required -- none are required here. — David C. Rankin, May 11 '17 at 14:55
Scope of `Trip newTrip;` is inside this if-block. Moreover, You use it repeatedly. — BLUEPIXY, May 11 '17 at 14:56
newTrip is just middleman who then add the trip into List* trips — Hristo Vutov, May 11 '17 at 15:00
But it is limited in scope to within the `if` block`. Once you exit the `if` block, its memory address (which you have stored in `trips`) is released as "available for reuse" by the system (which of course is happening every iteration). So you likely end up storing the all values input at the same address and assign that address within `trips` which leaves you attempting to access a memory address that has gone out of scope outside the `if` statement and right off the cliff into *Undefined Behavior*. — David C. Rankin, May 11 '17 at 15:02
Additionally, you do not need `option` at all. Just initialize `List *trips = NULL;` (**note:** the `'*'` goes with the variable, not the `type`). Then all you need is a check `if (trips == NULL) {..create 1st node..} else {..add node to end..}` — David C. Rankin, May 11 '17 at 15:12

Support Ukraine · Answer 1 · 2017-05-11T15:29:54.140

1

A problem with your code is the newTrip variable. You can not add it to the list the way you are trying. It is a local variable in a part of main. Therefore it goes out of scope (i.e. becomes invalid) when you exit that part of main. I'll suggest that you rewrite createTripData like:

Trip *trip createTripData()
{
    Trip* t = malloc(sizeof(*t));
    if (!t)
    {
        // Ups... add error handling
        exit(1);
    }

    printf("Enter id: ");
    if (scanf("%d", &t->code) != 1)
    {
        // Ups... add error handling
        exit(1);
    }

    return t;
}

and call it like:

    if (option == 1)
    {
        Trip* newTrip = createTripData();

        if(first == 1)
        {
            first = -1;
            trips = create_List(newTrip);
        }
        else
        {
            trips = add_List(trips, newTrip);
        }
    }

Besides that you can simplify your code by handling the empty list case inside add_List.

List* trips = NULL;     // Notice this initialization

while(1)
{
    printf("Select an option: \n 1-add new trip \n 2-Iterate\n");
    scanf("%d", &option);

    if (option == 1)
    {
        Trip* newTrip = createTripData();
        trips = add_List(trips, &newTrip);
    }

edited May 11 '17 at 15:29

answered May 11 '17 at 15:24

Support Ukraine

42,271
4
38
63

Thank you a lot,earlier i saw even when i scanf the new id changes the one in the list even when i dont use add_List – Hristo Vutov May 11 '17 at 15:30
@HristoVutov - that was because you used the local variable. This answer allocates a new `Trip` each time and therefore it doesn't happen. – Support Ukraine May 11 '17 at 15:35
I know this is copy, but it is good practice to show `List *trips = NULL;` Especially when helping new C programmers, because in `int* a, b, c;` (`b` and `c` are certainly not pointers). Attaching the `'*'` to the `type` lends itself to confusion, `int *a, b, c;` is much less susceptible to misinterpretation. – David C. Rankin May 12 '17 at 00:18
@DavidC.Rankin - I just copied from OPs question. I didn't pay any attention to it. I don't think you'll find a common consensus. http://stackoverflow.com/a/2660648/4386427 – Support Ukraine May 12 '17 at 06:40
@DavidC.Rankin IMO `List* trips = NULL;` is fine, and you should avoid writing `int * a, b, c;` regardless of positioning of the star – M.M May 13 '17 at 02:27
Differing opinions on the subject are fine. The `int *a, b, c;` is largely for example purposes, although I see no problem with it if for clarity if a `'\n'` follows each `comma`. Agreed, the compiler doesn't care either way, the issue, to me, is clarity and readability of what is declared as a pointer. – David C. Rankin May 13 '17 at 02:51

user3629249 · Answer 2 · 2017-05-12T23:44:59.100

the function: createTripData() calls scanf() but fails to check the returned value (not the parameter values) so if the user enters just a return key or enters anything but digit(s) then the code accepts the nothing input, This is an error

The function: main() has two parameters, but neither is being used. the function main() has two valid signatures:

int main( int argc, char *argv[] )
int main( void )

since the parameters are not used, the code should use the signature:

int main( void )

The function: main() is getting the menu selection from the user via:

scanf( "%d", &option );

but not checking the returned value (not the parameter values) so when the user only enters a 'return' or any character other than a digit, the prior value in the field option is used. if the user entered any digit besides 1 or 2 then the code 'screams' through the while() loop, doing either nothing or per what ever value was previously in the variable option.

I.E. ALWAYS check the returned value from calls to any of the scanf() family of functions and handle errors.

in the function: main(), a assumption is made that the user entered either 1 or 2. That is NOT a valid assumption. (never trust the user to do the right thing) Strongly suggest the if( option == 1 and else if(option == 2) be replaced with a switch() statement that includes adefault` case for when the user enters any number other than 1 or 2 I.E.

switch( option )
{
    case 1:
        ...
        break;

    case 2:
        ...
        break;

    default:
        printf( "INVALID option: %d, valid options are 1, 2\n", option );
        break;
}

this field, in the definition of struct list

Trip* Trip;

It is very poor programming practice to name a variable the same as the type of the variable. A possible fix:

Trip *myTrip;

Then update the other references in the code to use myTrip

It is very poor programming practice to name a variable the same as the type of the variable, with the only difference being the capitalization. Such practice leads to confusion in the human reading the code.

In function: create_list() what happens when the call to malloc() fails? A NULL pointer is returned to function: add_List() which returns that NULL pointer to function: main(), which (successful or not) overlays the value in the variable trips.

the statement:

fflush(stdin);

while being allowed in visual studio, is NOT portable. suggest using something like:

int ch;
while( (ch = getchar()) != EOF && '\n' != ch );

regarding the calls to scanf(), here is one way to handle the error events:

if( 1 != scanf( "%d", &trip->code ) )
{ // then error occurred as 'scanf' returns number of successful input/conversions
    perror( "scanf for code failed" );
    exit( EXIT_FAILURE );
}

// implied else, scanf successful

The above suggested method immediately exits the program (leaving the OS to cleanup all the allocated memory, which would be poor programming practice) You might want to, instead code a loop that informs the user of the problem, empties stdin and allowing the user to try again.

this statement:

printf("Select an option: \n 1-add new trip \n 2-Iterate\n");

will become very 'difficult' to read and understand if there are several choices in the menu. suggest making use of the C feature of bring all the consecutive strings together and writing the statement like this:

printf("Select an option: \n"
       " 1-add new trip \n"
       " 2-Iterate\n");

Note: the suggested formatting also pays attention to the right edge of the printed page, making for a much nicer layout.

the function: delete_List() is never called, so the 'menu' in main() is missing an option.

BTW: the function: delete_List() uses recursion, which is OK if the linked list is rather short. However, if there are a lot of entries in the linked list then (especially on windows) there is a high probability of overflowing the stack. I suggest using a loop to walk the linked list, passing each entry to free() after saving the ->next pointer Then it does not matter how many entries are in the linked list

for ease of readability and understanding: 1. separate code blocks (for, if, else, while, do... while, switch, case, default) via a single blank line 2. separate functions by 2 or 3 blank lines (be consistent) 3. follow the axiom: only one statement per line and (at most) one variable declaration per statement.

Now, why are you seeing the problem you mention in the question:

the second pointer passed to add_List() is to a uninitialized local pointer on the stack in main(), not to the actual pointer to the linked list that was declared by: List* trips;

Suggest using unique names so YOU can keep track of what is pointing to what.

Also,in function: create_List(), the new node is being set to point to that uninitialized pointer in main(), rather than to the desired entry in trips in main.

Note, in function: main(), in modern C compilers, if the returned value is always 0, then the return statement is not needed.

Note: the statement return is not a function, so there is no need (unless wrapping multiple expression into a single result value) to have parens around the value passed back by return.

for simplification, suggest elimination of the variable first, changing List* trips; to List *trips = NULL; and replacing: if(first == 1) with: if( !trips )

I.E. always try to write the simplest code that still performs the functionality.

Function changes data on other variables

2 Answers2