3
\$\begingroup\$

I am trying to implement a "proper" pthread multithreading program with pthread mutex. I want it to be as C-standard-conforming and POSIX-conforming as possible. The below is my implementation, any thoughts or bugs?:

main.c:

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

#include "common.h"


static char shared_string[512];
pthread_mutex_t my_mutex;


void* func_that_takes_params(void* tpl) {
    struct ThreadPayload* args = (struct ThreadPayload*)tpl;
    while (!should_stop) {
        // Let's add a bit of perturbation, but not too much
        usleep(1000 * 1000 - args->thread_id);
        if (pthread_mutex_lock(&my_mutex) != 0) {
            perror("pthread_mutex_lock()");
            continue;
        }
        sprintf(shared_string, "[%d] Message from caller: %s\n",
            args->thread_id, args->message);
        write(STDOUT_FILENO, shared_string, strlen(shared_string));
        if (pthread_mutex_unlock(&my_mutex) != 0) {
            perror("pthread_mutex_unlock()");
            return NULL;
        }
    }
    size_t* ret = malloc(sizeof(size_t));
    if (ret != NULL) {
        *ret = strlen(args->message);
    } else {
        perror("malloc()");
    }
    return (void*)ret;
}


int main(void) {
    install_signal_handler();
    should_stop = false;
    if (pthread_mutex_init(&my_mutex, NULL) != 0) {
        perror("pthread_mutex_init()");
        return 1;
    }

    size_t thread_count = sizeof(thread_payloads)/sizeof(thread_payloads[0]);

    pthread_t ths[thread_count];
    size_t started_threads;
    for (started_threads = 0; started_threads < thread_count; ++started_threads) {
        if (pthread_create(&ths[started_threads], NULL, func_that_takes_params,
            &thread_payloads[started_threads]) != 0) {
            perror("pthread_create()");
            --started_threads;
            break;
        }
    }
    printf("%d threads started\n", started_threads);
    for (int i = 0; i < started_threads; ++i) {
        size_t* ret = NULL;
        if (pthread_join(ths[i], (void**)&ret) != 0) {
            perror("pthread_join()");
            continue;
        }
        if (ret != NULL) {
            printf("ret: %u\n", *ret);
        } else {
            printf("ret is NULL\n");
        }
        free(ret);
    }
    if (pthread_mutex_destroy(&my_mutex) != 0) {
        perror("pthread_mutex_destroy()");
    }
    return 0;
}

  • common.h
#include <signal.h>
#include <stdbool.h>

volatile sig_atomic_t should_stop;

struct ThreadPayload {
    size_t thread_id;
    char* message;
};

static struct ThreadPayload thread_payloads[] = {
    {
        .thread_id = 0,
        .message = "This is a test message"
    }, {
        .thread_id = 1,
        .message = "This is also a test message"
    }, {
        .thread_id = 2,
        .message = "This is yet another a test message"
    }, {
        .thread_id = 3,
        .message = "Hello world!!"
    }, {
        .thread_id = 4,
        .message = "foobar"
    }, {
        .thread_id = 5,
        .message = "0xDEADBEEF"
    }, {
        .thread_id = 6,
        .message = "The quick brown fox jumps over the lazy dog"
    }, {
        .thread_id = 7,
        .message = "Lorem ipsum dolor sit amet, consectetur adipiscing elit."
    }
};

static void signal_handler(int signum) {
    char msg[] = "Signal [  ] caught\n";
    msg[8] = '0' + signum / 10;
    msg[9] = '0' + signum % 10;
    size_t len = sizeof(msg) - 1;
    size_t written = 0;
    while (written < len) {
        ssize_t ret = write(STDOUT_FILENO, msg + written, len - written);
        if (ret == -1) {
            perror("write()");
            break;
        }
        written += ret;
    }
    should_stop = true;
}

// This function calls abort() if fails
void install_signal_handler() {
    struct sigaction act;
    if (sigemptyset(&act.sa_mask) == -1) {
        perror("sigemptyset()");
        abort();
    }
    act.sa_handler = signal_handler;  
    act.sa_flags = 0;
    if (sigaction(SIGINT,  &act, 0) + sigaction(SIGABRT, &act, 0) +
        sigaction(SIGQUIT, &act, 0) + sigaction(SIGTERM, &act, 0) < 0) {
        perror("sigaction()");
        abort();
    }
}
  • Makefile
CC = gcc
LIBS = -lpthread -lglib-2.0
OPTS = -O2 -Wall -pedantic -Wextra -Wc++-compat

main: main.out

main.out: main.c common.h
    $(CC) main.c -o main.out $(LIBS)

.PHONY: clean
clean:
    rm *.out
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Incorporating advice from an answer into the question violates the question-and-answer nature of this site. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so the answers make sense again. \$\endgroup\$ May 2 at 7:55

3 Answers 3

