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_at_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
Received on 2013-12-10 04:41:44 CET