4
\$\begingroup\$

Hi, I asked this question over on stack overflow. However, they closed the thread and suggested to ask it on code review. So here I am :) The code below is an updated version of my original code, incorporating suggestions made on stack overflow.

I'm currently trying to improve my C skills and am practising with some tutorials. Right now I am trying to read a CSV file which contains names and phone numbers of party guests separated by ';'. Each line contains exactly one entry.

Although my code seems to work, I would like to hear some feedback regarding performance, security and elegance (possible memory leaks, misconceptions, possible improvements, runtime errors caused by unexpected file contents) to avoid making a habit out of bad code.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#define MAX_LINE_WIDTH 100

struct Guest {
    char *name;
    char *number;
};

void cleanUp(int, struct Guest *, char *, FILE *);

int main() {
    FILE *file = fopen("gaesteliste.txt", "r");
    if(file == NULL) {
        perror("Error while reading the file");
        exit(EXIT_FAILURE);
    }
    char *line = malloc(MAX_LINE_WIDTH * sizeof(char));
    if(line == NULL) {
        free(file);
        perror("Error while allocating required memory space");
        exit(EXIT_FAILURE);
    }
    struct Guest *guests = NULL;
    int guestCounter = 0;
    /*read a line: */
    while(fgets(line, MAX_LINE_WIDTH, file) != NULL) {
        /*allocate more space: */
        struct Guest *guestsNew = realloc(guests, (guestCounter+1)*sizeof(struct Guest));
        if(guestsNew == NULL) {
            free(guests);
            free(line);
            free(file);
            perror("Error while allocating required memory space");
            exit(EXIT_FAILURE);
        }
        guests = guestsNew;
        /*parse line: */
        line[strlen(line)-1] = '\0'; /*delete line-break character*/
        char *token = strtok(line, ";");
        guests[guestCounter].name = strdup(token);
        token = strtok(NULL, ";");
        if(token == NULL) {
            guests[guestCounter].number = strdup("missing");
        }
        else {
            guests[guestCounter].number = strdup(token);
        }
        if(guests[guestCounter].number == NULL) {
            perror("Error while allocating required memory space");
            cleanUp(guestCounter, guests, line, file);
            exit(EXIT_FAILURE);
        }
        guestCounter++;
    }
    /*input is done, now output file contents: */
    int i;
    for(i=0; i<guestCounter; i++) {
        printf("Name: %s, Number: %s\n", guests[i].name, guests[i].number);
    }
    /*clean up:*/
    cleanUp(guestCounter, guests, line, file);
    return EXIT_SUCCESS;
}

void cleanUp(int guestCounter, struct Guest *guests, char *line, FILE *file) {
    int i;
    for(i=0; i<guestCounter; i++) {
        free(guests[i].name);
        free(guests[i].number);
    }   
    free(guests);
    free(line);
    fclose(file);
}
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Just to be sure, you are aware that a CSV file structure is a bit more complex than that, and that fields can be quoted, and will be if they contain double quotes, the separator character, or line feeds? Also that the separator may be ; or ,? \$\endgroup\$
    – jcaron
    Sep 7 '21 at 0:22
  • 1
    \$\begingroup\$ Thanks for the reminder. I havn't thought about this because I created the files to be read myself. Maybe I will wirte a new version that can handle more complex data. \$\endgroup\$
    – WuselDusel
    Sep 7 '21 at 6:43
  • \$\begingroup\$ @jcaron Not only can the separator be a ,, but it should be a comma, as CSV literally stands for Comma Separated Value. \$\endgroup\$
    – Glen Yates
    Sep 7 '21 at 18:31
  • \$\begingroup\$ @GlenYates in countries which use a comma rather than a dot as a decimal separator it is usual to use a semi-colon rather than a comma as column separator in CSV files. But CSV is just a mess that should be avoided at all costs. \$\endgroup\$
    – jcaron
    Sep 7 '21 at 20:46
8
\$\begingroup\$
#define MAX_LINE_WIDTH 100

Arbitrary limits such as this are difficult to get right. No matter how large you make it, the real world somehow always manages to find data that exceeds the limit.

In this case, as we use this limit with fgets(), we really need to check whether the terminating newline was read instead of always assuming we get a complete line.


FILE *file = fopen("gaesteliste.txt", "r");
if(file == NULL) {
    perror("Error while reading the file");
    exit(EXIT_FAILURE);
}

Consider just reading from stdin, and let the user redirect appropriately. This removes all the file handling from our program, allowing it to be simpler, and it gives more flexibility because we can now use any file we like.


    free(file);

This is plain wrong - we are not allowed to free the pointer returned by fopen(). We need fclose(file) here instead.


