Sufficient error checking of strtol()?

[William Payne]

Hello, I am in the process of converting a C++ program to a C program. The
user of the program is supposed to supply an integer on the command line and
in the C++ version of the program I was using something called stringstreams
to do the conversion. Here's my C version, can I leave it as it is or does
it need to be robustified or changed in any manner, regarding error
checking?

char* endptr;

errno = 0;

unsigned long port_number = strtol(argv[2], &endptr, 0);

if(strcmp("", endptr) != 0)
{
fprintf(stderr, "Couldn't convert %s to a valid port number.\n",
argv[2]);

return EXIT_FAILURE;
}

if(errno == ERANGE)
{
fprintf(stderr,
"Value %s is too big or small to fit in an unsigned long.\n",
argv[2]);

return EXIT_FAILURE;
}

Thanks for any replies!

/ William Payne

 

[Thomas Pfaff]

char* endptr;
errno = 0;

Make sure argv[2] is valid and non-NULL

if (argc > 2)

unsigned long port_number = strtol(argv[2], &endptr, 0);

strtol returns a long. Use strtoul for unsigned long.

if(strcmp("", endptr) != 0)

if (*endptr != '\0')

{
fprintf(stderr, "Couldn't convert %s to a valid port number.\n",
argv[2]);

[...]

The rest looks fine to me.

 

[nrk]

Hello, I am in the process of converting a C++ program to a C program. The
user of the program is supposed to supply an integer on the command line
and in the C++ version of the program I was using something called
stringstreams to do the conversion. Here's my C version, can I leave it as
it is or does it need to be robustified or changed in any manner,
regarding error checking?

char* endptr;

errno = 0;

unsigned long port_number = strtol(argv[2], &endptr, 0);

if(strcmp("", endptr) != 0)
{
fprintf(stderr, "Couldn't convert %s to a valid port number.\n",
argv[2]);

return EXIT_FAILURE;
}

Using strcmp here is overkill. I would prefer:
if ( endptr == argv[2] || *endptr )

The first condition above checks for the case of the empty string (OT: I
can't come up with a way of passing in empty strings from my shell (bash)
[note that exec* calls are excluded from this discussion]).

if(errno == ERANGE)
{
fprintf(stderr,
"Value %s is too big or small to fit in an unsigned
long.\n", argv[2]);

Probably would use strerror to get a localized error message and then create
my custom one from that.

-nrk.

 

[Eric Sosman]

Hello, I am in the process of converting a C++ program to a C program. The
user of the program is supposed to supply an integer on the command line and
in the C++ version of the program I was using something called stringstreams
to do the conversion. Here's my C version, can I leave it as it is or does
it need to be robustified or changed in any manner, regarding error
checking?

char* endptr;

errno = 0;

unsigned long port_number = strtol(argv[2], &endptr, 0);

Since you want an `unsigned long' result, you should
probably use strtoul() instead of strtol(). Otherwise, you'll
get possibly unwelcome behavior for input like "-3".

Also, give careful consideration to the third argument. As
you've written it, an input like "010" will produce the value eight
rather than ten. If that's what you want, fine -- but if you think
your users may be more familiar with decimal numbers than with C's
source conventions for octal, you might want to change it.

if(strcmp("", endptr) != 0)
{
fprintf(stderr, "Couldn't convert %s to a valid port number.\n",
argv[2]);

return EXIT_FAILURE;
}

You've got a design decision here: Should you allow trailing
whitespace? Note that you've implicitly allowed leading whitespace
and signs by using strto*().

If you choose to treat all "leftovers" as errors, I'd suggest
testing `if (*endptr != '\0')' instead of calling strcmp().

You might also want to do some usage-specific range checking.
For example, there might be reason to reject 1073741823 or zero
as valid "port numbers."

if(errno == ERANGE)
{
fprintf(stderr,
"Value %s is too big or small to fit in an unsigned long.\n",
argv[2]);

return EXIT_FAILURE;
}

Thanks for any replies!

It's usually recommended to clear zero `errno' before the
call, just in case it happened to be holding the value `ERANGE'
beforehand.

Putting all this together, I'd suggest something like

errno = 0;
port_number = strtoul(argv[2], &endptr, 10);
if (endptr == argv[2])
...; /* no conversion at all */
else if (*endptr != '\0')
...; /* incomplete conversion */
else if (errno == ERANGE)
...; /* out of `unsigned long' range */
else if (port_number < lowest || port_number > highest)
...; /* valid `unsigned long' but invalid port */
else
...; /* whoopee! */

Of course, you could collapse some of these cases if you don't
need to discriminate between them. Also, you could check
`isdigit( (unsigned char) argv[2][0] )' if you want to reject
leading whitespace and/or signs.

 

[William Payne]

char* endptr;
errno = 0;

Make sure argv[2] is valid and non-NULL

if (argc > 2)

unsigned long port_number = strtol(argv[2], &endptr, 0);

strtol returns a long. Use strtoul for unsigned long.

if(strcmp("", endptr) != 0)

if (*endptr != '\0')

{
fprintf(stderr, "Couldn't convert %s to a valid port number.\n",
argv[2]);

[...]

The rest looks fine to me.

Thanks for your reply Thomas. I already had an argc check in my program, but
I forgot to mention it in the post. The user has to supply two arguments in
fact, a hostname and a port number so the first thing I do is check if argc
< 3 and if it's less than three I exit with an error message. Do I have to
check something else before passing argv[2] to strtoul()? The
strtol()/strtoul() confusion arises from the fact that I realised when
writing my original post that only non-negative port numbers are valid in my
program so I changed from strtol() to strtoul(). I will get rid of the
strcmp() and do what you suggested instead.

// WP

 

[AJ Mohan Rao]

Hello, I am in the process of converting a C++ program to a C program.
The
user of the program is supposed to supply an integer on the command line
and in the C++ version of the program I was using something called
stringstreams to do the conversion. Here's my C version, can I leave it
as
it is or does it need to be robustified or changed in any manner,
regarding error checking?

char* endptr;

errno = 0;

unsigned long port_number = strtol(argv[2], &endptr, 0);

if(strcmp("", endptr) != 0)
{
fprintf(stderr, "Couldn't convert %s to a valid port number.\n",
argv[2]);

return EXIT_FAILURE;
}

Using strcmp here is overkill. I would prefer:
if ( endptr == argv[2] || *endptr )

The first condition above checks for the case of the empty string (OT: I
can't come up with a way of passing in empty strings from my shell (bash)

$ ./a.out ""

(*argv[1] == 0) is true