Why is the encoding messed up when strcpy'd in C

Question

Source Code:

char CUSTOMERS_FILE[50] = "customers.txt";

typedef struct Customer {
    char name[50];
    char password[50];
    char billing_address[100];
    char phone_number[15];
    double amount_paid;
    double amount_due;
    char date[20];
} Customer;

char* read_string(int length) {
    char data[length];
    rewind(stdin);
    fgets(data, length, stdin);

    if (data[0] == '\n') {
        data[0] = '\0';
    }

    strtok(data, "\n");

    printf("DATA: %s", data);

    return data;
}

void handle_modify_customer(Customer customer) {
    Customer edited_details;

    printf("\nMODIFYING DETAILS\n==============\n\n");

    printf("CREATE A CUSTOMER PROFILE\n=========================\n");

    printf("Name (%s): ", customer.name);
    strcpy(edited_details.name, read_string(50));

    printf("Password (%s): ", customer.password);
    strcpy(edited_details.password, read_string(50));

    printf("Billing Address (%s): ", customer.billing_address);
    strcpy(edited_details.billing_address, read_string(100));

    printf("Phone Number (%s): ", customer.phone_number);
    strcpy(edited_details.phone_number, read_string(15));

    printf("Amount Paid (%10.2lf): ", customer.amount_paid);
    scanf("%lf", &edited_details.amount_paid);

    printf("Amount Due (%10.2lf): ", customer.amount_due);
    scanf("%lf", &edited_details.amount_due);

    printf("Payment Date (%s): ", customer.date);
    strcpy(edited_details.date, read_string(20));

    /*
    if (strlen(edited_details.name) == '\0' || strlen(edited_details.billing_address) == '\0' || strlen(edited_details.password) == '\0' || strlen(edited_details.phone_number) == '\0' || strlen(edited_details.date) == '\0') {
        printf("All fields must be filled in!");
        handle_modify_customer(customer);
    }*/

    if (edited_details.name[0] == '\0' || edited_details.billing_address[0] == '\0' || edited_details.password[0] == '\0' || edited_details.phone_number[0] == '\0' || edited_details.date[0] == '\0') {
        printf("All fields must be filled in!");
        handle_modify_customer(customer);
    }

    FILE *file = fopen(CUSTOMERS_FILE, "r");
    FILE *new_file = fopen("customers_new.txt", "ab+");

    Customer record;

    while (fscanf(file, "[%[^]]], [%[^]]], [%[^]]], [%[^]]], [%lf], [%lf], [%[^]]]\n",
                  record.name, record.password, record.billing_address, record.phone_number,
                  &record.amount_paid, &record.amount_due, record.date) == 7)
    {
        if (strcmp(customer.name, record.name) == 0) {
            printf("P: %s\nD: %s", edited_details.phone_number, edited_details.date);
            fprintf(new_file, "[%s], [%s], [%s], [%s], [%lf], [%lf], [%s]\n", edited_details.name, edited_details.password, edited_details.billing_address, edited_details.phone_number, edited_details.amount_paid, edited_details.amount_due, edited_details.date);
        } else {
            fprintf(new_file, "[%s], [%s], [%s], [%s], [%lf], [%lf], [%s]\n", record.name, record.password, record.billing_address, record.phone_number, record.amount_paid, record.amount_due, record.date);
        }
    }

    fclose(file);
    fclose(new_file);

    remove(CUSTOMERS_FILE);
    rename("customers_new.txt", CUSTOMERS_FILE);

    printf("\nThe customer details have been successfully modified!\n");
    key_to_continue();
}

Execution Sample:

MODIFYING DETAILS
==============

CREATE A CUSTOMER PROFILE
=========================
Name (dumbfk): test
DATA: testPassword (abc123): lol
DATA: lolBilling Address (pukima jalan): lol
DATA: lolPhone Number (6969696969): 499449
DATA: 499449Amount Paid (   6969.00): 499449
Amount Due (6969699.00): 499494
Payment Date (6/9/1969): 22/2/2000
DATA: 22/2/2000P: �O���
D: �O���
The customer details have been successfully modified!

Data File (before):

[well lol], [abc123], [wtf bro? 24], [0183188383], [3000.000000], [4000.000000], [12/12/2012]
[chow hai], [abc123], [lol jalan], [6969696969], [6969.000000], [6969699.000000], [6/9/1969]
[lol head], [abc123], [lol jalan], [6969696969], [6969.000000], [6969699.000000], [6/9/1969]
[stupid face], [abc123], [lol jalan], [6969696969], [6969.000000], [6969699.000000], [6/9/1969]
[dumbfk], [abc123], [pukima jalan], [6969696969], [6969.000000], [6969699.000000], [6/9/1969]

Data File (after):

