Daniel Shahaf wrote on Tue, Sep 13, 2011 at 03:27:07 +0300:
> On the fs-successor-ids branch we store 64bit ints in big endian format
> in one of the files. The following patch changes the function which
> reads that file such that it returns an apr_off_t offset (which is
> convenient for further processing), and in return moves and changes the
> overflow check.
>
> I'll fix the overflow check which I add to mirror the wording of the one
> which I remove. Other than that, does below look correct?
>
> Thanks.
>
> [[[
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 1169978)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -5950,7 +5950,7 @@ copy_file_partially(apr_file_t *source_file,
> * handle, which is allocated in RESULT_POOL.
> */
> static svn_error_t *
> -read_successor_index_entry(apr_uint64_t *revision_offset,
> +read_successor_index_entry(apr_off_t *revision_offset,
> apr_file_t **file_p,
> svn_fs_t *fs,
> svn_revnum_t revision,
> @@ -5962,6 +5962,7 @@ static svn_error_t *
> apr_file_t *file;
> apr_uint32_t m;
> apr_uint32_t n;
> + apr_uint64_t nm;
> const char *local_abspath = path_successor_ids(fs, revision, scratch_pool);
>
> /* The trivial case: The caller asked for the first revision in a file. */
> @@ -6002,7 +6003,13 @@ static svn_error_t *
> _("Missing data at offset %llu in file '%s'"),
> offset + 4, local_abspath);
> }
> - *revision_offset = ((apr_uint64_t)(ntohl(n)) << 32) | ntohl(m);
> + nm = ((apr_uint64_t)(ntohl(n)) << 32) | ntohl(m);
> + if ((apr_off_t)nm != nm)
I'll change this to
+ if ((apr_off_t)nm != nm || (apr_off_t)nm < 0)
> + return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
> + _("Successors data lives at offset larger than "
> + "this platform can represent (%ld<<32 + %ld)"),
> + (long)n, (long)m);
> + *revision_offset = nm;
>
> if (file_p && ! *file_p)
> *file_p = file;
> @@ -6044,23 +6051,12 @@ update_successor_ids_file(const char **successor_i
> }
> else
> {
> - apr_uint64_t prev_successor_ids_offset;
> + apr_off_t prev_successor_ids_offset;
> apr_file_t *successor_ids_file;
>
> /* Figure out the offset of successor data for the previous revision. */
> SVN_ERR(read_successor_index_entry(&prev_successor_ids_offset,
> NULL, fs, new_rev - 1, pool, pool));
> -
> - /* Check for offset overflow.
> - * This gives a "will never be executed" warning on some platforms. */
> - if (sizeof(apr_off_t) < sizeof(apr_uint64_t) &&
> - prev_successor_ids_offset > APR_UINT32_MAX)
> - return svn_error_createf(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> - _("Cannot seek to offset %llu in successor "
> - "data file; platform does not support "
> - "large files; this repository can only "
> - "be used on platforms which support "
> - "large files"), prev_successor_ids_offset);
>
> /* Copy successor IDs index and the data for revisions older than
> * the previous revision into the temporary successor data file. */
> ]]]
Received on 2011-09-13 02:49:40 CEST