Hi Stefan,
APR is a moving target...
On Sep 7, 2010, at 6:42 , Greg Stein wrote:
> 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.
Uh oh, some buildbots complain about APR_UINT{32,64}_MAX being
undefined. It looks like they were first defined in APR 1.3.
>>
>> * 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/log.c:320: warning: implicit declaration of function ‘malformed_element_error’
I get just a warning, but this is an error on some buildbots.
FWIW, copy-pasting the static function from replay.c makes the
compiler happy.
>>
>> * 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;
>> }
>> }
>> }
>>
>>
>>
--
Stephen Butler | Senior Consultant
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Received on 2010-09-07 09:18:13 CEST