Hi,
I've noticed that the svn_fs_fs__get_changes function currently invokes
undefined behavior when there is no CHANGES_CACHE present. According to the
fs_fs_data_t contract, this is a perfectly valid situation (see
subversion\libsvn_fs_fs\fs.h around trunk@1549686):
[[[
/* Private (non-shared) FSFS-specific data for each svn_fs_t object.
Any caches in here may be NULL. */
]]]
However, explicitly setting the CHANGES_CACHE to NULL leads to a massive amount
of failing tests (1085 fails on my 64-bit Ubuntu machine, some of them are
segfaults). This erroneous behavior was introduced in r1512904 [1] and happens
due to the usage of the uninitialized variable FOUND when the CHANGES_CACHE
is absent. The behavior in this case is undefined and that will likely lead to
a crash. Here is a tip from valgrind:
[[[
Conditional jump or move depends on uninitialised value(s)
at 0x4852AF: svn_fs_fs__get_changes (cached_data.c:2183)
by 0x469E4C: svn_fs_fs__paths_changed (transaction.c:889)
by 0x475CDE: fs_paths_changed (tree.c:3291)
by 0x44DCE7: svn_fs_paths_changed2 (fs-loader.c:1006)
...
Use of uninitialised value of size 8
at 0x469E8C: svn_fs_fs__paths_changed (transaction.c:892)
by 0x475CDE: fs_paths_changed (tree.c:3291)
by 0x44DCE7: svn_fs_paths_changed2 (fs-loader.c:1006)
...
]]]
This happens with both FSFS and FSX, so I've attached two patches to fix this
issue (these patches fix the svn_fs_fs__get_changes and svn_fs_x__get_changes
functions accordingly).
[1] http://svn.apache.org/viewvc?view=revision&revision=r1512904
Thanks and regards,
Evgeny Kotkov
Index: subversion/libsvn_fs_fs/cached_data.c
===================================================================
--- subversion/libsvn_fs_fs/cached_data.c (revision 1549686)
+++ subversion/libsvn_fs_fs/cached_data.c (working copy)
@@ -2177,8 +2177,14 @@ svn_fs_fs__get_changes(apr_array_header_t **change
/* try cache lookup first */
if (ffd->changes_cache)
- SVN_ERR(svn_cache__get((void **) changes, &found, ffd->changes_cache,
- &rev, pool));
+ {
+ SVN_ERR(svn_cache__get((void **) changes, &found, ffd->changes_cache,
+ &rev, pool));
+ }
+ else
+ {
+ found = FALSE;
+ }
if (!found)
{
Avoid undefined behavior (and crashing) within svn_fs_fs__get_changes when
there is no changes cache. Caches in fs_fs_data_t are optional by design:
[[[
/* Private (non-shared) FSFS-specific data for each svn_fs_t object.
Any caches in here may be NULL. */
]]]
Follow-up to r1512904.
* subversion/libsvn_fs_fs/cached_data.c
(svn_fs_fs__get_changes): Avoid possible uninitialized variable access by
setting FOUND to FALSE in case we do not have a CHANGES_CACHE.
Patch by: Evgeny Kotkov <evgeny.kotkov{_AT_}visualsvn.com>
Index: subversion/libsvn_fs_x/cached_data.c
===================================================================
--- subversion/libsvn_fs_x/cached_data.c (revision 1549686)
+++ subversion/libsvn_fs_x/cached_data.c (working copy)
@@ -2335,6 +2335,10 @@ svn_fs_x__get_changes(apr_array_header_t **changes
SVN_ERR(svn_cache__get((void **) changes, &found, ffd->changes_cache,
&rev, pool));
}
+ else
+ {
+ found = FALSE;
+ }
if (!found)
{
Avoid undefined behavior (and crashing) within svn_fs_x__get_changes when
there are no changes and changes container caches. Caches in fs_x_data_t are
optional by design:
[[[
/* Private (non-shared) FSX-specific data for each svn_fs_t object.
Any caches in here may be NULL. */
]]]
Follow-up to r1508041.
* subversion/libsvn_fs_x/cached_data.c
(svn_fs_x__get_changes): Avoid possible uninitialized variable access by
setting FOUND to FALSE in case we do not have CHANGES_CACHE and
CHANGES_CONTAINER_CACHE.
Patch by: Evgeny Kotkov <evgeny.kotkov{_AT_}visualsvn.com>
