1

As the title states, I'm getting an error

Access violation reading location 0xCDCDCDCD.

Now I'm dealing with an array of linked lists, and I believe the trouble to be something around adding to the linked list. I'm fine with this usually, but I feel I'm doing something wrong with memory allocation.

Here are my structs:

Graph:

typedef struct graph
{
    int V;
    int *state;
    EdgeList *edges;
} Graph;

Edge:

typedef struct edge
{
    int toVertex;
    int weight;
} Edge;

EdgeList:

typedef struct edgeNode
{
    Edge edge;
    struct edgeNode *next;
} *EdgeList;

Here is the main function that runs it all:

main()
{
    Graph myGraph;
    scanf("%d", &(myGraph.V));
    myGraph.state = (int)malloc(myGraph.V*sizeof(int));
    myGraph.edges = (EdgeList*)malloc(myGraph.V*sizeof(EdgeList));
    int *inDegrees;
    inDegrees = (int)malloc(sizeof(int)*myGraph.V);

    /*  Sets all array values to 0  */
    for (int counter = 0; counter < myGraph.V; counter++)
    {
        inDegrees[counter] = 0;
    }


    for (int i = 0; i < myGraph.V; i++)
    {
        int number_of_edges;
        int input = 0;  /*For that little experimental bit*/
        scanf("%d", &(myGraph.state[i]));
        scanf("%d", &number_of_edges);
        if (number_of_edges > 0)
        {
            for (int j = 0; j < number_of_edges; j++)
            {
                Edge newEdge;
                scanf("%d,%d", &(newEdge.toVertex), &(newEdge.weight));
                inDegrees[newEdge.toVertex]++;
                printf("%s%d\n", "\nOoh, new input for ", newEdge.toVertex);

                /*insert at front*/
                EdgeList newNode = (EdgeList)malloc(sizeof (struct edgeNode));
                newNode->edge = newEdge;

                newNode->next = myGraph.edges[i];
                myGraph.edges[i] = newNode;


                /* Bit to calculate state.*/

                EdgeList current = myGraph.edges[i];

                while (current != NULL)
                {
                    if (current->edge.toVertex == i)
                    {
                        input += (current->edge.weight)*(myGraph.state[i]);
                    }
                    current = current->next;
                }
            }
            if (input > 0)
            {
                myGraph.state[i] = 1;
            }
            else
            {
                myGraph.state[i] = 0;
            }
        }
    }

    //print
    for (int k = 0; k < myGraph.V; k++)
    {
        printf("\n%s%d%s", "In degrees for ", k, ": ");
        printf("%d", inDegrees[k]);
    }

}

In particular, the error comes during the traversal of the linked list. It's in the above code, but I'll highlight it here:

EdgeList current = myGraph.edges[i];

while (current != NULL)
{
    if (current->edge.toVertex == i)
    {
        input += (current->edge.weight)*(myGraph.state[i]);
    }
    current = current->next;
}

If anyone can help, it'd be greatly appreciated because I'm rather stuck.

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • There may be dereference of uninitialized pointers. – MikeCAT Apr 30 '16 at 03:37
  • 1
    It would be helpful to know which line in all that code actually caused the error. Have you run this under a debugger? – Ryan Bemrose Apr 30 '16 at 03:40
  • Yes, @RyanBemrose. The 'next statement to be executed' thing that VS gives, stops at the line `printf("%s%d\n", "current one is: ", current->edge.toVertex);` However, the while loop does run at least once, because I've tested with print statements. After that though, it crashes. – user3414510 Apr 30 '16 at 03:42

1 Answers1

2
  1. An value in uninitialized buffer allocated via malloc() is assigned to newNode->edge in newNode->next = myGraph.edges[i];.
  2. The newNode is set to current via myGraph.edges[i] = newNode; and EdgeList current = myGraph.edges[i];.
  3. Assuming that malloc() succeeded, current isn't NULL here, so it is entering the loop.
  4. The uninitialized value assinged in 1 is assigned to current in current = current->next;.
  5. An undefined behavior is invoked by using value in buffer allocated via malloc() and uninitialized at current != NULL.

To fix this error, initialize myGraph.edges in, for example, this way:

myGraph.edges = (EdgeList*)malloc(myGraph.V*sizeof(EdgeList));
for (int i = 0; i < myGraph.V; i++)
{
    myGraph.edges[i] = NULL;
}

Also, remove the harmful casts to int of the pointer returned from malloc(). Casting the return values to pointers explicitly is also not considered as good.

Community
  • 1
  • 1
MikeCAT
  • 73,922
  • 11
  • 45
  • 70
  • I just asked a friend if that was a solution for this, seconds before I saw this reply. Thanks anyway, and thanks again for explaining the theory behind it (because I was just stabbing in the dark). – user3414510 Apr 30 '16 at 03:49
  • This solution initializes the pre-allocated `edges` memory block to zeros, thus setting all of those nodes' `next` pointers to NULL. The rest of the code is inserting new nodes at the front of the `edges` list. Traversing the list will NEVER be able to touch any of the pre-allocated nodes after the first node, since its `next` is always NULL even though other nodes exist after it. So either 1) don't pre-allocate the nodes if they are just going to waste memory, or else 2) initialize the `next` pointers after zeroing the memory block so the nodes can be used. – Remy Lebeau Apr 30 '16 at 03:55
  • @RemyLebeau Where do you think are the nodes to be wasted? Is this just an advice for the future? – MikeCAT Apr 30 '16 at 10:11
  • @MikeCAT this code is completely mishandling node allocation and node pointers. This is not how a linked list is supposed to be implemented. – Remy Lebeau Apr 30 '16 at 15:59
  • @RemyLebeau I did some [simulation](http://i.stack.imgur.com/0YxXg.png) and I don't think this implementation of linked list with the initialization added and the harmful castings removed is completely wrong, even though there are relatively minor problems (absense of error handling for input reading and memory allocation). Specifically what makes you think this code is completely wrong? – MikeCAT May 01 '16 at 05:00