Checking NULL Pointer In C Doesn't Work

Question

I have a function which returns a multiple indirection pointer as result like so:

typedef struct {
    int id;
    char *name;
} user;

user **myfn(int users_count) {
    user **a;
    a = malloc(sizeof(user) * user_counts);
    for(int i = 0 ; i<user_counts ; i++) {
        *(a+i) = malloc(sizeof(user));
        (*(a+i))->id = 1;
        (*(a+i))->name = malloc(sizeof(char) * 25);
        strncpy((*(a+i))->name, "Morteza", 25); // just for example
    }
    return a;
 }

Now, I when I want to crawl in this result in main function, it will show all users name, but at the end it will encounter with Segmentation fault error.

int main() {
    user **a = myfn(10);
    int i = 0;
    while((*(a+i)) != NULL) {
        printf("ID: %d \t %s\n", (*(a+i))->id, (*(a+i))->name);
        i++;
    }
}

Results:

ID: 1 Morteza
ID: 2 Morteza
...
...
ID: 10 Morteza
Segmentation fault (core dumped)

Why condition of while doesn't work fine?

a = malloc(sizeof(user) * user_counts); is wrong. *a is a user *, not a user. — melpomene
– melpomene, Commented Jan 25, 2017 at 22:10
@Mortezaipo The general rule is when the pointer has N stars in the declaration, you use N-1 stars in the sizeof argument in malloc(). — Barmar
– Barmar, Commented Jan 25, 2017 at 22:38

John Bode · Accepted Answer · 2017-01-25 22:53:08Z

2

First of all,

a = malloc(sizeof(user) * user_counts);

has a problem - you want to allocate user_counts instances of pointers to user, not instances of user, so that line should be

a = malloc(sizeof(user *) * user_counts);

However, there's an easier way around this - the sizeof operator can take expressions as arguments as well as type names. So you can rewrite that line as

a = malloc( sizeof *a * user_counts );

The expression *a has type user *, so sizeof *a is equivalent to sizeof (user *). This makes your life a bit simpler, in that you don't have to puzzle out the type that a points to - make the compiler do the hard work.

You should always check the result of a malloc call.

a = malloc( sizeof *a * users_count );
if ( a )
{
  // do stuff with a
}

Second of all, don't use *(a+i) to index into a - use a[i] instead. Makes things a bit easier to read. So,

   *(a+i) = malloc(sizeof(user));
    (*(a+i))->id = 1;
    (*(a+i))->name = malloc(sizeof(char) * 25);

becomes

   a[i] = malloc(sizeof *a[i]);
   if ( a[i] )
   {
     a[i]->id = 1;
     a[i]->name = malloc(sizeof *a[i]->name * 25);
   }
   else
   {
     /* deal with memory allocation failure */
   }

Now, to your actual problem. Your code is crashing for one of the following reasons:

the initial malloc of a failed, so you're crashing on the first line that uses a[i];
the malloc of one of your a[i] failed, so you're crashing on a[i]->id = 1;
you've successfully allocated memory for all users_count elements of a - no a[i] is NULL, so you loop past the last element of your array and try to dereference the object immediately following it, which is most likely not a valid pointer.

In addition to adding the checks after each malloc call, you should probably loop based on users_count:

for ( i = 0; i < users_count; i++ )
  printf("ID: %d \t %s\n", a[i]->id, a[i]->name);

Or, you need to allocate one extra element for a and set it to NULL:

a = malloc( sizeof *a * (users_count + 1) );
if ( a )
{
  a[users_count] = NULL;
  for ( i = 0; i < users_count; i++ )
    ...
}

Note that calloc initializes all allocated memory to 0, so you can use that and not worry about explicitly setting an element to NULL:

a = calloc( users_count + 1, sizeof *a );
if ( a )
{
  for ( i = 0; i < users_count; i++ )
    ...
}

answered Jan 25, 2017 at 22:53

John Bode

125k19 gold badges130 silver badges211 bronze badges

Sign up to request clarification or add additional context in comments.

9 Comments

melpomene Over a year ago

Technically setting a pointer to all-bits-zero (as calloc does) is not guaranteed to produce a null pointer.

Andrew Henle Over a year ago

@melpomene Do you have a reference for that? C Standard, 6.3.2.3 Pointers, paragraph 3, states "An integer constant expression with the value 0, or such an expression cast to type void *, is called a null pointer constant. ..." That would at least be an implementation-dependent NULL pointer value on most platforms, I'd think. .

melpomene Over a year ago

@AndrewHenle I don't see how this applies here. calloc(...) doesn't involve any integer constant expression with the value 0.

John Bode Over a year ago

@AndrewHenle: Specifically, 5.18: "Q: Is a run-time integral value of 0, cast to a pointer, guaranteed to be a null pointer? A: No. Only constant integral expressions with value 0 are guaranteed to indicate null pointers. See also questions 4.14, 5.2, and 5.19." This is something I forget from time to time and have to be reminded of. Remember, 6.3.2.3/3 says "an integer constant expression with the value 0...". A runtime 0 is not an integer constant expression.

John Bode Over a year ago

@Mortezaipo: Depends on the database API, then - if it doesn't return the number of records returned, or if it doesn't provide a cursor for you to iterate through the result set, then it should add a sentinel record at the end of the sequence. If I understand what you're saying, anyway.

|

Collectives™ on Stack Overflow

Checking NULL Pointer In C Doesn't Work

1 Answer 1

9 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

9 Comments

Your Answer

Sign up or log in

Post as a guest

Related