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. system() and read() also return something meaningful.

Neither of the calls to signal() are checked in your application. The calls to clock_gettime(), system() read(), mkdir() 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. 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 use 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");