-1

Apologies for the really bad question - I wasn't really sure how to word it. I'm executing a piece of code where I'm trying to use a dynamic array. It is segfault-ing at this line: 

void myFunction(....) {
     // other code up here
     Stack *s = stack_new(); //segfault here
}

The relevant header file for my struct is:

typedef struct {
    void **A;   
    int size;   
    int top;    // the index of the current top of the stack
} Stack;

and the function stack_new() is:

Stack 
*stack_new() {
    Stack *s; 
    s->size = 1; 
    s->top = -1; 
    s->A = (void **)malloc(s->size);
    return s;
}

I think I've included everything that is relevant, but please let me know if you need more code.

I think that the problem is with the way I'm using malloc, but have had a search online and have tried a few different options and am still getting the segfault. Is anyone able to offer some insight?

Thank you heaps

KittiCat
  • 41
  • 1
  • 8

3 Answers3

4

This is your problem:

Stack *s; 
s->size = 1;

you're not actually allocating a Stack. s is uninitialized and points to an arbitrary location in the memory. s->size will obviously fail then.

Try:

Stack *s = malloc(sizeof(*s));
if (s == NULL)
{
    fprintf(stderr, "Memory allocation error\n");
    exit(1);
}
s->size = 1;

Note: you should also check if s->A is NULL. If so, return an error code (such as NULL) and before that remember to free the Stack you allocated, or alternatively print an error message and exit the program. If you exit the program, the operating system will reclaim all memory used so no need to do it explicitly then.

Another note: when doing

s->size = 1; 
s->top = -1; 
s->A = (void **)malloc(s->size);

...you allocate 1 byte of memory even though you should be allocating sizeof(void*) bytes of memory. Try doing

s->A = (void **)malloc(s->size*sizeof(void*));

instead.

juhist
  • 4,210
  • 16
  • 33
  • I disagree with @RickyMutschlechner, it's `sizeof(*s)` not `sizeof(s)`. This answer looks good to me. – Maxime Chéramy Mar 25 '15 at 11:42
  • @Maxime just checked and I think this is correct (what you/answerer said) – Ricky Mutschlechner Mar 25 '15 at 11:43
  • 1
    What `Stack *s` means essentially is that `*s` is a `Stack` following the declaration follows usage principle. So, it is correct. `malloc(sizeof(s))` would be 4 or 8 bytes depending on 32-bit / 64-bit arch as `s` is a `Stack*` – juhist Mar 25 '15 at 11:44
  • Thank you!!! This is worked! I commented down below on @RickyMutschlechner's post, just confirming I understood the problem right. Just one more question (that is probably very stupid), would size still be 1 now? – KittiCat Mar 25 '15 at 11:52
  • 1
    Note that this isn't the only problem in your program. You allocate 1 byte of memory even though you should allocate `sizeof(int)` bytes of memory. – juhist Mar 25 '15 at 11:54
  • @juhist Oh! I think I understand. So I'm not allocating enough memory for even a single int? – KittiCat Mar 25 '15 at 12:02
  • 1
    Correction: you should allocate `sizeof(void*)`, not `sizeof(int)`. I have edited my post to show how you should allocate the memory. – juhist Mar 25 '15 at 12:10
  • Okay, just to make sure I understand what's going on: so initially I had s, but had not allocated it memory, and so had to do so. Then I also had to allocate memory to s->A, which is fine, but instead of giving it room to fit a "void*" (since it's an array of void*), I had just given it 1 byte. And that's why I should use sizeof(void*) * s->size, so that s->size tells me how many void* I can fit in the array? Does that mean that when I realloc, I only realloc for s->A or should I realloc memory for s too? – KittiCat Mar 25 '15 at 12:15
  • You don't need to reallocate memory for `s` too because it is fixed-size. But memory for `s->A` needs to be reallocated if `size` grows. I recommend adding `capacity` in addition to `size` and allocating memory for `capacity` entries. Then, `size` tells how many entries contain useful data and `capacity` tells how many entries fit into the array at most. Then, when `size` would exceed `capacity`, you multiply `capacity` by 2 and `realloc` memory for the new `capacity`. – juhist Mar 25 '15 at 12:18
2

Here's your first problem:

Stack *s; 
s->size = 1;

What do you actually expect the value of s is at this point? It could be literally anything. You can't set a field of a struct if the struct itself isn't already allocated.

Try:

Stack *s = malloc(sizeof(*s));
if(!s){
     //... error checking / exiting ..
}

and then everything else you were doing.

Ricky Mutschlechner
  • 4,291
  • 2
  • 31
  • 36
1

You are accessing a not initialized pointer! 

Stack 
*stack_new() {
    Stack *s = std::nullptr;  // initialize this pointer with nullptr
                              // and then you will see later (one line
                              // beyond) that you will try to access a
                              // null pointer
    s->size = 1; // the problem occurs here!!
                 // you are accessing a pointer, for which has never
                 // been allocated any memory
    s->top = -1; 
    s->A = (void **)malloc(s->size);
    return s;
}

You will have to use "malloc" to allocate some memory for this pointer. sth. like this is missing between those two lines, I commented:

Stack 

*stack_new() {
  Stack *s = (Stack*)malloc(sizeof(Stack));
  s->size = 1;
  s->top = -1; 
  s->A = (void **)malloc(s->size);
  return s;
}
Mr.Yellow
  • 197
  • 1
  • 8