Thread: C - realloc() I Keep getting Segmentation fault (core dumped)

  1. #1
    Registered User
    Join Date
    Jan 2020
    Posts
    5

    Unhappy C - realloc() I Keep getting Segmentation fault (core dumped)

    I have been getting generic Segmentation fault (core dumped) error for a day now. I just can't figure out what's the problem with my code.

    Code:
    typedef struct Node {
        int id;
        int type;
        float value;
        float bias;
        float pos;
    } Node;
    
    
    // Global
    int global_nodes_capacity = 64;
    int n_global_nodes = 0;
    Node *global_nodes;
    
    
    void increase_nodes_capacity(Node *nodes, int *capacity, int needed) {
        if (needed > *capacity) {
            *capacity = pow(2, ceil(log2(needed)));
            nodes = (Node *) realloc(nodes, *capacity * sizeof(Node));
        }
    }
    
    
    // In main()
    global_nodes = (Node *) malloc(global_nodes_capacity * sizeof(Node));
    
    
    // In another function called from main()
    increase_nodes_capacity(global_nodes, &global_nodes_capacity, n_global_nodes + 1);

  2. #2
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    The problem is that in increase_nodes_capacity, nodes is a pointer to Node. It should be a pointer to a pointer to Node so that you can change it from within the function.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  3. #3
    Registered User
    Join Date
    Jan 2020
    Posts
    5
    I already tried that, I think it didn't work. Let me check real quick.

  4. #4
    Registered User
    Join Date
    Jan 2020
    Posts
    5
    Ok, this worked. Thank you. I have 1 more problem tho.

    Code:
    typedef struct Node {
        int id;
        int type;
        float value;
        float bias;
        float pos;
    } Node;
    
    
    // Global
    int global_nodes_capacity = 64;
    int n_global_nodes = 0;
    Node *global_nodes;
    
    
    void increase_nodes_capacity(Node **nodes, int *capacity, int needed) {
        if (needed > *capacity) {
            *capacity = pow(2, ceil(log2(needed)));
            *nodes = (Node *) realloc(*nodes, *capacity * sizeof(Node));
        }
    }
    
    
    // In main()
    global_nodes = (Node *) malloc(global_nodes_capacity * sizeof(Node));
    
    
    // In another function called from main()
    increase_nodes_capacity(&global_nodes, &global_nodes_capacity, n_global_nodes + 1);
    This is the other thing that doesn't work.
    Code:
    typedef struct Creature {
        int id;
        int specie;
    
        int nodes_capacity;
        int n_nodes;
        Node *nodes;
    
        int connections_capacity;
        int n_connections;
        Connection *connections;
    
        int x, y;
        float energy;
        int alive;
        int age;
        float fitness;
    } Creature;
    
    
    void create_brain(Creature *creature) {
        creature->nodes_capacity = 64;
        creature->n_nodes = 0;
        creature->nodes = (Node *) malloc(creature->nodes_capacity * sizeof(Node));
    }
    
    
    void add_node(Creature *creature, int type) {
        increase_nodes_capacity(&creature->nodes, &creature->nodes_capacity, creature->n_nodes + 1);
    }
    Seg fault on realloc() again.

    EDIT: This is the error I'm getting.
    realloc(): invalid next size
    Aborted (core dumped)

    I also got realloc(): corrupt double-linked list
    Last edited by TypicalHog; 01-14-2020 at 04:12 PM.

  5. #5
    Registered User
    Join Date
    Feb 2019
    Posts
    1,078
    An warning: Don't do this:
    Code:
    nodes = realloc( nodes, *capacity * sizeof( Node ) );
    reallod() can fail (returning NULL). In that case, you'll loose the address contained in nodes pointer and get a memory leakage in your hands. It is safer to use an intermediary pointer, as in:
    Code:
    Node *p;
    
    p = realloc( nodes, *capacity * sizeof( Node ) );
    if ( ! p ) { ... deal with the failure here - nodes will not be affected... }
    ...
    nodes = p;  // if everything is ok, then you update the 'nodes' pointer.
    But remember: 'nodes' cannot be changed between function calls. Your 'increase_nodes_capacity()' should be something like this (assuming the code is correct):
    Code:
    // Now you must pass a pointer to the node pointer, so you can change it.
    int increase_nodes_capacity( Node **nodespp, size_t *capacity, size_t needed )
    {
      if ( needed > *capacity )
      {
        Node *p;
        size_t new_capacity;
    
        // No floating point needed - Store new_capacity to discard it in case of allocation error.
        // For x86-64 (for example), size_t is 64 bits long, so the constant is 1ULL.
        // ceil(log2(needed)) is the same as the position of the leading 1 bit.
        // and pow(2,n) is the same as shifting 1 'n' bits to the left.
        new_capacity = 1ULL << ( sizeof ( size_t ) - __builtin_clz( needed ) - 1);
    
        // 'new_capacity' can overflow, so this test is important.
        if ( ! new_capacity )
          return 0; // 0 means error.
    
        // Tries to allocate the new buffer.
        p = realloc( *nodespp, new_capacity * sizeof( Node ) );
        if ( ! p )
          return 0;  // 0 means error!
    
        // Everything OK, update the pointer and the capacity.
        *capacity = new_capacity;
        *nodespp = p;
      }
    
      return 1;  // 1 means OK.
    }
    Last edited by flp1969; 01-14-2020 at 04:19 PM.

  6. #6
    Registered User
    Join Date
    Jan 2020
    Posts
    5
    Thank your for the suggestions.
    Could you take a look at the 2nd problem I have (I assume you were writing an answer when I posted the other reply).

  7. #7
    Registered User
    Join Date
    Dec 2017
    Posts
    1,626
    @flp, Since he wants the ceiling of the value you should remove the -1 (although then it will overestimate an exact power of 2, but so what).
    However, it may be just as effective to simply double capacity when more space is needed. It depends on how memory needs increase.
    A little inaccuracy saves tons of explanation. - H.H. Munro

  8. #8
    misoturbutc Hodor's Avatar
    Join Date
    Nov 2013
    Posts
    1,791
    I think there is a typo here:
    Quote Originally Posted by flp1969 View Post
    new_capacity = 1ULL << ( sizeof ( size_t ) - __builtin_clz( needed ) - 1);
    Should really be using __builtin_clzll otherwise the calculation will be incorrect. Also, I believe, that sizeof(size_t) should be multiplied to get the number of bits in size_t. (also added john.c's suggestion)

    Code:
        new_capacity = 1ULL << ( sizeof ( size_t ) * CHAR_BIT - __builtin_clzll( needed - 1 ));

    Although it's clear I think it's important to mention that it's not portable code
    Last edited by Hodor; 01-14-2020 at 06:02 PM.

  9. #9
    Registered User
    Join Date
    Dec 2017
    Posts
    1,626
    I didn't even notice he left out the * CHAR_BIT too! I even had a * 8 in my little test. But presumably it should use an unsigned long long instead of size_t since size_t is not guaranteed to be unsigned long long. Maybe it should just use an unsigned long or even just an unsigned. Or if we really need to specify the number of bits, use one of the fixed-width types from stdint.h.

    As for portable code, maybe just:
    Code:
    while (capacity < needed) capacity *= 2;
    // Obviously capacity can't be 0.
    A little inaccuracy saves tons of explanation. - H.H. Munro

  10. #10
    misoturbutc Hodor's Avatar
    Join Date
    Nov 2013
    Posts
    1,791
    Quote Originally Posted by john.c View Post
    I didn't even notice he left out the * CHAR_BIT too! I even had a * 8 in my little test. But presumably it should use an unsigned long long instead of size_t since size_t is not guaranteed to be unsigned long long. Maybe it should just use an unsigned long or even just an unsigned. Or if we really need to specify the number of bits, use one of the fixed-width types from stdint.h.

    As for portable code, maybe just:
    Code:
    while (capacity < needed) capacity *= 2;
    // Obviously capacity can't be 0.
    I don't know what I'd do. On something with FPU I'd probably just leave it as

    Code:
    pow(2, ceil(log2(needed)))
    or use one of the bit twiddling approaches until profiling indicated that I needed something more efficient, basically because it doesn't seem that it's going to be called all that often unless you have a linked list that grows very quickly and in that case is a linked list the best thing anyway[1]? So, depends on the situation I guess

    [1] Edit, actually it's only going to be called about 58 times before things overflow
    Last edited by Hodor; 01-14-2020 at 09:07 PM.

  11. #11
    misoturbutc Hodor's Avatar
    Join Date
    Nov 2013
    Posts
    1,791
    Quote Originally Posted by TypicalHog View Post
    Ok, this worked. Thank you. I have 1 more problem tho.

    <...>

    void increase_nodes_capacity(Node **nodes, int *capacity, int needed) {
    if (needed > *capacity) {
    *capacity = pow(2, ceil(log2(needed)));
    *nodes = (Node *) realloc(*nodes, *capacity * sizeof(Node));
    }

    <...>
    I doubt the code I highlighted in bold is correct. Without seeing how things are being used I'd say that it's more likely that it should be

    Code:
    if (needed > *capacity - 1) {
    Assuming you're using 0-based indexing, but this is just a guess (Edit: in hindsight the problem is more likely to be how you're accessing the previously allocated memory rather than what I said and highlighted above)

    Also, is increasing capacity by powers of 2 really necessary? Seems like overkill to me (I'd probably just double it each time as john.c suggested)


    Edit2: Actually it's not even going to call realloc anywhere near 58 times before overflowing because you're multipying by sizeof(Node)
    Last edited by Hodor; 01-14-2020 at 10:39 PM.

  12. #12
    Registered User
    Join Date
    Feb 2019
    Posts
    1,078
    A small correction on Post #5:

    Code:
    ...
    new_capacity = 1ULL << ( 8 * sizeof ( size_t ) - __builtin_clz( needed ) - 1);
    ...

  13. #13
    Registered User
    Join Date
    Feb 2019
    Posts
    1,078
    Quote Originally Posted by Hodor View Post
    I think there is a typo here:


    Should really be using __builtin_clzll otherwise the calculation will be incorrect. Also, I believe, that sizeof(size_t) should be multiplied to get the number of bits in size_t. (also added john.c's suggestion)

    Code:
        new_capacity = 1ULL << ( sizeof ( size_t ) * CHAR_BIT - __builtin_clzll( needed - 1 ));

    Although it's clear I think it's important to mention that it's not portable code
    You are right: __builtin_clzll() and it is not portable... and I forgot sizeof returns the number of bytes, so I need to multiply sizeof( size_t ) by 8 as well...

    There is a small confusion with the use of CHAR_BIT... this is the number of bits of char type. A byte is defined as 8 bits long (since 1960's I believe).

    Thanks, anyway, for the correction!
    Last edited by flp1969; 01-15-2020 at 02:03 AM.

  14. #14
    misoturbutc Hodor's Avatar
    Join Date
    Nov 2013
    Posts
    1,791
    Quote Originally Posted by flp1969 View Post
    You are right: __builtin_clzll() and it is not portable... and I forgot sizeof returns the number of bytes, so I need to multiply sizeof( size_t ) by 8 as well...

    There is a small confusion with the use of CHAR_BIT... this is the number of bits of char type. A byte is defined as 8 bits long (since 1960's I believe).

    Thanks, anyway, for the correction!
    Cool.

    As for the CHAR_BIT, I don't think I'm interpreting things incorrectly. Section 6.2.6.1p4 of C11 says "Values stored in non-bit-field objects of any other object type consist of n x CHAR_BIT bits, where n is the size of an object of that type, in bytes". Section 6.5.3.4p2 says "The sizeof operator yields the size (in bytes) of its operand". 5.2.4.2.1 says that CHAR_BIT is "number of bits for smallest object that is not a bit-field (byte)". Based on that I would say that sizeof(size_t)*CHAR_BIT is the portable way, rather than multiplying by 8
    Last edited by Hodor; 01-15-2020 at 02:37 AM.

  15. #15
    Registered User
    Join Date
    Feb 2019
    Posts
    1,078
    Quote Originally Posted by Hodor View Post
    Cool.

    As for the CHAR_BIT, I don't think I'm interpreting things incorrectly. Section 6.2.6.1p4 of C11 says "Values stored in non-bit-field objects of any other object type consist of n x CHAR_BIT bits, where n is the size of an object of that type, in bytes"
    Thank you! This is very useful!

    []s
    Fred

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Segmentation Fault (Core Dumped)
    By Alma in forum C Programming
    Replies: 2
    Last Post: 09-29-2017, 02:14 PM
  2. Segmentation fault (core dumped) in #c?
    By hs101 in forum C Programming
    Replies: 9
    Last Post: 12-06-2012, 02:38 PM
  3. Segmentation Fault (core dumped)
    By Kevin Jerome in forum C++ Programming
    Replies: 5
    Last Post: 09-09-2012, 12:58 AM
  4. Segmentation Fault (Core Dumped)
    By pureenergy13 in forum C Programming
    Replies: 3
    Last Post: 11-02-2011, 07:50 AM
  5. Segmentation fault, core dumped
    By dweenigma in forum C Programming
    Replies: 2
    Last Post: 05-21-2007, 03:50 PM

Tags for this Thread