0

While working on my school project I keep receiving following error from Valgrind after compiling my project on Unix school server.

==2951== Memcheck, a memory error detector
==2951== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==2951== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==2951== Command: ./Euler
==2951==
==2951== Invalid read of size 8
==2951==    at 0x400B65: GInit (Euler.c:64)
==2951==    by 0x400DD1: main (Euler.c:118)
==2951==  Address 0x1786100 is 0 bytes after a block of size 48 alloc'd
==2951==    at 0x100688B: malloc (in /usr/local/lib/valgrind/vgpreload_memcheck-amd64-freebsd.so)
==2951==    by 0x400A80: GInit (Euler.c:43)
==2951==    by 0x400DD1: main (Euler.c:118)
==2951==
==2951== Invalid write of size 4
==2951==    at 0x400B6B: GInit (Euler.c:64)
==2951==    by 0x400DD1: main (Euler.c:118)
==2951==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==2951==
==2951==
==2951== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==2951==  Access not within mapped region at address 0x0
==2951==    at 0x400B6B: GInit (Euler.c:64)
==2951==    by 0x400DD1: main (Euler.c:118)
==2951==  If you believe this happened as a result of a stack
==2951==  overflow in your program's main thread (unlikely but
==2951==  possible), you can try to increase the size of the
==2951==  main thread stack using the --main-stacksize= flag.
==2951==  The main thread stack size used in this run was 16777216.
==2951==
==2951== HEAP SUMMARY:
==2951==     in use at exit: 32,981 bytes in 16 blocks
==2951==   total heap usage: 16 allocs, 0 frees, 32,981 bytes allocated
==2951==
==2951== LEAK SUMMARY:
==2951==    definitely lost: 0 bytes in 0 blocks
==2951==    indirectly lost: 0 bytes in 0 blocks
==2951==      possibly lost: 0 bytes in 0 blocks
==2951==    still reachable: 32,981 bytes in 16 blocks
==2951==         suppressed: 0 bytes in 0 blocks
==2951== Reachable blocks (those to which a pointer was found) are not shown.
==2951== To see them, rerun with: --leak-check=full --show-reachable=yes
==2951==
==2951== For counts of detected and suppressed errors, rerun with: -v
==2951== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
Segmentation fault: 11

I seem to be incorrectly allocating memory when using malloc. I am aware of not freeing the memory, as I haven't yet implemented a delete function. Function GInit should read formatted data from file Graph1.txt and create a graph made up of nodes. File contains number of nodes and an incidence matrix.

Here is my code:

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

#define MAXFILENAME 20

typedef struct tNode{
    int Deg;
    int Val;    
    int* Neigh; 
} *tNodePtr;

typedef struct tGraph{
    int Num;    
    tNodePtr* Nodes;    
} *tGraphPtr;


void GInit(tGraphPtr G, const char *FNum)
{
    char FileName[MAXFILENAME];
    char *FileNamePrefix = "Graph";
    char *FileNamePostfix = ".txt";
    FILE *FilePtr;
    int FileBrowser;
    int i, j, k, countNeigh;
    char *line;
    char c;

    strcpy(FileName, FileNamePrefix);
    strcat(FileName, FNum);
    strcat(FileName, FileNamePostfix);

    FilePtr = fopen(FileName, "r");

    if(!FilePtr)
        printf("Can't open file \"%s\"\n", FileName);
    else
    {
        fscanf(FilePtr, "%d", &FileBrowser);

        G->Num = FileBrowser;
        G->Nodes = (tNodePtr*) malloc(sizeof(tNodePtr) * G->Num);

        for(i = 0; i < G->Num; i++)
            G->Nodes[i] = (tNodePtr) malloc(sizeof(struct tNode));

        line = (char*) malloc(sizeof(char) * (2*G->Num + 1));   

        i = 0;
        fscanf(FilePtr, "%c", &c);
        fgets(line, 2*G->Num + 1, FilePtr); 
        while(!feof(FilePtr))
        {
            countNeigh = 0;
            j = 0;
            while(line[j] != '\0')
            {
                if(line[j] == '1')
                    countNeigh++;
                j++;
            }

            G->Nodes[i]->Deg = countNeigh;
            G->Nodes[i]->Val = i;
            G->Nodes[i]->Neigh = (int*) malloc(sizeof(int) * countNeigh);

            j = 0;
            k = 0;
            while(line[j] != '\0')
            {
                if(line[j] == '1')
                {
                    G->Nodes[i]->Neigh[k] = j/2;
                    k++;
                }
                j++;
            }

            i++;    
            fgets(line, 2*G->Num + 1, FilePtr); 
        }

        free(line);
    }

    fclose(FilePtr);
}

void GPrint(const tGraphPtr G)
{
    int j, k;

    printf("Graph demonstration:\n");
    for(j = 0; j < G->Num; j++)
    {
        printf("I'm Node: %d , my degree is: %d and my neighbours are:\t", G->Nodes[j]->Val, G->Nodes[j]->Deg);
        for(k = 0; k < G->Nodes[j]->Deg; k++)
            printf("%3d", G->Nodes[j]->Neigh[k]);
        printf("\n");
    }
}

