Dynamic allocation of a string in a linked list problem

Question

I have created 2 functions that read some data from a file and write the data in another file, but using linked lists and dynamically allocated strings in that list, but I have a logic error that I can't find:

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

struct product {
    char id[6];
    char *name;
    int price;
    struct product *next;
};

struct product *read(struct product *head, FILE *input) {
    struct product *p, *q, *aux;
    p = (struct product *)malloc(sizeof(struct product));
    char aux_id[6];
    char aux_name[20];
    int aux_price;
    fscanf(input, "%s %s %d", aux_id, aux_name, &aux_price);
    strcpy(p->id, aux_id);
    p->name = (char *)malloc(strlen(aux_name) * (sizeof(char)));
    strcpy(p->name,aux_name);
    p->price = aux_price;
    p->next = NULL;
    head = p;

    while (fscanf(input, "%s %s %d", aux_id, aux_name, &aux_price) != EOF) {
        q = (struct product *)malloc(sizeof(struct product));
        q->name = (char *)malloc(strlen(aux_name) * (sizeof(char)));
        q->next = NULL;
        strcpy(q->name, aux_name);
        strcpy(q->id, aux_id);
        q->price = aux_price;
        p->next = q;
        p = q;
    }
    return head;
}

void write(struct product *head, FILE *output) {
    struct product *p;
    p = head;
    while (p != NULL) {
        fprintf(output, "%s %s %d\n", p->id, p->name, p->price);
        p = p->next;
    }
}

int main() {
    struct product *head, *p, *q;
    FILE *input = fopen("input.txt", "r+");
    FILE *output = fopen("output.txt", "w+");
    head = read(head, input);
    write(head, output);
    fclose(input);
    fclose(output);
}

input file looks like this:

333444 Cola 3
332312 Pepsi 4
123451 Mountain 3

output file looks like this

333444 Cola 3
332312°)q   4
123451à)q   3

