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.
Since error and range checking needs to be done in any case I don't
think it's a good idea to offer an interface that leaves these checks
up to the caller. Rather, the caller must not be given a way around
these checks or our users will inevitably be running code that forgets
about them.
A few years ago I spent a bit of time on introducing some svn_cstring
interfaces for number conversions. I think we should use them
wherever possible. Here's a recent event where this mattered:
http://svn.haxx.se/users/archive-2014-08/0033.shtml
This made me rummage through the code again for unchecked string-to-number
conversions which led me to stumble across svn__strtol(). Unfortunately
I'm a bit late to the party since this code already made it into 1.8.x.
This patch uses checked strtol() call in svn_time_from_cstring()
and elsewhere in places where the svn_cstring APIs are difficult to
use because code relies on *endp being available. I'm thinking about
extending the svn_cstring interface to cover these cases later but
this patch should be good enough for now.
Any objections to this change?
[[[
Remove svn__strtol() and svn__strtoul(). They were introduced as a
performance enhancement but don't provide sufficient error checking.
* include/private/svn_string_private.h
(svn__strtoul, svn__strtol): Remove declarations of these functions.
* libsvn_fs_fs/id.c
(part_parse, svn_fs_fs_id_parse): Use svn_cstring_atoi64() instead.
(txn_id_parse): Use checked strtol() call instead.
* libsvn_subr/string.c
(svn__strtoul, svn__strtol): Remove.
* libsvn_subr/time.c
(svn_time_from_cstring): Use checked strtol() calls instead.
* libsvn_subr/types.c
(svn_revnum_parse): Use checked strtol() call instead. While here,
eliminate space-before-paren.
]]]
Index: subversion/include/private/svn_string_private.h
===================================================================
--- subversion/include/private/svn_string_private.h (revision 1616191)
+++ subversion/include/private/svn_string_private.h (working copy)
@@ -131,21 +131,6 @@ svn_membuf__nzero(svn_membuf_t *membuf, apr_size_t
svn_string_t *
svn_stringbuf__morph_into_string(svn_stringbuf_t *strbuf);
-/** Like strtoul but with a fixed base of 10 and without overflow checks.
- * This allows the compiler to generate massively faster (4x on 64bit LINUX)
- * code. Overflow checks may be added on the caller side where you might
- * want to test for a more specific value range anyway.
- */
-unsigned long
-svn__strtoul(const char *buffer, const char **end);
-
-/** Like strtol but with a fixed base of 10 and without overflow checks.
- * This allows the compiler to generate massively faster code.
- * (E.g. Avoiding locale specific processing)
- */
-long
-svn__strtol(const char *buffer, const char **end);
-
/** Number of chars needed to represent signed (19 places + sign + NUL) or
* unsigned (20 places + NUL) integers as strings.
*/
Index: subversion/libsvn_fs_fs/id.c
===================================================================
--- subversion/libsvn_fs_fs/id.c (revision 1616191)
+++ subversion/libsvn_fs_fs/id.c (working copy)
@@ -79,8 +79,17 @@ part_parse(svn_fs_fs__id_part_t *part,
}
{
- const char *end;
- part->revision = svn__strtol(data+1, &end);
+ apr_int64_t rev;
+ svn_error_t *err;
+
+ err = svn_cstring_atoi64(&rev, data + 1);
+ if (err)
+ {
+ svn_error_clear(err);
+ return FALSE;
+ }
+
+ part->revision = (svn_revnum_t)rev;
}
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)
+ 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++ != '-')
+ 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)
+ 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;
}
Received on 2014-08-06 17:12:27 CEST