You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@wandisco.com> on 2015/08/26 09:17:40 UTC
Re: svn commit: r1697828 -
/subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
On 26.08.2015 09:14, brane@apache.org wrote:
> Author: brane
> Date: Wed Aug 26 07:14:59 2015
> New Revision: 1697828
>
> URL: http://svn.apache.org/r1697828
> Log:
> Fix a 64-bit to 32-bit conversion warning on 64-bit platforms.
>
> * subversion/libsvn_subr/cache-membuffer.c
> (prefix_pool_get_internal): Safely cast a pointer to an index, with range check.
>
> Modified:
> subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
>
> Modified: subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/cache-membuffer.c?rev=1697828&r1=1697827&r2=1697828&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/cache-membuffer.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/cache-membuffer.c Wed Aug 26 07:14:59 2015
> @@ -339,7 +339,9 @@ prefix_pool_get_internal(apr_uint32_t *p
> value = apr_hash_get(prefix_pool->map, prefix, prefix_len);
> if (value != NULL)
> {
> - *prefix_idx = value - prefix_pool->values;
> + const apr_size_t index = value - prefix_pool->values;
> + SVN_ERR_ASSERT(index < prefix_pool->values_used);
> + *prefix_idx = (apr_uint32_t) index;
> return SVN_NO_ERROR;
> }
Stefan2, please double-check this change. It's not strictly correct
since the type of the expression is ptrdiff_t, not size_t. In any case,
I wasn't comfortable with just blindly casting the result.
-- Brane
Re: svn commit: r1697828 - /subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Aug 26, 2015 at 8:17 AM, Branko Čibej <br...@wandisco.com> wrote:
> On 26.08.2015 09:14, brane@apache.org wrote:
> > Author: brane
> > Date: Wed Aug 26 07:14:59 2015
> > New Revision: 1697828
> >
> > URL: http://svn.apache.org/r1697828
> > Log:
> > Fix a 64-bit to 32-bit conversion warning on 64-bit platforms.
> >
> > * subversion/libsvn_subr/cache-membuffer.c
> > (prefix_pool_get_internal): Safely cast a pointer to an index, with
> range check.
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
> >
> > Modified: subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/cache-membuffer.c?rev=1697828&r1=1697827&r2=1697828&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_subr/cache-membuffer.c (original)
> > +++ subversion/trunk/subversion/libsvn_subr/cache-membuffer.c Wed Aug 26
> 07:14:59 2015
> > @@ -339,7 +339,9 @@ prefix_pool_get_internal(apr_uint32_t *p
> > value = apr_hash_get(prefix_pool->map, prefix, prefix_len);
> > if (value != NULL)
> > {
> > - *prefix_idx = value - prefix_pool->values;
> > + const apr_size_t index = value - prefix_pool->values;
> > + SVN_ERR_ASSERT(index < prefix_pool->values_used);
> > + *prefix_idx = (apr_uint32_t) index;
> > return SVN_NO_ERROR;
> > }
>
> Stefan2, please double-check this change. It's not strictly correct
> since the type of the expression is ptrdiff_t, not size_t.
The only way the first cast would be a problem is some coding snafu.
Otherwise, the diff should never be negative and be within the
VALUES array, i.e. never exceed apr_size_t. Any sign and length
conversion should be safe in that case.
> In any case,
> I wasn't comfortable with just blindly casting the result.
>
The SVN_ERR_ASSERT is fine; this is not on a hot code path.
And it is likely to catch coding snafus, even in cases where the
first cast is not strictly correct.
-- Stefan^2.