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, <s...@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
