invalid read of size 1 from valgrind

Question

After I fixed the memory leak, valgrind shows me a bunch of lines errors as the following, and I have no idea how to fix that. Is it because I free more space than I need or something?

line 39:

root = importTree(*(argv+1));

line 72:

 printf("Result found for %d:\n       city: %s\n       state:%s\n", zip, new->city, new->state);

Node *importTree(char *filename) {
    Node *root = NULL;
    FILE *fp = fopen(filename, "r");

    if (!fp) {
        printf("Error opening file.\n");
        return NULL;
    }

    while (!feof(fp)) {
        Node *new = malloc(sizeof(Node));
        if (!new) {
            printf("Failed to allocate memory. Ending read.\n");
            exit(1);
            fclose(fp);
        }
        new->city = malloc(sizeof(char) * MAXCITYNAME);
        if (!(new->city)) {
            printf("Failed to allocate memory. Ending read.\n");
            exit(1);
            fclose(fp);
        }
        new->left = NULL;
        new->right = NULL;
        char *line = malloc(sizeof(char) * MAXLINELENGTH);
        if (!line) {
            printf("Failed to allocate memory. Ending read.\n");
            exit(1);
            fclose(fp);
        }
        if (fgets(line, MAXLINELENGTH, fp) == NULL) {
            if (!feof(fp)) {
                printf("File reading ended prematurely. Check for errors in the file.\n");
                exit(1);
                fclose(fp);
            }
            free(new->city);
            free(line);
            free(new);
            fclose(fp);
            break;
        }
        char *tmp = strtok(line, ",");
        new->zipCode = atoi(tmp);
        tmp = strtok(NULL, ",");
        strcpy(new->city, tmp);
        new->city[strlen(tmp) + 1] = '\0';
        tmp = strtok(NULL, ",");
        strcpy(new->state, tmp);
        new->state[2] = '\0';
        root = addNode(root, new);
        if (!root) {
            printf("Root of tree is still NULL! Ending read.\n");
            exit(1);
        }
        free(line);
        free(new->city); \\this is the line 220
    }
    fclose(fp);
    return root;
}

Here is valgrind's output:

Invalid read of size 1
==7879==    at 0x517FAB4: vfprintf (vfprintf.c:1635)
==7879==    by 0x5188C98: printf (printf.c:34)
==7879==    by 0x400BBD: main (hw3.c:72)
==7879==  Address 0x5657a30 is 0 bytes inside a block of size 30 free'd
==7879==    at 0x4C2AD17: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==7879==    by 0x40103F: importTree (hw3.c:220)
==7879==    by 0x400A31: main (hw3.c:39)
==7879==
==7879== Invalid read of size 1
==7879==    at 0x51ADA99: _IO_file_xsputn@@GLIBC_2.2.5 (fileops.c:1342)
==7879==    by 0x517FA6C: vfprintf (vfprintf.c:1635)
==7879==    by 0x5188C98: printf (printf.c:34)
==7879==    by 0x400BBD: main (hw3.c:72)
==7879==  Address 0x5657a37 is 7 bytes inside a block of size 30 free'd
==7879==    at 0x4C2AD17: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==7879==    by 0x40103F: importTree (hw3.c:220)
==7879==    by 0x400A31: main (hw3.c:39)
==7879==
==7879== Invalid read of size 1
==7879==    at 0x51ADAAC: _IO_file_xsputn@@GLIBC_2.2.5 (fileops.c:1342)
==7879==    by 0x517FA6C: vfprintf (vfprintf.c:1635)
==7879==    by 0x5188C98: printf (printf.c:34)
==7879==    by 0x400BBD: main (hw3.c:72)
==7879==  Address 0x5657a36 is 6 bytes inside a block of size 30 free'd

Answer 1

Why do you free the city member of the new structure after inserting the node in the tree? It belongs to the structure, it will be freed twice when you deallocate the tree.

There are other issues in your code:

• while (!feof(fp)) is always wrong. In your case, just run forever with for(;;) and exit the loop gracefully when fgets() fails at end of file.
• You should not use new as an identifier, it confuses the code colorizer (and the reader) because it is a keyword in C++. Just call it node or np.
• The line new->city[strlen(tmp) + 1] = '\0'; is at best useless, and may cause a buffer overflow.
• indeed, we don't know the values of your array sizes, but you do not check the sizes before calling strcpy() to copy line chunks. This may invoke undefined behavior.
• You should instead use strdup() to allocate the correct size for the line fragments and copy the contents in one simple step.
• Similarly, allocating the node after a successful fgets() would simplify the error cases.
• You do not check the return value of strtok(). Invalid file contents may cause the return value to be NULL, invoking undefined behavior instead of a graceful fatal error.

Here is a simpler corrected version:

Node *importTree(const char *filename) {
    char line[MAXLINELENGTH];
    Node *root = NULL;
    FILE *fp = fopen(filename, "r");

    if (!fp) {
        printf("Error opening file.\n");
        return NULL;
    }

    while (fgets(line, MAXLINELENGTH, fp) != NULL) {
        Node *node = malloc(sizeof(Node));
        if (!node) {
            printf("Failed to allocate memory. Ending read.\n");
            fclose(fp);
            exit(1);
        }
        new->left = NULL;
        new->right = NULL;
        char *tmp = strtok(line, ",");
        if (!tmp) {
            printf("Invalid file contents. Ending read.\n");
            fclose(fp);
            exit(1);
        }
        new->zipCode = atoi(tmp);
        tmp = strtok(NULL, ",");
        if (!tmp) {
            printf("Invalid file contents. Ending read.\n");
            fclose(fp);
            exit(1);
        }
        node->city = strdup(tmp);
        if (!(new->city)) {
            printf("Failed to allocate memory. Ending read.\n");
            fclose(fp);
            exit(1);
        }
        tmp = strtok(NULL, ",");
        if (!tmp) {
            printf("Invalid file contents. Ending read.\n");
            fclose(fp);
            exit(1);
        }
        tmp[2] = '\0';
        node->state = strdup(tmp);
        if (!node->state) {
            printf("Failed to allocate memory. Ending read.\n");
            fclose(fp);
            exit(1);
        }
        root = addNode(root, node);
        if (!root) {
            printf("Root of tree is still NULL! Ending read.\n");
            fclose(fp);
            exit(1);
        }
    }
    if (!feof(fp)) {
        printf("File reading ended prematurely. Check for errors in the file.\n");
        fclose(fp);
        exit(1);
    }
    fclose(fp);
    return root;
}