char *line = malloc(MAX_LINE_WIDTH * sizeof(char));

It's not clear why we allocated this from dynamic memory. Since we never change its size or keep a long-lived pointer, we could just use automatic storage:

char line[MAX_LINE_WIDTH];

Alternatively, keep the dynamic allocation, but be prepared to enlarge the buffer when we find a longer line. On POSIX platforms (likely since we're assuming strdup() is available), we have getline() available to do that for us.

As an aside, note that multiplying by sizeof (char) is a no-op, since char is the fundamental unit of storage used for measuring sizes and therefore its size must be 1.


Reallocating the array of guests every time we add one is inefficient. It's better to allocate in larger chunks, and keep track of our capacity and how much of it that we've used so far:

struct GuestList {
    size_t capacity;
    size_t count;
    struct Guest *data;
};

Then we only need to allocate when count reaches capacity, and we can then increase the capacity by a decent amount.

(A more advanced approach would use a flexible array member to store the list and its data in a single allocation.)

When we fail to reallocate, we free some of our storage:

    if(guestsNew == NULL) {
        free(guests);
        free(line);
        free(file);
        perror("Error while allocating required memory space");
        exit(EXIT_FAILURE);
    }

Again, we should have fclose(file) rather than free(file). The other error is that we haven't freed the contents of guests, as we would have done if we'd just called the cleanUp() function.

I would actually change that cleanUp() as it's very closely tied to main() (and really ought to be declared with static linkage). Instead, we should have a general guest_list_free() that can be used by any code working with a struct GuestList:

void guest_list_free(struct GuestList* list)
{
    if (!list) {
        return;  /* nothing to do */
    }
    for (size_t i = 0;  i < list->count;  ++i) {
        free(list->data[i].name);
        free(list->data[i].number);
    }
    free(list->data);
    free(list);
}

That's best paired with a constructor function that creates a fully initialised list:

struct GuestList* guest_list_alloc(void)
{
    struct GuestList* list = malloc(sizeof *list);
    if (list) {
        list->count = list->capacity = 0;
        list->data = NULL;
    }
    return list;
}

    line[strlen(line)-1] = '\0'; /*delete line-break character*/

This line doesn't do what the comment says if the input stream line was MAX_LINE_WIDTH or more.


    guests[guestCounter].name = strdup(token);

We have null-pointer tests for almost all the memory allocation functions, except this one. I guess that's an oversight? It could easily be added to the if test of the .number copy.


    if(token == NULL) {
        guests[guestCounter].number = strdup("missing");
    }
    else {
        guests[guestCounter].number = strdup(token);
    }

Simplification:

    guests[guestCounter].number = strdup(token ? token : "missing");

Modified code

This is in two parts, and I've made some other changes not explicitly called out in the review above.

First, the GuestList type and its methods:

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

struct Guest {
    char *name;
    char *number;
};

struct GuestList {
    size_t capacity;
    size_t count;
    struct Guest *data;
};

struct GuestList* guest_list_alloc(void)
{
    struct GuestList* list = malloc(sizeof *list);
    if (!list) { return list; }
    list->count = list->capacity = 0;
    list->data = NULL;
    return list;
}

void guest_list_free(struct GuestList* list)
{
    if (!list) {
        return;  /* nothing to do */
    }
    for (size_t i = 0;  i < list->count;  ++i) {
        free(list->data[i].name);
        free(list->data[i].number);
    }
    free(list->data);
    free(list);
}

struct Guest *guest_list_item(const struct GuestList* list, size_t index)
{
    return list->data + index;
}

/* allocation size */
static const size_t guest_list_increment = 10;

struct Guest *guest_list_add(struct GuestList* list, const char *name, const char *number)
{
    if (!list || !name || !number) {
        return NULL;
    }
    if (list->capacity <= list->count) {
        /* increase capacity */
        size_t new_capacity = list->capacity + guest_list_increment;
        struct Guest *data = realloc(list->data, sizeof *data * new_capacity);
        if (!data) {
            return NULL;
        }
        list->data = data;
        list->capacity = new_capacity;
    }

    struct Guest *this_item = list->data + list->count;
    this_item->name = strdup(name);
    this_item->number = strdup(number);
    if (!this_item->name || !this_item->number) {
        free(this_item->name);
        free(this_item->number);
        return NULL;
    }
    ++list->count;
    return this_item;
}

Then the program to use it:

#include <errno.h>
#include <stdio.h>

/* populate list from stream.  Sets errno if it fails. */
static void read_guest_list(struct GuestList* list, FILE *stream)
{
    char *line = NULL;
    size_t line_len = 0;
    ssize_t chars_read;
    while ((chars_read = getline(&line, &line_len, stream)) > 0) {
        /*parse line: */
        if (line[chars_read-1] == '\n') {
            line[chars_read-1] = '\0';
        }
        char *name = strtok(line, ";");
        char *number = strtok(NULL, ";");
        if (!number) {
            /* no delimiter - just ignore this line */
            continue;
        }
        if (!guest_list_add(list, name, number)) {
            free(line);
            return;
        }
    }
    free(line);
}


int main(void)
{
    struct GuestList *guests = guest_list_alloc();
    if (!guests) {
        perror("Couldn't create guest list");
        return EXIT_FAILURE;
    }

    errno = 0;
    read_guest_list(guests, stdin);
    if (errno) {
        perror("Failed to add a guest");
        guest_list_free(guests);
        return EXIT_FAILURE;
    }

    /*input is done, now output file contents: */
    for (size_t i = 0;  i < guests->count;  ++i) {
        const struct Guest *g = guest_list_item(guests, i);
        printf("Name: %s, Number: %s\n", g->name, g->number);
    }

    guest_list_free(guests);
    return EXIT_SUCCESS;
}
\$\endgroup\$
5
  • \$\begingroup\$ Thanks for your detailed feedback. It helped a lot. I tried to implement the guestList structure and free-function you recommended. However, I do not know whether the name and number pointers at position guests.count have already been assigned, when guest_list_free is called. As both pointers use unitialized memory, do I risk to hand "random pointers" to free? If yes, how do I avoid it? \$\endgroup\$
    – WuselDusel
    Sep 6 '21 at 13:44
  • \$\begingroup\$ The best way is to write a function that returns either a null pointer (if the allocation failed) or a pointer to a fully initialised structure. If you have ever done any object-oriented programming (perhaps in Java, for example), you'll recognise that pattern as a constructor. \$\endgroup\$ Sep 7 '21 at 6:56
  • \$\begingroup\$ I've edited to show a suitable constructor. \$\endgroup\$ Sep 7 '21 at 7:02
  • \$\begingroup\$ I can't help but see this as a continuous example of why C++ is better. Check for error and free up all the memory before exiting: destructors. Tracking space remaining in a collection and re-allocating when it gets full: vector. Fixed size string buffers when reading input -- yipes! guest_list_free : just busywork that you must get right, but the compiler should have done for you. \$\endgroup\$
    – JDługosz
    Sep 10 '21 at 15:33
  • 1
    \$\begingroup\$ @JDługosz, agreed. C++, Java, Python and many other languages are much easier on the programmer. I guess the only good reason to choose C for this is the one given in the question: "trying to improve my C skills"... ;) \$\endgroup\$ Sep 21 '21 at 7:43