void GDelete(tGraphPtr G)
{

}

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

    tGraphPtr TmpGraph;
    char *FNum;
    FNum = "1";

    TmpGraph = (tGraphPtr) malloc(sizeof(struct tGraph));

    GInit(TmpGraph, FNum);

    GPrint(TmpGraph);


    return(0);  
}

Here is file Graph1.txt I am reading from:

6
0 1 0 1 0 0
1 0 1 0 1 1
0 1 0 1 1 1
1 0 1 0 0 0
0 1 1 0 0 0
0 1 1 0 0 0

Any advice how to fix this error is appreciated. BTW Microsoft VS2013 succesfully build this code and runs with no error. Thank you. John

scanjett
  • 25
  • 4
  • Side note: `type *name = malloc(count * sizeof *name)` is more maintainable, easier to write, shorter and more future proof than `type *name = (type *)malloc(count * sizeof(type))` which is very redundant. – Shahbaz Dec 05 '13 at 21:26
  • Valgrind says your issue is on line 64. Stand there with a debugger, inspect the variables, such as `i` , and see if the variables have the value you assume they have. Note e.g. if your file has an extra newline at the end, there's nothing in your code that stops you from reading more than 6 lines. Read this question too, and see if it applies to you: http://stackoverflow.com/questions/5431941/while-feof-file-is-always-wrong – nos Dec 06 '13 at 12:28

2 Answers2

2

You should do more error checking. Here are some places:

fscanf(FilePtr, "%d", &FileBrowser)

You're assuming that fscanf has successfully retrieved an int from the file. You should check this by verifying the value returned from fscanf is 1. If it's 0, you have a garbage value in FileBrowser.

Here is another problem:

G->Nodes = (tNodePtr*) malloc(sizeof(tNodePtr) * G->Num);

First of all, there's no need to typecast the return of malloc, so remove (tNodePtr*). Secondly, you're again assuming that malloc succeeded. You should make sure that it definitely did by comparing the address of G->Nodes to NULL. NULL would indicate a failure.

Fiddling Bits
  • 8,712
  • 3
  • 28
  • 46
  • I haven't ever been so convinced that you shouldn't cast malloc, I like to write code that doesn't freak out a C++ compiler where possible. – Grady Player Dec 05 '13 at 21:42
  • but yeah +1 for that fscanf.. that is probably a problem. – Grady Player Dec 05 '13 at 21:45
  • @GradyPlayer I haven't ever been totally convinced you shouldn't cast `malloc`, but smarter people than I have so you should, so... – Fiddling Bits Dec 05 '13 at 21:47
  • @user3071966 What about what `fscanf` is returning? – Fiddling Bits Dec 05 '13 at 21:56
  • @GradyPlayer, there are many things in C that freak out a C++ compiler. If you are so worried about the C++ compiler, of course you can always write C-like code in C++. The things that freak out C++ freak it out because they are not meant to be used in C++ and it has other ways of doing them. If you are writing C, you'd just be cluttering your code with such casts to satisfy a hypothetical C++ build which would detest your ways anyway! – Shahbaz Dec 05 '13 at 22:03
  • @GradyPlayer, casting to and from `void *` is just an example. Another example is C99's designated initializers: `var = (struct type_of_var){ .member1 = value1, .member2 = value2 };` which is a huge advantage in C code and it doesn't compile in C++. If you are trying to satisfy a C++ compiler while writing C code, you are just making it very hard on yourself (and basically limiting yourself to less than C89)! – Shahbaz Dec 05 '13 at 22:06
  • @Shahbaz you are right C++ compilers do hate my C (And Objective-C)... so that isn't a great reason, but it just falls into the style of being explicit, which I favor. always using braces to nest for/do/while/if/else blocks even for one element, while I don't generally cast the return of malloc/calloc, I also don't ever argue that it should be removed... does no harm. – Grady Player Dec 05 '13 at 22:11
  • @BitFiddlingCodeMonkey I have added some testing on NULL address after each malloc and none of the addresses is NULL. Fscanf is never 0 and fgets is never NULL except when scanning last line of the file, which is ok. Valgrind reports the same errors. – scanjett Dec 05 '13 at 22:20
  • 1
    @GradyPlayer, being too cautious could sometimes be dangerous. Specifically, casting the return value of `malloc` [could indeed be dangerous](http://stackoverflow.com/a/605858/912144). That said, using `sizeof(type)` is also dangerous (as opposed to `sizeof *var_being_allocated`), imagine a typo (one more or less `*`) or future change to the type of the variable (where updating the `sizeof` inside `malloc` could easily be forgotten). – Shahbaz Dec 05 '13 at 22:50
0

Just to extend Bit Fiddling Code Monkey's response, the fact that Valgrind complains about a bad read and a bad write in line 64, this is:

G->Nodes[i]->Deg = countNeigh;

Means that you're overpassing Nodes' size (bad read of Nodes[i]) and writing on a non allocated memory address (bad write in Deg).

This can be because fscanf failed or because the file contains more lines that initially stated in FileBrowser. For example, an extra empty line at the end of the file would cause this invalid read/write.

jcm
  • 2,568
  • 14
  • 18