[well lol], [abc123], [wtf bro? 24], [0183188383], [3000.000000], [4000.000000], [12/12/2012]
[chow hai], [abc123], [lol jalan], [6969696969], [6969.000000], [6969699.000000], [6/9/1969]
[lol head], [abc123], [lol jalan], [6969696969], [6969.000000], [6969699.000000], [6/9/1969]
[stupid face], [abc123], [lol jalan], [6969696969], [6969.000000], [6969699.000000], [6/9/1969]
[test], [lol], [lol], [�O���], [499449.000000], [499494.000000], [�O���]

The Problem:

As you may see, the issue is that the Payment Date and Phone Number fields get messed up. It happens right after I use strcpy. I debugged the read_string(..) function and it seems to be fine. I don't understand why this is happening. Any help would be highly appreciated to solve this problem.

The funny part: Only date and phone_number are affected. name, password, billing_address do not have tis problem.

Answer 1

This is a very good example of where pointers and arrays are not the same thing:

char* read_string(int length) {
    char data[length];
    // code
    return data;
}

will not work, because data is a local array allocated on the stack and will cease to exist when the function returns.

If you change char data[length] to static char data[length] it will work. Do however note that the previous read will be overwritten, so this would not work as intended:

char *s1, *s2;
s1 = read_string(10);
s2 = read_string(10);
printf("First string: %s\n", s1);
printf("Second string: %s\n", s2);

A way to get around that is to use char data* = malloc(length * sizeof *data). That way you will be able to use previous reads. But in general you want to avoid hidden mallocs like this, since you need to free them afterwards. If you want to go for that solution, do like this:

char * read_string(char * dest, int length) {
    char data[length];
    // code
    return strncpy(dest, data, length);
}

And then call it like this:

char * str = malloc(length);
if(! read_string(str, length)) {
    fprintf(stderr, "Error reading string\n");
    exit(EXIT_FAILURE);
}
// Code
free(str);

Answer 2

There's no direct support for strings in C. Most of the time you have to deal with (shall I say "chunks of memory" instead of "arrays"?) chunks of memory containing characters and a terminating NUL in the end.

In your case it goes like this:

1. You read a string (a certain number of characters) from a file into a chunk of memory

2. You want that chunk of memory to remain valid up to the point where you make use of the string

3. You use your string. This is it.

Number 2 is what is broken in your code.

char* read_string(int length) {
    /* "data" is your chunk of memory. 
     * It is an array. 
     * This array is local to containing function.
     * This array is automatically allocated on the stack.
     * This array has its address in memory. Like &data[0].
     * The address may change from one invocation of the function to another.
     */
    char data[length];

    /* some code here... */

    return data; /* true, this returns the address &data[0] */
} 
/* The function is done. Local variables (array by the name of "data") are gone.
 * The memory on the stack that was once allocated for local variables is  
 * considered "free-to-use" by any other function you will invoke next. This memory
 * is not valid for use outside the function any more, but you may and you do
 * return a pointer to that memory, which is dangerous to use and harmful 
 */

There are basically three ways to fix this:

You either declare a global chunk of memory, say a global array:

char data[MAX_POSSIBLE_LENGTH];  /* Static storage. Global scope. */  
char* read_string() {
    /* some code here... */
    return data; 
} 

... or make the array static with a fixed length:

char* read_string() {
    static char data[MAX_POSSIBLE_LENGTH];  /* Static storage. Function scope. */  
    /* some code here... */
    return data; 
} 

... or pass the array in from the outside along with its length:

char* read_string(char *data, int length) {
    /* some code here... */
    return data; 
} 

... or use dynamic allocation

char* read_string(int length) {
    /* some code here... */
    char *data;
    data = malloc(length); /* Dynamic storage. Must be freed somewhere./* 
                           /* This is not an "array", now this really is a "chunk of memory" :) */

    return data; 
} 

void handle_modify_customer(Customer customer)
{
    Customer edited_details;
    char * input_str_ptr;

    printf("\nMODIFYING DETAILS\n==============\n\n");

    printf("CREATE A CUSTOMER PROFILE\n=========================\n");

    printf("Name (%s): ", customer.name);
    strcpy(edited_details.name, input_str_ptr = read_string(50));
    free(input_str_ptr); /* Don't allow mem leakage */

    /* ... */
}

Answer 3

Another option would to pass a pointer along with the length.

void read_string(char *data, int length) {

    fgets(data, length, stdin);

    if (data[0] == '\n') {
        data[0] = '\0';
    }

    strtok(data, "\n");

    printf("DATA: %s", data);

}

call with

 read_string(edited_details.name, 50);

then there is no need to use strcpy()

Answer 4

change the local char data in your read_string function to a static variable so that the memory is not lost

  #define MAX_STR_LENGTH 100

 char* read_string(int length) 
 {
    static char data[MAX_STR_LENGTH]; // change to static -- it will not change
    rewind(stdin);
    fgets(data, length, stdin);

    if (data[0] == '\n') {
        data[0] = '\0';
    }

    strtok(data, "\n");

    printf("DATA: %s", data);

    return data;
}