Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C Programming > Request for code review.

Reply
Thread Tools

Request for code review.

 
 
K.M Jr
Guest
Posts: n/a
 
      10-14-2004
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 */
 
Reply With Quote
 
 
 
 
Richard Bos
Guest
Posts: n/a
 
      10-14-2004
http://www.velocityreviews.com/forums/(E-Mail Removed) (K.M Jr) wrote:

> 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
 
Reply With Quote
 
 
 
 
Dave Thompson
Guest
Posts: n/a
 
      10-21-2004
On Thu, 14 Oct 2004 09:55:09 GMT, (E-Mail Removed) (Richard
Bos) wrote:

> (E-Mail Removed) (K.M Jr) wrote:

<snip>
> > ip_len = delim_pos - application_addr;

<snip>
> > 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.

> > /* 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.
>

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

> > /* 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!".
>

which assists in checking for this.

- David.Thompson1 at worldnet.att.net
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Just got two emails: 1. your request for a new Access Code and 2. Request for my MCP ID# What's going on??!! belfast-biker Microsoft Certification 0 01-14-2006 12:49 PM
Re: Accessing Request.InputStream / Request.BinaryRead *as the request is occuring*: How??? Brian Birtle ASP .Net 2 10-16-2003 02:11 PM
best way to get data: request.form, request.params, controlname.value Christian H ASP .Net 1 07-29-2003 05:27 AM
Re: difference bet. request.querystring and Request.Params Daniel Bass ASP .Net 2 07-04-2003 12:12 PM
System.Web.HttpException: Request timed out - [HttpException (0x80004005): Request timed out.] Steve ASP .Net 0 07-01-2003 12:11 AM



Advertisments