4
\$\begingroup\$

Toby Speight has already given a great answer, I'd just like to highlight the security issues:

Remove the line length limit

You have arbitrarily set a limit to the length of lines. However, what happens if the input has a line longer than that? If fgets() reaches the maximum line length, it will stop reading any further characters, and will return a pointer to the start of the buffer. That means the buffer has no newline character at the end. You are deleting the last character blindly. You then proceed to parse the line, reading incorrect data.

Even worse, the next call to fgets() will start where the previous one stopped. So it will continue reading the remainder of the line.

A 100 character line length would mean you would not be able to store the full name of people like Picasso, and there are people with even longer names.

Note that phone numbers themselves can be quite long; the E.164 standard limits them to 15 digits, but that is excluding any dashes or leading + sign, and also excludes any further digits needed for private phone switches. While I don't think you will ever run into a 100 digit telephone number, it can reduce the amount of characters available for the person's name.

Failure to correctly parse the line and continue without detecting the problem is a security issue. To solve it, there are a few options:

  • Use a buffer allocated with malloc(), and if there is no newline at the end of the buffer when fgets() returns, double the size of the buffer with realloc(), and try to read the remaining part of the line with fgets(), until you do find a newline.
  • Use the POSIX getline() function, which will allocate a buffer that is large enough for you.
  • Use a fixed but much larger size for the buffer, but exit with an error if there is no newline at the end of the buffer after calling fgets().

Validate the input

The CSV file might come from an untrusted source, or might have been corrupted in some way due to disk or network errors. It is possible that NUL bytes or control characters are part of the lines. It is a good idea to validate the input, by checking that there are no unexpected characters in any of the fields.

However, don't be too strict; in particular, don't assume anyhing about the format of names.

Avoid using strtok()

The function strtok() stores its state in a global buffer. This is fine for your program as it is now, but it will be a problem if your program grows and more parts of it want to parse things with strtok(). It is not thread-safe, and also has issues with reentrancy. I recommend you either use the C11 strtok_s() or the POSIX strtok_r() if available.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.