[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

fsfs-stats potentially busted on some architectures

From: Ben Reser <ben_at_reser.org>
Date: Wed, 19 Feb 2014 15:00:01 -0800

Found via the following compiler warning on OS X:
/Users/breser/wandisco/share/wcs/svn-trunk/tools/server-side/fsfs-stats.c:1556:61:
warning: implicit conversion loses integer precision: 'long' to 'int'
[-Wshorten-64-to-32]
                                    ffd->youngest_rev_cache + 1,
                                    ~~~~~~~~~~~~~~~~~~~~~~~~^~~
1 warning generated.

The rest of the statement for that line:
[[[
  /* create data containers and caches */
  (*fs)->revisions = apr_array_make(pool,
                                    ffd->youngest_rev_cache + 1,
                                    sizeof(revision_info_t *));
]]]

apr_array_make is declared as:
apr_array_header_t * apr_array_make (apr_pool_t *p, int nelts, int elt_size)

int is only guaranteed to be 16 bits. We support up to 32-bit revision numbers
(which technically some platforms supported up to 64-bit revision numbers until
we recently started enforcing that it stayed under 32-bits in our revision
string parser).

Thus running fsfs-stats on a machine with a 16-bit int and a repository with
more than 32768 revisions should have a problem.

I'm not sure if we should care about this. I'm guessing we in general don't
support 16-bit architectures, which is where this is likely to be a problem.
We almost certainly have a lot worse problems elsewhere.

I only see two paths to fixing this:

1) Create a new APR array interface that uses something other than int for the
size of the array.

2) Rework the code so that it doesn't need to maintain information about every
revision. However, this seems like it'd make things much slower due to needing
to look up rep sharing.

I'm tempted to commit the following for now:
[[[
Index: tools/server-side/fsfs-stats.c
===================================================================
--- tools/server-side/fsfs-stats.c (revision 1569653)
+++ tools/server-side/fsfs-stats.c (working copy)
@@ -1551,9 +1551,14 @@
   SVN_ERR(fs_open(fs, path, pool));
   ffd = (*fs)->fs->fsap_data;

- /* create data containers and caches */
+ /* create data containers and caches
+ * Note: this assumes that int is at least 32-bits and that we only support
+ * 32-bit wide revision numbers (actually 31-bits due to the signedness
+ * of both the nelts field of the array and our revision numbers). This
+ * means this code will fail on platforms where int is less than 32-bits
+ * and the repository has more revisions than int can hold. */
   (*fs)->revisions = apr_array_make(pool,
- ffd->youngest_rev_cache + 1,
+ (int) ffd->youngest_rev_cache + 1,
                                     sizeof(revision_info_t *));
   (*fs)->null_base = apr_pcalloc(pool, sizeof(*(*fs)->null_base));
   initialize_largest_changes(*fs, 64, pool);
]]]
Received on 2014-02-20 00:00:31 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.