5
\$\begingroup\$

... to be as C-standard-conforming and POSIX-conforming as possible.

Enable more warnings

Save time, enable more warnings to see more weaknesses.

I compiled with gcc and -pedantic -Wall -Wextra -Wconversion and saw these below warnings. The format errors are especially egregious. Not sure why my compilation caught those and yours did not.

//sprintf(shared_string, "[%d] Message from caller: %s\n",
//        args->thread_id, args->message);
sprintf(shared_string, "[%zu] Message from caller: %s\n",
        args->thread_id, args->message);

-Wconversion is a pesky, yet insightful option. The 2nd & 3rd warnings, IMO, are noisy.

In function 'signal_handler':
warning: conversion from 'int' to 'char' may change value [-Wconversion]
   48 |     msg[8] = '0' + signum / 10;
      |              ^~~

warning: conversion from 'int' to 'char' may change value [-Wconversion]
   49 |     msg[9] = '0' + signum % 10;
      |              ^~~

warning: conversion to 'long unsigned int' from 'ssize_t' {aka 'long int'} may change the sign of the result [-Wsign-conversion]
   58 |         written += ret;
      |                 ^~

In function 'func_that_takes_params':
warning: format '%d' expects argument of type 'int', but argument 3 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
   93 |         sprintf(shared_string, "[%d] Message from caller: %s\n",
      |                                  ~^
      |                                   |
      |                                   int
      |                                  %ld
   94 |             args->thread_id, args->message);
      |             ~~~~~~~~~~~~~~~        
      |                 |
      |                 size_t {aka long unsigned int}

In function 'main':
warning: format '%d' expects argument of type 'int', but argument 2 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
  131 |     printf("%d threads started\n", started_threads);
      |             ~^                     ~~~~~~~~~~~~~~~
      |              |                     |
      |              int                   size_t {aka long unsigned int}
      |             %ld