Side note: malloc( strlen(...)) does not allocate memory for the terminating null character. Why don't you use strdup instead of malloc+strcpy? And why are you duplicating so much code in read? Last but not least: where is you error checking, especially for fscanf? — Werner Henze
– Werner Henze, Commented Mar 20, 2021 at 15:20
Is there a reason, why you can't change char *name to char name[20] in struct product? Then you wouldn't need to allocate memory for q->name manually. — Andy Sukowski-Bang
– Andy Sukowski-Bang, Commented Mar 20, 2021 at 15:25
@WernerHenze strdup is not a function in the C standard library (it's posix). — Paul Hankin
– Paul Hankin, Commented Mar 20, 2021 at 15:26
@PaulHankin Good point, thanks. But it looks like that is going to change with C23. — Werner Henze
– Werner Henze, Commented Mar 20, 2021 at 15:32

Kraego · Accepted Answer · 2021-03-20 15:22:34Z

2

if char aux_id[6]; has length 6 then "332312" is to big, you need space for the terminating symbol '\0' to work with string functions.

answered Mar 20, 2021 at 15:22

Kraego

4,0123 gold badges30 silver badges38 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

chqrlie · Accepted Answer · 2021-03-20 15:49:31Z

0

There are multiple problems in your code:

the arrays into which you read the strings are too small: 6 bytes is not enough to store a 6 digit id, you need space for the null terminator. Same problem in the structure definition.
since you do not provide the maximum number of bytes to store into these arrays, fscanf() stores the null terminator beynd the end of the aux_id array, causing undefined behavior which explains the output.
the argument head is unused in function read and passed uninitialized in main().
You do not allocate enough space for the null terminator. You should either use:
```
p->name = malloc(strlen(aux_name) + 1);
strcpy(p->name, aux_name);
```

or simply:

  p->name = strdup(aux_name);

Also note that naming your functions read and write might cause problems as these names are used in the C library for system call wrappers.
opening the files for update more is unnecessary: just use "r" and "w".
you do not check for fopen() or malloc() failures.

Here is a modified version:

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

struct product {
    char id[7];
    char *name;
    int price;
    struct product *next;
};

struct product *read(FILE *input) {
    struct product *head = NULL, *tail = NULL, *p;
    char buf[100];
    char aux_id[7];
    char aux_name[20];
    int aux_price;

    while (fgets(buf, sizeof buf, input)) {
        if (sscanf(buf, "%6s %19s %d", aux_id, aux_name, &aux_price) != 3) {
            fprintf(stderr, "line format error: %s", buf);
            continue;
        }
        p = (struct product *)malloc(sizeof(struct product));
        if (p == NULL) {
            fprintf(stderr, "memory allocation failure");
            break
        }
        strcpy(p->id, aux_id);
        p->name = strdup(aux_name);
        if (p->name == NULL) {
            fprintf(stderr, "memory allocation failure");
            free(p);
            break;
        }
        p->price = aux_price;
        p->next = NULL;
        if (head == NULL) {
            head = p;
        } else {
            tail->next = p;
        }
        tail = p;
    }
    return head;
}

void write(struct product *head, FILE *output) {
    struct product *p = head;
    while (p != NULL) {
        fprintf(output, "%s %s %d\n", p->id, p->name, p->price);
        p = p->next;
    }
}

int main() {
    struct product *head;
    FILE *input = fopen("input.txt", "r");
    if (input == NULL) {
        fprintf(stderr, "cannot open input file\n");
        return 1;
    }
    FILE *output = fopen("output.txt", "w");
    if (output == NULL) {
        fprintf(stderr, "cannot open output file\n");
        return 1;
    }
    head = read(head, input);
    write(head, output);
    fclose(input);
    fclose(output);
    return 0;
}

strdup() is a POSIX function that is available on most systems and will be included in the next version of the C Standard. If your system does not have it, you can write it this way:

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

char *strdup(const char *s) {
    size_t size = strlen(s) + 1;
    char *p = malloc(size);
    if (p == NULL)
        return NULL;
    return memcpy(p, s, size);
}

edited Mar 20, 2021 at 15:49

answered Mar 20, 2021 at 15:22

chqrlie

153k12 gold badges146 silver badges232 bronze badges

7 Comments

Werner Henze Over a year ago

All true, but it does not explain the seen output.

stefan_bobeica Over a year ago

I already tried to give +1 space, but it still does not work, I don't get why it works before while loop . I also tried strdup(aux_name) and it is the same problem...

Paul Hankin Over a year ago

strdup is posix, and not in the c stdlib.

chqrlie Over a year ago

@stefan_bobeica: there are more problems... I updated the answer.

dbush Over a year ago

@chqrlie It also specifies two's complement representation for integers, although strangely it still leaves out-of-range unsigned to signed conversion implementation defined.

|

dbush · Accepted Answer · 2021-03-20 15:31:47Z

0

You have two problems here, both of which result from not accounting for the terminating null byte for a string.

By not leaving enough space, you write past the bounds of the array / allocated memory where the string would be stored. This triggers undefined behavior which manifests as the garbled output you see in the output file.

The first is here (in two places):

malloc(strlen(aux_name)*(sizeof(char)));

This should be:

malloc(strlen(aux_name)*(sizeof(char)) + 1);

The second is in your struct and also in read:

char id[6];

It should be:

char id[7];

This latter case is the most likely reason why your output file looks as it does, since writing / reading past id in the struct could step on name.

Also, read and write are the name of library functions. You should rename them to something else that doesn't conflict with those. You should also free the allocated memory when you're done using it.

edited Mar 20, 2021 at 15:31

answered Mar 20, 2021 at 15:26

dbush

234k27 gold badges262 silver badges336 bronze badges

1 Comment

Paul Hankin Over a year ago

strlen(aux_name)*(sizeof(char)) + 1 only works because sizeof(char) is always 1. Which it is defined to be, but just strlen(aux_name) + 1 is simpler in that case.

Collectives™ on Stack Overflow

Dynamic allocation of a string in a linked list problem

3 Answers 3

Comments

7 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

7 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related