[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r993183 - in /subversion/trunk/subversion: include/ libsvn_diff/ libsvn_ra_serf/ libsvn_subr/ mod_dav_svn/reports/

From: Greg Stein <gstein_at_gmail.com>
Date: Tue, 7 Sep 2010 00:42:17 -0400

The buildbots are failing all over the place after this revision...

On Mon, Sep 6, 2010 at 20:09, <stsp_at_apache.org> wrote:
> Author: stsp
> Date: Tue Sep  7 00:09:46 2010
> New Revision: 993183
>
> URL: http://svn.apache.org/viewvc?rev=993183&view=rev
> Log:
> Introduce a new family of functions to parse numbers from strings.
>
> The goal is to replace direct calls to functions like atoi() and apr_atoi64(),
> which have very inconvenient error handling to the point where a lot of
> our code simply ignores conversion errors and continues operating with
> the converted data in potentially undefined state.
> The new functions, on the other hand, report conversion errors via the
> standard Subversion error handling interface, thereby not allowing callers
> to ignore conversion errors.
>
> This commit also switches selected pieces of code over to the new functions.
> More conversion to come.
>
> * subversion/include/svn_string.h
>  (svn_cstring_strtoi64, svn_cstring_atoi64, svn_cstring_atoi,
>   svn_cstring_strtoui64, svn_cstring_atoui64, svn_cstring_atoui): Declare.
>
> * subversion/libsvn_subr/svn_string.c
>  (): Include svn_private_config.h for the _() gettext macro.
>  (svn_cstring_strtoi64, svn_cstring_strtoui64, svn_cstring_atoi64,
>   svn_cstring_atoi): New.
>
> * subversion/libsvn_ra_serf/serf.c
>  (dirent_walker): Use svn_cstring_atoi64() instead of apr_atoi64().
>
> * subversion/libsvn_diff/parse-diff.c
>  (parse_offset): Call svn_cstring_strtoui64() instead of calling
>   apr_atoi64() and performing manual overflow checking.
>
> * subversion/mod_dav_svn/reports/log.c
>  (dav_svn__log_report): Use svn_cstring_atoi() instead of atoi() for
>   parsing CDATA of the "limit" element.
>
> * subversion/mod_dav_svn/reports/replay.c
>  (dav_svn__replay_report): Use svn_cstring_strtoi64() instead of atoi() for
>   parsing CDATA of the "send-deltas" element.
>
> * subversion/libsvn_subr/win32_xlate.c
>  (get_page_id_from_name): Use svn_cstring_atoui() instead of atoi() to parse
>   the number of a codepage.
>
> * subversion/libsvn_subr/io.c
>  (svn_io_read_version_file): Use svn_cstring_atoi() instead of atoi().
>
> * subversion/libsvn_subr/hash.c
>  (svn_hash_read): Use svn_cstring_atoi() instead of atoi().
>
> Modified:
>    subversion/trunk/subversion/include/svn_string.h
>    subversion/trunk/subversion/libsvn_diff/parse-diff.c
>    subversion/trunk/subversion/libsvn_ra_serf/serf.c
>    subversion/trunk/subversion/libsvn_subr/hash.c
>    subversion/trunk/subversion/libsvn_subr/io.c
>    subversion/trunk/subversion/libsvn_subr/svn_string.c
>    subversion/trunk/subversion/libsvn_subr/win32_xlate.c
>    subversion/trunk/subversion/mod_dav_svn/reports/log.c
>    subversion/trunk/subversion/mod_dav_svn/reports/replay.c
>
> Modified: subversion/trunk/subversion/include/svn_string.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_string.h?rev=993183&r1=993182&r2=993183&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_string.h (original)
> +++ subversion/trunk/subversion/include/svn_string.h Tue Sep  7 00:09:46 2010
> @@ -388,6 +388,71 @@ svn_cstring_join(const apr_array_header_
>  int
>  svn_cstring_casecmp(const char *str1, const char *str2);
>
> +/**
> + * Parse the C string @a str into a 64 bit number, and return it in @a *n.
> + * Assume that the number is represented in base @a base.
> + * Raise an error if conversion fails (e.g. due to overflow), or if the
> + * converted number is smaller than @a minval or larger than @a maxval.
> + *
> + * @since New in 1.7.
> + */
> +svn_error_t *
> +svn_cstring_strtoi64(apr_int64_t *n, const char *str,
> +                     apr_int64_t minval, apr_int64_t maxval,
> +                     int base);
> +
> +/**
> + * Parse the C string @a str into a 64 bit number, and return it in @a *n.
> + * Assume that the number is represented in base 10.
> + * Raise an error if conversion fails (e.g. due to overflow).
> + *
> + * @since New in 1.7.
> + */
> +svn_error_t *
> +svn_cstring_atoi64(apr_int64_t *n, const char *str);
> +
> +/**
> + * Parse the C string @a str into a 32 bit number, and return it in @a *n.
> + * Assume that the number is represented in base 10.
> + * Raise an error if conversion fails (e.g. due to overflow).
> + *
> + * @since New in 1.7.
> + */
> +svn_error_t *
> +svn_cstring_atoi(int *n, const char *str);
> +
> +/**
> + * Parse the C string @a str into an unsigned 64 bit number, and return
> + * it in @a *n. Assume that the number is represented in base @a base.
> + * Raise an error if conversion fails (e.g. due to overflow), or if the
> + * converted number is smaller than @a minval or larger than @a maxval.
> + *
> + * @since New in 1.7.
> + */
> +svn_error_t *
> +svn_cstring_strtoui64(apr_uint64_t *n, const char *str,
> +                      apr_uint64_t minval, apr_uint64_t maxval,
> +                      int base);
> +
> +/**
> + * Parse the C string @a str into an unsigned 64 bit number, and return
> + * it in @a *n. Assume that the number is represented in base 10.
> + * Raise an error if conversion fails (e.g. due to overflow).
> + *
> + * @since New in 1.7.
> + */
> +svn_error_t *
> +svn_cstring_atoui64(apr_uint64_t *n, const char *str);
> +
> +/**
> + * Parse the C string @a str into an unsigned 32 bit number, and return
> + * it in @a *n. Assume that the number is represented in base 10.
> + * Raise an error if conversion fails (e.g. due to overflow).
> + *
> + * @since New in 1.7.
> + */
> +svn_error_t *
> +svn_cstring_atoui(unsigned int *n, const char *str);
>
>  /** @} */
>
>
> Modified: subversion/trunk/subversion/libsvn_diff/parse-diff.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_diff/parse-diff.c?rev=993183&r1=993182&r2=993183&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_diff/parse-diff.c (original)
> +++ subversion/trunk/subversion/libsvn_diff/parse-diff.c Tue Sep  7 00:09:46 2010
> @@ -28,6 +28,7 @@
>  #include "svn_error.h"
>  #include "svn_io.h"
>  #include "svn_pools.h"
> +#include "svn_string.h"
>  #include "svn_utf.h"
>  #include "svn_dirent_uri.h"
>  #include "svn_diff.h"
> @@ -119,20 +120,15 @@ svn_diff_hunk_get_trailing_context(const
>  static svn_boolean_t
>  parse_offset(svn_linenum_t *offset, const char *number)
>  {
> -  apr_int64_t parsed_offset;
> +  svn_error_t *err;
>
> -  errno = 0; /* apr_atoi64() in APR-0.9 does not always set errno */
> -  parsed_offset = apr_atoi64(number);
> -  if (errno == ERANGE || parsed_offset < 0)
> -    return FALSE;
> -
> -  /* In case we cannot fit 64 bits into an svn_linenum_t,
> -   * check for overflow. */
> -  if (sizeof(svn_linenum_t) < sizeof(parsed_offset) &&
> -      parsed_offset > SVN_LINENUM_MAX_VALUE)
> -    return FALSE;
> -
> -  *offset = parsed_offset;
> +  err = svn_cstring_strtoui64(offset, number, 0, SVN_LINENUM_MAX_VALUE, 10);
> +  if (err)
> +    {
> +      svn_error_clear(err);
> +      return FALSE;
> +    }
> +
>   return TRUE;
>  }
>
>
> Modified: subversion/trunk/subversion/libsvn_ra_serf/serf.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/serf.c?rev=993183&r1=993182&r2=993183&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_serf/serf.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_serf/serf.c Tue Sep  7 00:09:46 2010
> @@ -721,7 +721,7 @@ dirent_walker(void *baton,
>         }
>       else if (strcmp(name, "getcontentlength") == 0)
>         {
> -          entry->size = apr_atoi64(val->data);
> +          SVN_ERR(svn_cstring_atoi64(&entry->size, val->data));
>         }
>       else if (strcmp(name, "resourcetype") == 0)
>         {
>
> Modified: subversion/trunk/subversion/libsvn_subr/hash.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/hash.c?rev=993183&r1=993182&r2=993183&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/hash.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/hash.c Tue Sep  7 00:09:46 2010
> @@ -344,11 +344,16 @@ svn_hash_read(apr_hash_t *hash,
>         }
>       else if ((buf[0] == 'K') && (buf[1] == ' '))
>         {
> +          size_t keylen;
> +          int parsed_len;
> +          void *keybuf;
> +
>           /* Get the length of the key */
> -          size_t keylen = (size_t) atoi(buf + 2);
> +          SVN_ERR(svn_cstring_atoi(&parsed_len, buf + 2));
> +          keylen = parsed_len;
>
>           /* Now read that much into a buffer, + 1 byte for null terminator */
> -          void *keybuf = apr_palloc(pool, keylen + 1);
> +          keybuf = apr_palloc(pool, keylen + 1);
>           SVN_ERR(svn_io_file_read_full(srcfile,
>                                         keybuf, keylen, &num_read, pool));
>           ((char *) keybuf)[keylen] = '\0';
> @@ -365,12 +370,15 @@ svn_hash_read(apr_hash_t *hash,
>           if ((buf[0] == 'V') && (buf[1] == ' '))
>             {
>               svn_string_t *value = apr_palloc(pool, sizeof(*value));
> +              apr_size_t vallen;
> +              void *valbuf;
>
>               /* Get the length of the value */
> -              apr_size_t vallen = atoi(buf + 2);
> +              SVN_ERR(svn_cstring_atoi(&parsed_len, buf + 2));
> +              vallen = parsed_len;
>
>               /* Again, 1 extra byte for the null termination. */
> -              void *valbuf = apr_palloc(pool, vallen + 1);
> +              valbuf = apr_palloc(pool, vallen + 1);
>               SVN_ERR(svn_io_file_read_full(srcfile,
>                                             valbuf, vallen,
>                                             &num_read, pool));
>
> Modified: subversion/trunk/subversion/libsvn_subr/io.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/io.c?rev=993183&r1=993182&r2=993183&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/io.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/io.c Tue Sep  7 00:09:46 2010
> @@ -3472,7 +3472,7 @@ svn_io_read_version_file(int *version,
>   }
>
>   /* Convert to integer. */
> -  *version = atoi(buf);
> +  SVN_ERR(svn_cstring_atoi(version, buf));
>
>   return SVN_NO_ERROR;
>  }
>
> Modified: subversion/trunk/subversion/libsvn_subr/svn_string.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/svn_string.c?rev=993183&r1=993182&r2=993183&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/svn_string.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/svn_string.c Tue Sep  7 00:09:46 2010
> @@ -25,12 +25,15 @@
>
>
>
> +#include <apr.h>
> +
>  #include <string.h>      /* for memcpy(), memcmp(), strlen() */
>  #include <apr_lib.h>     /* for apr_isspace() */
>  #include <apr_fnmatch.h>
>  #include "svn_string.h"  /* loads "svn_types.h" and <apr_pools.h> */
>  #include "svn_ctype.h"
>
> +#include "svn_private_config.h"
>
>
>  /* Our own realloc, since APR doesn't have one.  Note: this is a
> @@ -605,3 +608,88 @@ svn_cstring_casecmp(const char *str1, co
>         return cmp;
>     }
>  }
> +
> +svn_error_t *
> +svn_cstring_strtoui64(apr_uint64_t *n, const char *str,
> +                      apr_uint64_t minval, apr_uint64_t maxval,
> +                      int base)
> +{
> +  apr_int64_t parsed;
> +
> +  /* We assume errno is thread-safe. */
> +  errno = 0; /* APR-0.9 doesn't always set errno */
> +
> +  /* ### We're throwing away half the number range here.
> +   * ### Maybe implement our own number parser? */
> +  parsed = apr_strtoi64(str, NULL, base);
> +  if (errno != 0)
> +    return svn_error_return(
> +             svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
> +                               _("Could not convert '%s' into a number"),
> +                               str));
> +  if (parsed < 0 || parsed < minval || parsed > maxval)
> +    return svn_error_return(
> +             svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
> +                               _("Number '%s' is out of range"), str));
> +  *n = parsed;
> +  return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_cstring_atoui64(apr_uint64_t *n, const char *str)
> +{
> +  return svn_error_return(svn_cstring_strtoui64(n, str, 0,
> +                                                APR_UINT64_MAX, 10));
> +}
> +
> +svn_error_t *
> +svn_cstring_atoui(unsigned int *n, const char *str)
> +{
> +  apr_uint64_t parsed;
> +
> +  SVN_ERR(svn_cstring_strtoui64(&parsed, str, 0, APR_UINT32_MAX, 10));
> +  *n = (unsigned int)parsed;
> +  return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_cstring_strtoi64(apr_int64_t *n, const char *str,
> +                     apr_int64_t minval, apr_int64_t maxval,
> +                     int base)
> +{
> +  apr_int64_t parsed;
> +
> +  /* We assume errno is thread-safe. */
> +  errno = 0; /* APR-0.9 doesn't always set errno */
> +
> +  parsed = apr_strtoi64(str, NULL, base);
> +  if (errno != 0)
> +    return svn_error_return(
> +             svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
> +                               _("Could not convert '%s' into a number"),
> +                               str));
> +  if (parsed < minval || parsed > maxval)
> +    return svn_error_return(
> +             svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
> +                               _("Number '%s' is out of range"), str));
> +  *n = parsed;
> +  return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_cstring_atoi64(apr_int64_t *n, const char *str)
> +{
> +  return svn_error_return(svn_cstring_strtoi64(n, str, APR_INT64_MIN,
> +                                               APR_INT64_MAX, 10));
> +}
> +
> +svn_error_t *
> +svn_cstring_atoi(int *n, const char *str)
> +{
> +  apr_int64_t parsed;
> +
> +  SVN_ERR(svn_cstring_strtoi64(&parsed, str, APR_INT32_MIN,
> +                               APR_INT32_MAX, 10));
> +  *n = (int)parsed;
> +  return SVN_NO_ERROR;
> +}
>
> Modified: subversion/trunk/subversion/libsvn_subr/win32_xlate.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/win32_xlate.c?rev=993183&r1=993182&r2=993183&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/win32_xlate.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/win32_xlate.c Tue Sep  7 00:09:46 2010
> @@ -110,7 +110,7 @@ get_page_id_from_name(UINT *page_id_p, c
>   if ((page_name[0] == 'c' || page_name[0] == 'C')
>       && (page_name[1] == 'p' || page_name[1] == 'P'))
>     {
> -      *page_id_p = atoi(page_name + 2);
> +      SVN_ERR(svn_cstring_atoui(page_id_p, page_name + 2));
>       return APR_SUCCESS;
>     }
>
>
> Modified: subversion/trunk/subversion/mod_dav_svn/reports/log.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/reports/log.c?rev=993183&r1=993182&r2=993183&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/mod_dav_svn/reports/log.c (original)
> +++ subversion/trunk/subversion/mod_dav_svn/reports/log.c Tue Sep  7 00:09:46 2010
> @@ -311,7 +311,15 @@ dav_svn__log_report(const dav_resource *
>       else if (strcmp(child->name, "end-revision") == 0)
>         end = SVN_STR_TO_REV(dav_xml_get_cdata(child, resource->pool, 1));
>       else if (strcmp(child->name, "limit") == 0)
> -        limit = atoi(dav_xml_get_cdata(child, resource->pool, 1));
> +        {
> +          serr = svn_cstring_atoi(&limit,
> +                                  dav_xml_get_cdata(child, resource->pool, 1));
> +          if (serr)
> +            {
> +              svn_error_clear(serr);
> +              return malformed_element_error("limit", resource->pool);
> +            }
> +        }
>       else if (strcmp(child->name, "discover-changed-paths") == 0)
>         discover_changed_paths = TRUE; /* presence indicates positivity */
>       else if (strcmp(child->name, "strict-node-history") == 0)
>
> Modified: subversion/trunk/subversion/mod_dav_svn/reports/replay.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/reports/replay.c?rev=993183&r1=993182&r2=993183&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/mod_dav_svn/reports/replay.c (original)
> +++ subversion/trunk/subversion/mod_dav_svn/reports/replay.c Tue Sep  7 00:09:46 2010
> @@ -466,10 +466,18 @@ dav_svn__replay_report(const dav_resourc
>             }
>           else if (strcmp(child->name, "send-deltas") == 0)
>             {
> +              apr_int64_t parsed_val;
> +
>               cdata = dav_xml_get_cdata(child, resource->pool, 1);
>               if (! cdata)
>                 return malformed_element_error("send-deltas", resource->pool);
> -              send_deltas = atoi(cdata);
> +              err = svn_cstring_strtoi64(&parsed_val, cdata, 0, 1, 10);
> +              if (err)
> +                {
> +                  svn_error_clear(err);
> +                  return malformed_element_error("send-deltas", resource->pool);
> +                }
> +              send_deltas = parsed_val ? TRUE : FALSE;
>             }
>         }
>     }
>
>
>
Received on 2010-09-07 06:43:17 CEST

This is an archived mail posted to the Subversion Dev mailing list.