warning: comparison of integer expressions of different signedness: 'int' and 'size_t' {aka 'long unsigned int'} [-Wsign-compare]
  132 |     for (int i = 0; i < started_threads; ++i) {
      |                       ^
warning: format '%u' expects argument of type 'unsigned int', but argument 2 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
  139 |             printf("ret: %u\n", *ret);
      |                          ~^     ~~~~
      |                           |     |
      |                           |     size_t {aka long unsigned int}
      |                           unsigned int
      |                          %lu

Avoid signum out of range

If signum < 0 or more than 99 leads to woes. Maybe:

static void signal_handler(int signum) {
  signum = (int) ((unsigned) signum % 100u);  // Add
  char msg[] = "Signal [  ] caught\n";
  msg[8] = '0' + signum / 10; 
  msg[9] = '0' + signum % 10;

Note signum %= 100; does not ensure positivity.

Maybe const?

Perhaps a little tighter control with const.

struct ThreadPayload {
  size_t thread_id;
  // char* message;
  const char* message;
};

Uncommon to put function definitions in a .h file

More common to create a common.c with function/object definitions and companion common.h with function/object declarations, typedefs, ....


Minor

  • Cast not needed in return (void*)ret;. I suppose cast is there for "-Wc++-compat".

sprintf()

A common feedback is to use snprintf() vs. sprintf()

// sprintf(shared_string, ...
snprintf(shared_string, sizeof shared_string, ...

To me, unless the return value is checked, that trades one problem for another.

Code could test as below. Your call as to what level of checking you want.

int len = snprintf(shared_string, sizeof shared_string, ...
if (len < 0 || (unsigned) len >= sizeof shared_string) {
  ; //Handle error
}
// Or
int len = snprintf(shared_string, sizeof shared_string, ...
assert(len >= 0 && (unsigned) len < sizeof shared_string);
(void) len; // To quiet warning when not compiled in DEBUG mode.
\$\endgroup\$
19
  • \$\begingroup\$ On "Enable more warnings", Do you also use gcc for this? \$\endgroup\$ May 2 at 1:43
  • \$\begingroup\$ @D.J.Elkind "Do you also use gcc for this?" --> Answer has "I compiled with gcc ...". \$\endgroup\$ May 2 at 1:45
  • \$\begingroup\$ Emmm sorry for missing your info. I just reviewed my code, seems I forget to append $(OPTS) to the compilation command. I can see a lot of warnings after appending it. \$\endgroup\$ May 2 at 1:51
  • 1
    \$\begingroup\$ @TobySpeight I suspect you are correct about -Wc++-compat. I do not use it nor recommend its use and so don't spend effort optimizing it. \$\endgroup\$ May 2 at 13:16
  • 1
    \$\begingroup\$ @D.J.Elkind "in my case, all c-strings are guaranteed to be null-terminated," is unclear. All C-strings are null-terminated by definition, else it is not a string. The sprintf() vs. snprintf() is an issue of length and buffer size, not null character and how to handle any error. \$\endgroup\$ May 2 at 13:21
3
\$\begingroup\$

C11 has optional support for threads and mutexes

I want it to be as C-standard-conforming and POSIX-conforming as possible.

C11 introduced support for threads and mutexes, avoiding the need to rely on POSIX functions. The only drawback is that this an optional part of the C11 standard, and some compilers still don't implement it.

About mixing write(), printf() and mutexes

Calls to write() might be atomic, depending on whether you are writing to a file, socket, pipe or something else, and depending on the amount you are writing. Therefore, you might not need to lock a mutex around calls to it if you are just writing single lines to a terminal. However, the threads you start are not the only ones that are trying to write to standard output: you still use printf() on the main thread. There is no guarantee that every line printed using printf() will be done so using a single write() call. Thus, the output from the threads could be mixed in an incorrect way with the output from the main thread.

You could also consider using only printf() in your code to write anything. This might also be enough, since the standard library might internally take a lock around each printf() call (see this StackOverflow question). However, again this only works if you don't mix it with threads that call write().

The safest option however is to always use a mutex when writing to standard ouptut. That way, you can also mix and match write() and printf() however you like. However, taking a mutex in a signal handler is problematic: what if the thread that got the signal already had locked the mutex? Trying to lock it again from within the signal handler will cause a deadlock. You could use pthread_mutex_trylock() in this case to do a best effort attempt at keeping the output synchronized.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ On C11's thread support, it is great to know that. I am not aware of this at the time of preparing this implementation. On the write()'s atomicity, do you have any reference? My understanding is that POSIX only requires write() to be re-entrant, but it can be interrupted (and thus not atomic). \$\endgroup\$ May 2 at 8:19
  • \$\begingroup\$ The short answer is "it depends". I've made use of write()s atomicity in some contexts before, but it was platform dependent, so in general it's indeed better to not assume anything and just use a mutex everywhere to synchronize output to stdout. \$\endgroup\$
    – G. Sliepen
    May 2 at 9:25
3
\$\begingroup\$

Undefined behaviour:

perror("write()");

perror() is async-signal-unsafe. As such, is it not possible to call it safely in a signal handler without invoking undefined behavior.

Reduce duplication:

/*
struct ThreadPayload {
    size_t thread_id;
    char* message;
}; 

static struct ThreadPayload thread_payloads[] = {
{
        .thread_id = 0,
        .message = "This is a test message"
}
*/

#define DEF(_id, _msg) \
        { .thread_id = _id, .message = _msg } 

struct ThreadPayload {
    size_t thread_id;
    char* message;
} static thread_payloads[] = { 
    DEF (0, "This is a test message"),
    DEF (1, "DEADBEEF"),
    ...
};

/* Could undef it here, should you so desire. */
#undef DEF 

Use more const:

/* In signal_handler() */
struct ThreadPayload {
   // size_t thread_id;
   // char* message;
   const size_t thread_id; /* We could do with unsigned here. */
   const char *const message;
};

// size_t len = sizeof(msg) - 1;
const size_t len = sizeof msg - 1;

/* In main() */
// size_t thread_count = sizeof(thread_payloads)/sizeof(thread_payloads[0]);
   const size_t thread_count = sizeof(thread_payloads)/sizeof(thread_payloads[0]);


Why call abort() if an error occurs? Does exit(EXIT_FAILURE) not suffice?

The looping around write() in signal_handler() should be a separate function.

Avoid using magic numbers:

static char shared_string[512];

We need a defined constant for 512.

Avoid cluttering the code with redundant casts:

// struct ThreadPayload* args = (struct ThreadPayload*)tpl;
   struct ThreadPayload* args = tpl;

// return (void*)ret;
   return ret;

Reduce scope:

/*
size_t started_threads;
    for (started_threads = 0; started_threads < thread_count; ++started_threads) {
*/

for (size_t started_threads ...)

Avoid ignoring compiler warnings:

for (int i = 0; i < started_threads; ++i) {

The type of started_threads is size_t. There must have been a warning along the lines of:

Warning: Comparison between signed and unsigned or so.

If the compiler didn't complain, enable more warnings.

In func_that_takes_params(), check the return value of write().

\$\endgroup\$
4
  • \$\begingroup\$ The first issue looks the most interesting--what is the alternative way to handle the error on write()? \$\endgroup\$ May 2 at 12:59
  • \$\begingroup\$ @D.J.Elkind You can also write your own re-entrancy safe log utils. \$\endgroup\$
    – Harith
    May 2 at 13:22
  • \$\begingroup\$ my version will be using write() to log errors that occur to write() lol? \$\endgroup\$ May 2 at 14:05
  • \$\begingroup\$ Indeed, so there's no guarantee that a further call to write() would succeed. I wouldn't bother about reporting an error to stderr. \$\endgroup\$
    – Harith
    May 2 at 14:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.