Re: [PATCH] remove svn__strtol() and svn__strtoul()

On 6 August 2014 19:11, Stefan Sperling <stsp_at_elego.de> wrote:
> I'd like to remove svn__strtol() and svn__strtoul().
>
> Let's not get performance and micro-optimisation in the way of error checking.
> Even if the input does not come from the network or cannot for any other
> reason be inherently malicious it might be erroneous due to other
> problems such as disk corruption. I don't believe it's OK to willingly
> parse revision files without such checks. I think it's irresponsible
> to do so and makes diagnosing difficult problems unnecessarily harder.
>
I'm big +1 one this: we should not trade error checking for
performance. Especially for micro-optimization.

See my comments on patch inline.
> [[[
> Remove svn__strtol() and svn__strtoul(). They were introduced as a
> performance enhancement but don't provide sufficient error checking.
>
[...]

>
> return TRUE;
> @@ -93,8 +102,13 @@ static svn_boolean_t
> txn_id_parse(svn_fs_fs__id_part_t *txn_id,
> const char *data)
> {
> - const char *end;
> - txn_id->revision = svn__strtol(data, &end);
> + char *end;
> +
> + apr_set_os_error(0);
> + txn_id->revision = strtol(data, &end, 10);
> + if (data[0] == '\0' || apr_get_os_error() == ERANGE)
apr_get_os_error() uses GetLastError() on Windows, while strtol() uses
standard errno for error code.

> + return FALSE;

> +
> data = strchr(end, '-');
> if (data == NULL)
> return FALSE;
> @@ -478,7 +492,6 @@ svn_fs_fs__id_parse(char *data,
> if (str[0] == 'r')
> {
> apr_int64_t val;
> - const char *tmp;
> svn_error_t *err;
>
> /* This is a revision type ID */
> @@ -489,8 +502,15 @@ svn_fs_fs__id_parse(char *data,
> str = svn_cstring_tokenize("/", &data);
> if (str == NULL)
> return NULL;
> - id->private_id.rev_item.revision = svn__strtol(str, &tmp);
>
> + err = svn_cstring_atoi64(&val, str);
> + if (err)
> + {
> + svn_error_clear(err);
> + return NULL;
> + }
> + id->private_id.rev_item.revision = (svn_revnum_t)val;
> +
> err = svn_cstring_atoi64(&val, data);
> if (err)
> {
> Index: subversion/libsvn_subr/string.c
> ===================================================================
> --- subversion/libsvn_subr/string.c (revision 1616191)
> +++ subversion/libsvn_subr/string.c (working copy)
> @@ -1039,45 +1039,6 @@ svn_cstring_atoi(int *n, const char *str)
> return SVN_NO_ERROR;
> }
>
> -unsigned long
> -svn__strtoul(const char* buffer, const char** end)
> -{
> - unsigned long result = 0;
> -
> - /* this loop will execute in just 2 CPU cycles, confirmed by measurement:
> - 7 macro-ops (max 4 / cycle => 2 cycles)
> - 1 load (max 1 / cycle)
> - 1 jumps (compare + conditional jump == 1 macro op; max 1 / cycle)
> - 2 arithmetic ops (subtract, increment; max 3 / cycle)
> - 2 scale-and-add AGU ops (max 3 / cycle)
> - 1 compiler-generated move operation
> - dependency chain: temp = result * 4 + result; result = temp * 2 + c
> - (2 ops with latency 1 => 2 cycles)
> - */
> - while (1)
> - {
> - unsigned long c = (unsigned char)*buffer - (unsigned char)'0';
> - if (c > 9)
> - break;
> -
> - result = result * 10 + c;
> - ++buffer;
> - }
> -
> - *end = buffer;
> - return result;
> -}
> -
> -long
> -svn__strtol(const char* buffer, const char** end)
> -{
> - if (*buffer == '-')
> - return -(long)svn__strtoul(buffer+1, end);
> - else
> - return (long)svn__strtoul(buffer, end);
> -}
> -
> -
> /* "Precalculated" itoa values for 2 places (including leading zeros).
> * For maximum performance, make sure all table entries are word-aligned.
> */
> Index: subversion/libsvn_subr/time.c
> ===================================================================
> --- subversion/libsvn_subr/time.c (revision 1616191)
> +++ subversion/libsvn_subr/time.c (working copy)
> @@ -28,6 +28,7 @@
> #include <apr_pools.h>
> #include <apr_time.h>
> #include <apr_strings.h>
> +#include <apr_errno.h>
> #include "svn_io.h"
> #include "svn_time.h"
> #include "svn_utf.h"
> @@ -34,9 +35,7 @@
> #include "svn_error.h"
> #include "svn_private_config.h"
>
> -#include "private/svn_string_private.h"
>
> -
>
> /*** Code. ***/
>
> @@ -137,25 +136,32 @@ svn_time_from_cstring(apr_time_t *when, const char
> apr_time_exp_t exploded_time;
> apr_status_t apr_err;
> char wday[4], month[4];
> - const char *c;
> + char *c;
>
> - /* Open-code parsing of the new timestamp format, as this
> - is a hot path for reading the entries file. This format looks
> - like: "2001-08-31T04:24:14.966996Z" */
> - exploded_time.tm_year = (apr_int32_t) svn__strtoul(data, &c);
> - if (*c++ != '-') goto fail;
> - exploded_time.tm_mon = (apr_int32_t) svn__strtoul(c, &c);
> - if (*c++ != '-') goto fail;
> - exploded_time.tm_mday = (apr_int32_t) svn__strtoul(c, &c);
> - if (*c++ != 'T') goto fail;
> - exploded_time.tm_hour = (apr_int32_t) svn__strtoul(c, &c);
> - if (*c++ != ':') goto fail;
> - exploded_time.tm_min = (apr_int32_t) svn__strtoul(c, &c);
> - if (*c++ != ':') goto fail;
> - exploded_time.tm_sec = (apr_int32_t) svn__strtoul(c, &c);
> - if (*c++ != '.') goto fail;
> - exploded_time.tm_usec = (apr_int32_t) svn__strtoul(c, &c);
> - if (*c++ != 'Z') goto fail;
> + /* Open-code parsing of the new timestamp format.
> + This format looks like: "2001-08-31T04:24:14.966996Z" */
> + apr_set_os_error(0);
> + exploded_time.tm_year = (apr_int32_t) strtol(data, &c, 10);
> + if (apr_get_os_error() == ERANGE || *c++ != '-')
Same here.

> + goto fail;
> + exploded_time.tm_mon = (apr_int32_t) strtol(c, &c, 10);
> + if (apr_get_os_error() == ERANGE || *c++ != '-')
> + goto fail;
> + exploded_time.tm_mday = (apr_int32_t) strtol(c, &c, 10);
> + if (apr_get_os_error() == ERANGE || *c++ != 'T')
> + goto fail;
> + exploded_time.tm_hour = (apr_int32_t) strtol(c, &c, 10);
> + if (apr_get_os_error() == ERANGE || *c++ != ':')
> + goto fail;
> + exploded_time.tm_min = (apr_int32_t) strtol(c, &c, 10);
> + if (apr_get_os_error() == ERANGE || *c++ != ':')
> + goto fail;
> + exploded_time.tm_sec = (apr_int32_t) strtol(c, &c, 10);
> + if (apr_get_os_error() == ERANGE || *c++ != '.')
> + goto fail;
> + exploded_time.tm_usec = (apr_int32_t) strtol(c, &c, 10);
> + if (apr_get_os_error() == ERANGE || *c++ != 'Z')
> + goto fail;
>
> exploded_time.tm_year -= 1900;
> exploded_time.tm_mon -= 1;
> Index: subversion/libsvn_subr/types.c
> ===================================================================
> --- subversion/libsvn_subr/types.c (revision 1616191)
> +++ subversion/libsvn_subr/types.c (working copy)
> @@ -21,6 +21,7 @@
> * ====================================================================
> */
>
> +#include <apr_errno.h>
> #include <apr_pools.h>
> #include <apr_uuid.h>
>
> @@ -39,19 +40,34 @@ svn_revnum_parse(svn_revnum_t *rev,
> const char *str,
> const char **endptr)
> {
> - const char *end;
> + char *end;
> + apr_int64_t result;
>
> - svn_revnum_t result = (svn_revnum_t)svn__strtoul(str, &end);
> -
> if (endptr)
> *endptr = str;
>
> + /* Use the same error message string as svn_cstring_strtoui64()
> + to limit the burden on translators. */
> + if (str[0] == '\0')
> + return svn_error_createf(SVN_ERR_REVNUM_PARSE_FAILURE, NULL,
> + _("Could not convert '%s' into a number"), "");
> +
> + apr_set_os_error(0);
> +
> + result = (apr_int64_t)strtol(str, &end, 10);
> +
> + if (apr_get_os_error() == ERANGE)
And here.

> + return svn_error_createf(SVN_ERR_REVNUM_PARSE_FAILURE, NULL,
> + _("Number '%s' is out of range"), str);
> +
> + if (result < 0)
> + return svn_error_createf(SVN_ERR_REVNUM_PARSE_FAILURE, NULL,
> + _("Negative revision number found parsing '%s'"),
> + str);
> if (str == end)
> - return svn_error_createf
> - (SVN_ERR_REVNUM_PARSE_FAILURE, NULL,
> - *str == '-' ? _("Negative revision number found parsing '%s'")
> - : _("Invalid revision number found parsing '%s'"),
> - str);
> + return svn_error_createf(SVN_ERR_REVNUM_PARSE_FAILURE, NULL,
> + _("Invalid revision number found parsing '%s'"),
> + str);
>
> /* a revision number with more than 9 digits is suspicious.
> Have a closer look at those. */
> @@ -59,14 +75,12 @@ svn_revnum_parse(svn_revnum_t *rev,
> {
> /* we support 32 bit revision numbers only. check for overflows */
> if (str + 10 < end)
> - return svn_error_createf
> - (SVN_ERR_REVNUM_PARSE_FAILURE, NULL,
> + return svn_error_createf(SVN_ERR_REVNUM_PARSE_FAILURE, NULL,
> _("Revision number longer than 10 digits '%s'"), str);
>
> /* we support 32 bit revision numbers only. check for overflows */
> if (*str > '2' || (apr_uint32_t)result > APR_INT32_MAX)
> - return svn_error_createf
> - (SVN_ERR_REVNUM_PARSE_FAILURE, NULL,
> + return svn_error_createf(SVN_ERR_REVNUM_PARSE_FAILURE, NULL,
> _("Revision number too large '%s'"), str);
> }
>
> @@ -73,7 +87,7 @@ svn_revnum_parse(svn_revnum_t *rev,
> if (endptr)
> *endptr = end;
>
> - *rev = result;
> + *rev = (svn_revnum_t)result;
>
> return SVN_NO_ERROR;
> }

-- 
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com