Setting char* to NULL segfault

Question

In the function last_letter() The line c = NULL will cause this program to segfault at while (*c) Commenting it out will not. What's the reason? Setting char pointer to NULL and reassigning something to it? I thought setting pointers to NULL and reassigning them to something else was acceptable?

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

char s1[] = "abcdefg";
char s2[] = "xyz";
char* c;

void last_letter(char* a, int i) {
  printf("last_letter ( a is %s and i is %d)\n", a, i);
  sleep(i);
  c = NULL;   // comment out, what is different?
  sleep(i);
  c = a;
  sleep(i);
  while (*c) {
    c++;
  }
  printf("%c\n", *(c-1));
  return;
}


// This function will run concurrently.

void* aa(void *ptr) {
  last_letter(s2, 2);
  return NULL;
}

int main() {
  pthread_t t1;
  int iret1 = pthread_create(&t1, NULL, aa, NULL);
  if (iret1) {
    fprintf(stderr,"Cannot create thread, rc=%d\n", iret1);
  }
  last_letter(s1, 5);
  sleep(10);
  printf("Ended nicely this time\n");
  return 0; //never reached when c = NULL is not commented out.
}

If you set c to NULL then the other thread might try to use *c while c is NULL. — Stack Exchange Broke The Law
– Stack Exchange Broke The Law, Commented Feb 13, 2017 at 23:59

Henry Rillovian · Accepted Answer · 2017-02-14 00:00:20Z

4

First, this is a classic race condition. This program depends on the order that aa() is called.

The last argument to pthread_create() is the argument. From the manpage:

int pthread_create(pthread_t *thread, const pthread_attr_t *attr,
                   void *(*start_routine) (void *), void *arg);

So when aa() is called via the pthread mechanism, the a argument is NULL, since pthread_create() is called with a NULL arg.

Then this loop dereferences a NULL pointer, causing a crash:

c = a;
sleep(i);
while (*c) {
    c++;
}

so give pthread_create() a non-NULL argument and you're on a better track.

answered Feb 14, 2017 at 0:00

Henry Rillovian

1841 silver badge12 bronze badges

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

Comments

Paul Rooney · Accepted Answer · 2017-02-14 08:44:55Z

Your variable c is global and so shared by all threads. Its used in last_letter which has the possibility of being called in both the main and the new thread at the same time. As it cannot be known the order in which each thread will execute last_letter, both may be interleaving changes to c. So one thread could set it to NULL when the other is expecting it to be a valid value.

One very simple way to very likely stop the crash is to swap the order of the sleep and the call to last_letter in the main thread

sleep(10);
last_letter(s1, 5);

This buys t1 10 seconds to finish, which hopefully is enough. A better way to do it is to join t1 before calling last_letter in the main thread. Yet a better way to do it is to move c inside last_letter, therefore each thread has a unique version of cand will not tread on the toes of the other threads. If c had to be shared, you would need to protect it with a mutex.

This way you needn't hold up main executing last_letter while t1 also executes it. Since main and t1 are operating on distinct data and also don't mutate that data, it's safe.

You would still want to call join on t1 though, otherwise there is the possibility that the main might complete and exit before t1 completes.

#include <stdio.h>
#include <unistd.h>
#include <pthread.h>

char s1[] = "abcdefg";
char s2[] = "xyz";

void last_letter(char* a, int i) {

  char* c;
  printf("last_letter ( a is %s and i is %d)\n", a, i);
  sleep(i);
  c = NULL; // pointless but if you must
  sleep(i);
  c = a;
  sleep(i);
  while (*c) {
    c++;
  }
  printf("%c\n", *(c-1));
}

// This function will run concurrently.
void* aa(void *ptr) {
  last_letter(s2, 2);
  return NULL;
}

int main() {
  pthread_t t1;

  int iret1 = pthread_create(&t1, NULL, aa, NULL);
  if (iret1) {
    fprintf(stderr,"Cannot create thread, rc=%d\n", iret1);
    return 0;
  }  

  last_letter(s1, 5);
  pthread_join(t1, NULL); // should check return value in production code

  printf("Ended nicely this time\n");
  return 0; //never reached when c = NULL is not commented out.
}

Collectives™ on Stack Overflow

Setting char* to NULL segfault

2 Answers 2

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related