patterncMinor
Unix 'sleep' clone - unsure of safety
Viewed 0 times
unixsleepunsuresafetyclone
Problem
I decided to try making some simple clones of the various user-space Unix utilities in an effort to learn the system-specific headers and improve my C skills. The code that follows is a clone of
The program attempts to comply with the Open Group spec for
sleep and I'm curious as to whether there are any pitfalls I'm overlooking (including security vulnerabilities). It compiles and seems to behave fine so there's no issue there.The program attempts to comply with the Open Group spec for
sleep.#include
#include
#include
#include
#include
#define MAX_TIME_DIGITS (11)
int main(int argc, char *argv[]) {
if(argc != 2) {
fprintf(stderr, "sleep: too %s operands\n",
argc MAX_TIME_DIGITS) {
fprintf(stderr, "sleep: operand too long\n");
return 1;
}
int i = 0;
while(argv[1][i] && i < MAX_TIME_DIGITS) {
if(!isdigit((int)argv[1][i])) {
fprintf(stderr, "sleep: invalid operand\n");
return 1;
}
++i;
}
unsigned long int time = strtoul(argv[1], NULL, 10);
sleep(time);
return 0;
}Solution
-
An error message ("too few"/"too many") is not helpful. Typically when an Unix utility cannot deal with the command line it prints what it expects:
For example,
It is also customary to provide a
In any case, do not hardcode a
-
Just let it do its job:
-
It would be nice to add an option to define a
An error message ("too few"/"too many") is not helpful. Typically when an Unix utility cannot deal with the command line it prints what it expects:
For example,
fprintf(stderr, " Usage: %s sleep_duration", argv[0])It is also customary to provide a
usage() function to print such message.In any case, do not hardcode a
sleep string. Use argv[0].-
strtoul has very good error handling capabilities. It is pointless to try to intercept problems it can detect.Just let it do its job:
char * stop;
unsigned long int time = strtoul(argv[1], &stop, 10);
if (*stop != 0) {
// strtoul failed to parse the argument. Now printing an error message is appropriate.
usage("...");
}-
It would be nice to add an option to define a
SIGALRM behaviour. OpenGroup does not specify which action shall be taken, meaning it is up to you to define it.Code Snippets
fprintf(stderr, " Usage: %s sleep_duration", argv[0])char * stop;
unsigned long int time = strtoul(argv[1], &stop, 10);
if (*stop != 0) {
// strtoul failed to parse the argument. Now printing an error message is appropriate.
usage("...");
}Context
StackExchange Code Review Q#58594, answer score: 4
Revisions (0)
No revisions yet.