Request for code review.

[K.M Jr]

Hi group,

I usually don't code in C. I happened to write a piece of code that
parses an input string of the form "host_name:XXXX" into host_name and
XXXX. I would request the group to give me any tips, mistakes I have
done, good coding practices etc.

Thanks

PS - I know that goto is not the recommended way of doing things, so
forgive that part for now.

/*
* Function to parse host name and port address.
* appplication_addr - pointer to string to parse, format of string
is "hostname:XXXX"
* ip_buffer - pointer to buffer to hold the host name after parsing
* ip_buffer_len - length of ip_buffer
* port - pointer to integer to hold the port number after parsing
*/

int parse_application_address (const char *application_addr, char
*ip_buffer, int ip_buffer_len, int *port)
{

int ip_len;
int retval = RET_ERR;
const char *delim_pos = strchr(application_addr, ':');

if (delim_pos != NULL)
{
ip_len = delim_pos - application_addr;
if (ip_buffer_len < ip_len + 1)
{
log("Insuffcient buffer space\n", ip_buffer_len);
goto end;
}
strncpy(ip_buffer, application_addr, ip_len);
ip_buffer[ip_len] = 0;
/* TODO - Validate IP */
*port = atoi(delim_pos + 1);
/* TODO - Validate port range */
retval = RET_OK;
}
else
{
log("Invalid application address - %s\n", application_addr);
}
end:
return retval;
}

/* RET_OK and RET_ERR are #defines for 0 and -1 respectively */

 

[Richard Bos]

PS - I know that goto is not the recommended way of doing things, so
forgive that part for now.

goto is fine if used sparingly and correctly, which means it will mainly
be used for error conditions. As you've used it, I see no problem with
it, except that the function as it is is maybe a bit to small not to use
an else clause instead.

/*
* Function to parse host name and port address.
* appplication_addr - pointer to string to parse, format of string
is "hostname:XXXX"
* ip_buffer - pointer to buffer to hold the host name after parsing
* ip_buffer_len - length of ip_buffer
* port - pointer to integer to hold the port number after parsing
*/

int parse_application_address (const char *application_addr, char
*ip_buffer, int ip_buffer_len, int *port)
{

int ip_len;
int retval = RET_ERR;
const char *delim_pos = strchr(application_addr, ':');

if (delim_pos != NULL)
{
ip_len = delim_pos - application_addr;
if (ip_buffer_len < ip_len + 1)
{
log("Insuffcient buffer space\n", ip_buffer_len);

This log message could be more specific (unless, perhaps, log() is a
macro which uses __FILE__ and __LINE__, in which case I'd prefer to call
it LOG); and you've misspelled "Insufficient". Also, it seems later on
that log() expects printf()-like arguments, in which case you're missing
a conversion specifier for ip_buffer_len.

goto end;
}
strncpy(ip_buffer, application_addr, ip_len);
ip_buffer[ip_len] = 0;

Since IP addresses are never longer than fifteen characters, and won't
be thousands of bytes even with IPv6, this isn't much of a problem, but
do note that strncpy() pads its output with '\0's. This can be a waste
of cycles.

/* TODO - Validate IP */
*port = atoi(delim_pos + 1);

strtol() and (even better, in this case) strtoul() are safer, since they
won't cause undefined behaviour on overflow, as atoi() can; this means
that atoi() is vulnerable to malicious inputs, while strto*() aren't.

/* TODO - Validate port range */

Don't forget to check that someone doesn't pass something like
"127.0.0.1:not-a-number", or "127.0.0.1:123bang!".

retval = RET_OK;
}
else
{
log("Invalid application address - %s\n", application_addr);
}
end:
return retval;
}

/* RET_OK and RET_ERR are #defines for 0 and -1 respectively */

Congratulations. You write better C than many people who _do_ usually do
so.

Richard

 

[Dave Thompson]

(e-mail address removed) (K.M Jr) wrote:

ip_len = delim_pos - application_addr;

strncpy(ip_buffer, application_addr, ip_len);
ip_buffer[ip_len] = 0;

Since IP addresses are never longer than fifteen characters, and won't
be thousands of bytes even with IPv6, this isn't much of a problem, but
do note that strncpy() pads its output with '\0's. This can be a waste
of cycles.

In general, but in this code ip_len has been computed to be less than
the source string length; in this case strncpy() doesn't pad, or even
null-terminate, so the code correctly does so explicitly. memcpy()
would be just as good in this usage, and IMO slightly clearer.

strtol() and (even better, in this case) strtoul() are safer, since they
won't cause undefined behaviour on overflow, as atoi() can; this means
that atoi() is vulnerable to malicious inputs, while strto*() aren't.

Plus strto* can give you the end-pointer ...

Don't forget to check that someone doesn't pass something like
"127.0.0.1:not-a-number", or "127.0.0.1:123bang!".

which assists in checking for this.

- David.Thompson1 at worldnet.att.net