Hi,
I've recently discovered that the new code that creates the svn_fs_id_t objects
does something quite odd. There is a discrepancy in how functions from
subversion/libsvn_fs_fs/id.c initialize the GENERIC_ID field of the
'fs_fs__id_t' structs. For example, if you compare svn_fs_fs__id_deserialize
and svn_fs_fs__id_create_root around trunk@1549686...
[[[
svn_fs_fs__id_deserialize(void *buffer, svn_fs_id_t **in_out)
{
fs_fs__id_t *id;
...
id->generic_id.vtable = &id_vtable;
id->generic_id.fsap_data = id;
}
svn_fs_id_t *svn_fs_fs__id_create_root(const svn_revnum_t revision,
apr_pool_t *pool)
{
fs_fs__id_t *id = apr_pcalloc(pool, sizeof(*id));
id->private_id.txn_id.revision = SVN_INVALID_REVNUM;
id->private_id.rev_item.revision = revision;
id->private_id.rev_item.number = SVN_FS_FS__ITEM_INDEX_ROOT_NODE;
id->generic_id.vtable = &id_vtable;
id->generic_id.fsap_data = &id;
return (svn_fs_id_t *)id;
}
... you can spot the difference between in the FSAP_DATA initialization. The
second function incorrectly initializes it with a pointer to local variable,
which becomes dangling right after the function returns. Five functions in
subversion/libsvn_fs_fs/id.c have this problem, which was introduced in
r1506545 [1].
This problem does not have any visible consequences, because currently there
is no code directly accessing the FSAP_DATA field in svn_fs_id_t. Since
r1506545, functions like 'svn_fs_fs__id_unparse' utilize the fs_fs__id_t <->
svn_fs_id_t casts and do not require FSAP_DATA (as they do in the 1.8.x branch)
in order to obtain the private FS-ID details.
Still, throwing dangling pointers around (which escalate up to the FS layer,
e.g. svn_fs_begin_txn2 and svn_fs_apply_textdelta) definitely isn't the best
thing to do. The problem is relevant to both FSFS and FSX and I've attached
two patches that fix it (one for FSFS and another for FSX).
[1] http://svn.apache.org/viewvc?view=revision&revision=r1506545
Thanks and regards,
Evgeny Kotkov
Index: subversion/libsvn_fs_fs/id.c
===================================================================
--- subversion/libsvn_fs_fs/id.c (revision 1549686)
+++ subversion/libsvn_fs_fs/id.c (working copy)
@@ -359,7 +359,7 @@ svn_fs_fs__id_txn_create_root(const svn_fs_fs__id_
id->private_id.rev_item.revision = SVN_INVALID_REVNUM;
id->generic_id.vtable = &id_vtable;
- id->generic_id.fsap_data = &id;
+ id->generic_id.fsap_data = id;
return (svn_fs_id_t *)id;
}
@@ -374,7 +374,7 @@ svn_fs_id_t *svn_fs_fs__id_create_root(const svn_r
id->private_id.rev_item.number = SVN_FS_FS__ITEM_INDEX_ROOT_NODE;
id->generic_id.vtable = &id_vtable;
- id->generic_id.fsap_data = &id;
+ id->generic_id.fsap_data = id;
return (svn_fs_id_t *)id;
}
@@ -393,7 +393,7 @@ svn_fs_fs__id_txn_create(const svn_fs_fs__id_part_
id->private_id.rev_item.revision = SVN_INVALID_REVNUM;
id->generic_id.vtable = &id_vtable;
- id->generic_id.fsap_data = &id;
+ id->generic_id.fsap_data = id;
return (svn_fs_id_t *)id;
}
@@ -413,7 +413,7 @@ svn_fs_fs__id_rev_create(const svn_fs_fs__id_part_
id->private_id.rev_item = *rev_item;
id->generic_id.vtable = &id_vtable;
- id->generic_id.fsap_data = &id;
+ id->generic_id.fsap_data = id;
return (svn_fs_id_t *)id;
}
@@ -447,7 +447,7 @@ svn_fs_fs__id_parse(const char *data,
/* Alloc a new svn_fs_id_t structure. */
id = apr_pcalloc(pool, sizeof(*id));
id->generic_id.vtable = &id_vtable;
- id->generic_id.fsap_data = &id;
+ id->generic_id.fsap_data = id;
/* Now, we basically just need to "split" this data on `.'
characters. We will use svn_cstring_tokenize, which will put
Avoid initializing svn_fs_id_t parts with pointers to stack variables, which
will eventually become dangling. Follow-up to r1506545.
* subversion/libsvn_fs_fs/id.c
(svn_fs_fs__id_txn_create_root, svn_fs_fs__id_create_root,
svn_fs_fs__id_txn_create, svn_fs_fs__id_rev_create, svn_fs_fs__id_parse):
Fix the erroneous FSAP_DATA initialization with an address of a temporary.
Patch by: Evgeny Kotkov <evgeny.kotkov{_AT_}visualsvn.com>
Index: subversion/libsvn_fs_x/id.c
===================================================================
--- subversion/libsvn_fs_x/id.c (revision 1549686)
+++ subversion/libsvn_fs_x/id.c (working copy)
@@ -357,7 +357,7 @@ svn_fs_x__id_txn_create_root(const svn_fs_x__id_pa
id->rev_item.revision = SVN_INVALID_REVNUM;
id->generic_id.vtable = &id_vtable;
- id->generic_id.fsap_data = &id;
+ id->generic_id.fsap_data = id;
id->pool = pool;
return (svn_fs_id_t *)id;
@@ -373,7 +373,7 @@ svn_fs_id_t *svn_fs_x__id_create_root(const svn_re
id->rev_item.number = SVN_FS_X__ITEM_INDEX_ROOT_NODE;
id->generic_id.vtable = &id_vtable;
- id->generic_id.fsap_data = &id;
+ id->generic_id.fsap_data = id;
id->pool = pool;
return (svn_fs_id_t *)id;
@@ -393,7 +393,7 @@ svn_fs_x__id_txn_create(const svn_fs_x__id_part_t
id->rev_item.revision = SVN_INVALID_REVNUM;
id->generic_id.vtable = &id_vtable;
- id->generic_id.fsap_data = &id;
+ id->generic_id.fsap_data = id;
id->pool = pool;
return (svn_fs_id_t *)id;
@@ -414,7 +414,7 @@ svn_fs_x__id_rev_create(const svn_fs_x__id_part_t
id->rev_item = *rev_item;
id->generic_id.vtable = &id_vtable;
- id->generic_id.fsap_data = &id;
+ id->generic_id.fsap_data = id;
id->pool = pool;
return (svn_fs_id_t *)id;
@@ -449,7 +449,7 @@ svn_fs_x__id_parse(const char *data,
/* Alloc a new svn_fs_id_t structure. */
id = apr_pcalloc(pool, sizeof(*id));
id->generic_id.vtable = &id_vtable;
- id->generic_id.fsap_data = &id;
+ id->generic_id.fsap_data = id;
id->pool = pool;
/* Now, we basically just need to "split" this data on `.'
Avoid initializing svn_fs_id_t parts with pointers to stack variables, which
will eventually become dangling. Follow-up to r1508041.
* subversion/libsvn_fs_x/id.c
(svn_fs_x__id_txn_create_root, svn_fs_x__id_create_root,
svn_fs_x__id_txn_create, svn_fs_x__id_rev_create, svn_fs_x__id_parse):
Fix the erroneous FSAP_DATA initialization with an address of a temporary.
Patch by: Evgeny Kotkov <evgeny.kotkov{_AT_}visualsvn.com>
