Return to Revisions

Check the return value of library functions

From the man page:

signal() returns the previous value of the signal handler. On failure, it returns SIG_ERR, and errno is set to indicate the error.

Neither of the calls to signal() are checked in your application. The calls to clock_gettime(), system() read(), mkdir(), and fgets() et cetera also go unchecked.

Note that there is another way of setting the terminal back to cooked mode instead of calling system().

Calling async-signal-unsafe functions in signal handlers is undefined behavior

fflush(), fprintf(), printf(), exit(), and system() are not safe to call in a signal handler according to both the ISO C and POSIX standards. See: signal-safety(7) — Linux manual page.

The behavior is also undefined if the signal handler refers to any object other than errno with static storage duration other than by assigning a value to an object declared as volatile sig_atomic_t. The variable paused is of type int.

argv[0] can be NULL

program_name = argv[0];

This is risky. One can easily pass in argv[0] as a null pointer with an exec() syscall. Add a check for it, else a subsequent null pointer dereference would invoke undefined behavior.

Use the bool type to denote a binary state

case 's':
  sflag = 1;
  break;

case 'r':
  rflag = 1;
  break;

Include stdbool.h for bool, true, and false. This is not required in C2X, as they are keywords.

#define C2X_PLACEHOLDER 202000L

#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= C2X_PLACEHOLDER
    /* Coast is clear. */
#else
    #include <stdbool.h>            /* bool, true, and false. */
#endif

You could also make a struct containing all these flags and pass it around as an argument instead of making all the flags global. I'd also suggest moving this input parsing to a separate function. Furthermore, consider using EXIT_FAILURE and EXIT_SUCCESS from stdlib.h instead of non-standard exit codes.

In main(), exit(EXIT_SUCCESS) is equivalent to return EXIT_SUCCESS

I suggest eliding the calls to exit().

The call to strcat() might write to out-of-bounds memory

strncpy(file, getenv("HOME"), 255);
file[255] = '\0';
strcat(file, "/.sw");

You failed to verify whether the string returned by getenv() was less than 255 characters. The subsequent calls to strcat() have the same problem. You're inviting a buffer overflow attack. I suggest using strncat(), or using one of the better alternatives to strcat()/strncat() that POSIX might provide.

The subsequent call to strncmp():

else if (strncmp(mode, "w", 1) == 0

can simply be:

else if (mode[0] == 'w')

You also don't need separate invocations of fopen() in each branch. Move the call to the last line of the function and eliminate the unused variable (return fopen(...)).

Prior to C2X, a function with empty parentheses as the argument-list specifies an argument that takes a variadic number of arguments, not zero

Specify void wherever you are using empty parentheses.

I also do not see what the point of stopwatch.h is. I'd just elide it.

Consecutive string literals are concatenated

You can replace the 16 calls to fprintf() in the help message with:

fprintf(out, "\nOptions:\n"
             "  -h, --help    Show this help message and exit.\n"
             "  -s, --save    Save the final time to ~/.sw/savedtime\n"
             "  -r, --restore Restore time from ~/.sw/savedtime\n"
             "  -x, --exit    Exit instead of pausing.\n"
             "  -p, --paused  Start in paused state.\n"
             "  -a, --anykey  Exit upon any keypress. With -p, will exit upon any keypress after unpausing.\n"
             "\nControls:\n"
             "  Space         Pause or resume the stopwatch.\n"
             "  s             Save the current time to ~/.sw/savedtime.\n"
             "  +             Add one second to the time.\n"
             "  -             Subtract one second from the time.\n"
             "  r             Reset the stopwatch to zero.\n"
             "  q             Quit.\n");

The identifier fd is conventionally used for a file descriptor, and not a FILE *

Consider using fp or stream. Additionally, the arguments that are not modified in the function should be const-qualified. If you're using C99 or higher, you should also be using the ptr[static 1] notation for pointers that are expected to be non-null.

Do not use dynamic allocation when a fixed-size array would suffice

In here:

int bufsize = 32;
char *buf = malloc(bufsize * sizeof(char));
if (buf == NULL) {
  perror("Could not allocate memory to restore previous time.\n");
}
memset(buf, 0, bufsize);

The size of the buffer is already known, and there's no reason to call malloc() for it. Simply do:

#define BUFSIZE 32
char buf[BUFSIZE];

To zero-initialize it, which I don't believe has any value here:

char buf[BUFSIZE] = {0};

// Or in C2X, like C++:
char buf[BUFSIZE] = {};

And malloc()+memset() should really be calloc().

Also note that sizeof(char) is defined by the standard to be 1. So it can be safely elided.

Additionally, use an extra variable to avoid the two calls to strlen().