You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2014/09/30 13:10:02 UTC
svn commit: r1628393 - /subversion/trunk/subversion/libsvn_fs_fs/index.c
Author: stefan2
Date: Tue Sep 30 11:10:02 2014
New Revision: 1628393
URL: http://svn.apache.org/r1628393
Log:
Fix / silence some integer conversion warnings.
* subversion/libsvn_fs_fs/index.c
(packed_stream_read): We know those are well within apr_size_t's
value range.
(auto_open_l2p_index): The config reader limits the BLOCK_SIZE
to apr_size_t since r1628392.
(get_l2p_header_body): Use the appropriate type for the iteration
variable. Use maximally widening casts
for non-negative values where appropriate.
Cast revnum count after we verified its range
to be within MAX_FILES_PER_DIR.
(prefetch_l2p_pages): Exit early for non-positive ranges and use
maximally widening casts for non-negative
values where appropriate.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/index.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/index.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/index.c?rev=1628393&r1=1628392&r2=1628393&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/index.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/index.c Tue Sep 30 11:10:02 2014
@@ -263,11 +263,11 @@ packed_stream_read(svn_fs_fs__packed_num
read = sizeof(buffer);
block_left = stream->block_size - (stream->next_offset - block_start);
if (block_left >= 10 && block_left < read)
- read = block_left;
+ read = (apr_size_t)block_left;
/* Don't read beyond the end of the file section that belongs to this
* index / stream. */
- read = MIN(read, stream->stream_end - stream->next_offset);
+ read = (apr_size_t)MIN(read, stream->stream_end - stream->next_offset);
err = apr_file_read(stream->file, buffer, &read);
if (err && !APR_STATUS_IS_EOF(err))
@@ -897,7 +897,7 @@ auto_open_l2p_index(svn_fs_fs__revision_
rev_file->file,
rev_file->l2p_offset,
rev_file->p2l_offset,
- ffd->block_size,
+ (apr_size_t)ffd->block_size,
rev_file->pool));
}
@@ -918,11 +918,12 @@ get_l2p_header_body(l2p_header_t **heade
{
fs_fs_data_t *ffd = fs->fsap_data;
apr_uint64_t value;
- int i;
+ apr_size_t i;
apr_size_t page, page_count;
apr_off_t offset;
l2p_header_t *result = apr_pcalloc(result_pool, sizeof(*result));
apr_size_t page_table_index;
+ svn_revnum_t next_rev;
pair_cache_key_t key;
key.revision = rev_file->start_revision;
@@ -948,7 +949,7 @@ get_l2p_header_body(l2p_header_t **heade
SVN_ERR(packed_stream_get(&value, rev_file->l2p_stream));
result->revision_count = (int)value;
if ( result->revision_count != 1
- && result->revision_count != ffd->max_files_per_dir)
+ && result->revision_count != (apr_uint64_t)ffd->max_files_per_dir)
return svn_error_create(SVN_ERR_FS_INDEX_CORRUPTION, NULL,
_("Invalid number of revisions in L2P index"));
@@ -961,12 +962,11 @@ get_l2p_header_body(l2p_header_t **heade
return svn_error_create(SVN_ERR_FS_INDEX_CORRUPTION, NULL,
_("L2P index page count implausibly large"));
- if ( result->first_revision > revision
- || result->first_revision + result->revision_count <= revision)
+ next_rev = result->first_revision + (svn_revnum_t)result->revision_count;
+ if (result->first_revision > revision || next_rev <= revision)
return svn_error_createf(SVN_ERR_FS_INDEX_CORRUPTION, NULL,
_("Corrupt L2P index for r%ld only covers r%ld:%ld"),
- revision, result->first_revision,
- result->first_revision + result->revision_count);
+ revision, result->first_revision, next_rev);
/* allocate the page tables */
result->page_table
@@ -1328,6 +1328,17 @@ prefetch_l2p_pages(svn_boolean_t *end,
apr_pool_t *iterpool;
svn_fs_fs__page_cache_key_t key = { 0 };
+ /* Parameter check. */
+ if (min_offset < 0)
+ min_offset = 0;
+
+ if (max_offset <= 0)
+ {
+ /* Nothing to do */
+ *end = TRUE;
+ return SVN_NO_ERROR;
+ }
+
/* get the page table for REVISION from cache */
*end = FALSE;
SVN_ERR(get_l2p_page_table(pages, fs, rev_file, revision, scratch_pool));
@@ -1357,8 +1368,8 @@ prefetch_l2p_pages(svn_boolean_t *end,
continue;
/* skip pages outside the specified index file range */
- if ( entry->offset < min_offset
- || entry->offset + entry->size > max_offset)
+ if ( entry->offset < (apr_uint64_t)min_offset
+ || entry->offset + entry->size > (apr_uint64_t)max_offset)
{
*end = TRUE;
continue;
Re: svn commit: r1628393 - /subversion/trunk/subversion/libsvn_fs_fs/index.c
Posted by Branko Čibej <br...@wandisco.com>.
On 30.09.2014 15:39, Stefan Fuhrmann wrote:
> On Tue, Sep 30, 2014 at 2:40 PM, Branko Čibej <brane@wandisco.com
> <ma...@wandisco.com>> wrote:
>
> On 30.09.2014 13:10, stefan2@apache.org
> <ma...@apache.org> wrote:
> > Author: stefan2
> > Date: Tue Sep 30 11:10:02 2014
> > New Revision: 1628393
> >
> > URL: http://svn.apache.org/r1628393
> > Log:
> > Fix / silence some integer conversion warnings.
> >
> > * subversion/libsvn_fs_fs/index.c
> > (packed_stream_read): We know those are well within apr_size_t's
> > value range.
> > (auto_open_l2p_index): The config reader limits the BLOCK_SIZE
> > to apr_size_t since r1628392.
> > (get_l2p_header_body): Use the appropriate type for the iteration
> > variable. Use maximally widening casts
> > for non-negative values where appropriate.
> > Cast revnum count after we verified its
> range
> > to be within MAX_FILES_PER_DIR.
> > (prefetch_l2p_pages): Exit early for non-positive ranges and use
> > maximally widening casts for non-negative
> > values where appropriate.
>
> Uh.
>
> In my book, every cast is wrong,
>
>
> Well, casts are undesirable but not generally wrong.
> I wish I could do with "just int" but that is too large
> for arrays on e.g. x32 and too small for file sizes
> on others, e.g. Windows.
>
> The index code is particularly affected as it needs
> to mediate between in-memory sizes (apr_size_t),
> platform-independent representation (apr_uint64_t)
> and file addressing (apr_off_t).
As I said, it's OK to cast on API boundaries, as long as we can prove
that the casts are safe. SVN<->APR is an API boundary.
>
>
> and widening casts are doubly wrong
> because they're implicit.
>
>
> Yes, they are implicit but VS still warns about signed/
> unsigned conversions. The simplest portable way I can
> think of to make these comparisons work is to cast
> to the largest unsigned we have once we made sure
> that the value is non-negative.
Couldn't care less about VS ... it also warns about a ton of stuff
that's perfectly valid C even when casts are not involved.
The problem is that casts tend to mask actual coding errors from the
compiler. I prefer warnigns from some silly compiler to adding casts
just for the sake of shutting it up.
> Casts are marginally acceptable if they're
> needed to adapt to a poorly designed and/or hardware/platform-specific
> API. But in this case, all the values involved appear to be parameters
> or return values of our own private functions.
>
>
> No. The first one, for instance, converts from apr_off_t
> (for positions in the pack / rev file currently being read)
> to apr_size_t (what the system returns from sizeof).
Red flag right there: what guarantees that a revision or pack file
cannot be larger than 4 gigs, even on 32-bit platforms? One large binary
file (think video assets or chip design files) can easily be larger than
that. I thought we did not have restrictions on file content size.
-- Brane
Re: svn commit: r1628393 - /subversion/trunk/subversion/libsvn_fs_fs/index.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Sep 30, 2014 at 2:40 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 30.09.2014 13:10, stefan2@apache.org wrote:
> > Author: stefan2
> > Date: Tue Sep 30 11:10:02 2014
> > New Revision: 1628393
> >
> > URL: http://svn.apache.org/r1628393
> > Log:
> > Fix / silence some integer conversion warnings.
> >
> > * subversion/libsvn_fs_fs/index.c
> > (packed_stream_read): We know those are well within apr_size_t's
> > value range.
> > (auto_open_l2p_index): The config reader limits the BLOCK_SIZE
> > to apr_size_t since r1628392.
> > (get_l2p_header_body): Use the appropriate type for the iteration
> > variable. Use maximally widening casts
> > for non-negative values where appropriate.
> > Cast revnum count after we verified its range
> > to be within MAX_FILES_PER_DIR.
> > (prefetch_l2p_pages): Exit early for non-positive ranges and use
> > maximally widening casts for non-negative
> > values where appropriate.
>
> Uh.
>
> In my book, every cast is wrong,
Well, casts are undesirable but not generally wrong.
I wish I could do with "just int" but that is too large
for arrays on e.g. x32 and too small for file sizes
on others, e.g. Windows.
The index code is particularly affected as it needs
to mediate between in-memory sizes (apr_size_t),
platform-independent representation (apr_uint64_t)
and file addressing (apr_off_t).
> and widening casts are doubly wrong
> because they're implicit.
Yes, they are implicit but VS still warns about signed/
unsigned conversions. The simplest portable way I can
think of to make these comparisons work is to cast
to the largest unsigned we have once we made sure
that the value is non-negative.
> Casts are marginally acceptable if they're
> needed to adapt to a poorly designed and/or hardware/platform-specific
> API. But in this case, all the values involved appear to be parameters
> or return values of our own private functions.
>
No. The first one, for instance, converts from apr_off_t
(for positions in the pack / rev file currently being read)
to apr_size_t (what the system returns from sizeof).
The wiggle room we do have is with the data types
we use in our structs. The options are:
* Use platform-specific types. Then, we may need to
convert at the interfaces (files, network and API).
* Use SVN-specific types of a fixed size. Then we
need to convert when calling into libs / OS.
Traditionally, SVN tends to follow the first approach and
has seen some problems with 'long' as revision number
and 'int' as array index. For some parts of the FSFS index
and more so in FSX, I'm following the second approach.
Not sure which one is actually better in the long run.
-- Stefan^2.
Re: svn commit: r1628393 - /subversion/trunk/subversion/libsvn_fs_fs/index.c
Posted by Branko Čibej <br...@wandisco.com>.
On 30.09.2014 13:10, stefan2@apache.org wrote:
> Author: stefan2
> Date: Tue Sep 30 11:10:02 2014
> New Revision: 1628393
>
> URL: http://svn.apache.org/r1628393
> Log:
> Fix / silence some integer conversion warnings.
>
> * subversion/libsvn_fs_fs/index.c
> (packed_stream_read): We know those are well within apr_size_t's
> value range.
> (auto_open_l2p_index): The config reader limits the BLOCK_SIZE
> to apr_size_t since r1628392.
> (get_l2p_header_body): Use the appropriate type for the iteration
> variable. Use maximally widening casts
> for non-negative values where appropriate.
> Cast revnum count after we verified its range
> to be within MAX_FILES_PER_DIR.
> (prefetch_l2p_pages): Exit early for non-positive ranges and use
> maximally widening casts for non-negative
> values where appropriate.
Uh.
In my book, every cast is wrong, and widening casts are doubly wrong
because they're implicit. Casts are marginally acceptable if they're
needed to adapt to a poorly designed and/or hardware/platform-specific
API. But in this case, all the values involved appear to be parameters
or return values of our own private functions.
-- Brane