Request for code review.

Discussion in 'C Programming' started by K.M Jr, Oct 14, 2004.

  1. K.M Jr

    K.M Jr Guest

    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 */
     
    K.M Jr, Oct 14, 2004
    #1
    1. Advertising

  2. K.M Jr

    Richard Bos Guest

    (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
     
    Richard Bos, Oct 14, 2004
    #2
    1. Advertising

  3. On Thu, 14 Oct 2004 09:55:09 GMT, (Richard
    Bos) wrote:

    > (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
     
    Dave Thompson, Oct 21, 2004
    #3
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Code Review Request

    , Jun 30, 2005, in forum: Java
    Replies:
    2
    Views:
    534
    Andrew Thompson
    Jun 30, 2005
  2. P Kenter

    Request for code review

    P Kenter, May 28, 2004, in forum: C++
    Replies:
    4
    Views:
    368
    P Kenter
    Jun 2, 2004
  3. Ben Hanson

    Request for Code Review

    Ben Hanson, Jul 2, 2004, in forum: C++
    Replies:
    19
    Views:
    679
    Chris Gordon-Smith
    Jul 4, 2004
  4. Debashish Chakravarty

    request for code review

    Debashish Chakravarty, Nov 18, 2003, in forum: C Programming
    Replies:
    3
    Views:
    398
    Robert Stankowic
    Nov 18, 2003
  5. www
    Replies:
    51
    Views:
    1,501
Loading...

Share This Page