2
\$\begingroup\$

I wrote a program that implements a POSIX signal API. It implements the functions sigset(), sighold(), sigrelse(), sigignore() and sigpause().

I would like you to criticize my implementation, to find any problems that may arise.

#include <signal.h>
#include <unistd.h>
#include <errno.h>
#include <stdio.h>

#define SIG_HOLD ((__sighandler_t)2)
typedef void (*sighandler_t)(int);

sighandler_t
sigset(int sig, sighandler_t disp);
int sighold(int sig);
int sigrelse(int sig);
int sigignore(int sig);
int sigpause(int sig);

sighandler_t
sigset(int sig, sighandler_t disp)
{
    struct sigaction old_set;
    if (disp == SIG_HOLD)
    {
        sighold(sig);

        sigaction(sig, NULL, &old_set);
        return old_set.sa_handler;
    }

    struct sigaction act;
    sigemptyset(&act.sa_mask);
    act.sa_flags = 0;
    act.sa_handler = disp;
    sigaction(sig, &act, &old_set);
    return old_set.sa_handler;
}

int sighold(int sig)
{
    sigset_t set;
    sigemptyset(&set);
    sigaddset(&set, sig);
    return sigprocmask(SIG_BLOCK, &set, NULL);
}

int sigrelse(int sig)
{
    sigset_t set;
    sigemptyset(&set);
    sigaddset(&set, sig);
    return sigprocmask(SIG_UNBLOCK, &set, NULL);
}

int sigignore(int sig)
{
    if (signal(sig, SIG_IGN) == SIG_ERR)
        return -1;
    return 0;
}

int sigpause(int sig)
{
    sigset_t old_set;
    if (sigprocmask(SIG_BLOCK, NULL, &old_set) == -1)
        return -1;
    sigdelset(&old_set, sig);
    return sigsuspend(&old_set);
}

void handler(int sig)
{
    printf("handler\n"); // it's unsafe
}

int main()
{
    sigset(SIGTERM, handler);
    sighold(SIGABRT);
    sigrelse(SIGABRT);
    sigignore(SIGABRT);
    sighold(SIGTERM);
    raise(SIGTERM);
    sigpause(SIGTERM);

    return 0;
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Unsafe definition of SIG_HOLD

There are several issues with your definition of SIG_HOLD. First, since you #include <signal.h>, consider that this macro might already be defined there. Use #ifndef SIG_HOLD to only define it if it doesn't already exist.

Second, there are other macros named SIG_*, which might have the same value as you are using for SIG_HOLD. To avoid any issues, I would recommend making sure your SIG_HOLD has a unique value. This is easy to do, just create a dummy function that you can take the address of:

#ifndef SIG_HOLD
void sig_hold_(int) {}
#define SIG_HOLD (sig_hold_)
#endif

Missing error checking

It is interesting that you check or pass on the return value of sigprocmask(), but you don't check the return value of most other sig*() functions. However, all of them return an error code that you should check.

Avoid using signal()

You are using both sigaction() and signal() in your code. While using signal(sig, SIG_IGN) might be one of the few uses of signal() that is safe, I would not use it and only rely on sigaction(). Note that you can just do:

int sigignore(int sig) {
    return sigset(sig, SIG_IGN);
}

Split it into a header file and a source file

You have everything in one file now, but you should try to put all these functions in a separate .c file, and create a matching .h file that can be #included in the source files that do want to use these functions. Also make sure you #include the header file in the .c file containing your functions; this avoids needing forward declarations, and can catch mistakes like the declarations in the header file not matching the definitions in the source file.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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