You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by sv...@apache.org on 2015/03/19 19:24:01 UTC
svn commit: r1667837 - in /subversion/branches/1.9.x: ./ STATUS
subversion/libsvn_fs/fs-loader.c subversion/tests/cmdline/svnlook_tests.py
subversion/tests/libsvn_fs/fs-test.c
Author: svn-role
Date: Thu Mar 19 18:24:00 2015
New Revision: 1667837
URL: http://svn.apache.org/r1667837
Log:
Merge r1663738 from trunk:
* r1663738
Stop exposing and prohibit changing internal txn props through FS API
Justification:
Prevents an implementation detail leak. Prohibits changing the
internal behavior of our transactions via public API. Avoids a situation
with the API function call discarding the data and falsely reporting
success for "set" operations with 'svn:client-date' properties, i.e.,
with the API telling us that the operation completed successfully when
the change itself was not applied. (The last part is new-in-1.9.)
Notes:
While this is mostly an API correctness fix, there is a user-visible
consequence of 'svnlook proplist' no longer leaking internal properties
like svn:check-locks for transactions.
Votes:
+1: kotkov, rhuijben, stefan2
+0: philip (the new behaviour is fine but so is the old behaviour.
svn:client-date is internal so does not have to obey the
rules that apply to user properties. The proplist "leak" is
strictly a regression as it is no longer possible to determine
whether the CHECK_LOCKS flag is set on a txn.)
Modified:
subversion/branches/1.9.x/ (props changed)
subversion/branches/1.9.x/STATUS
subversion/branches/1.9.x/subversion/libsvn_fs/fs-loader.c
subversion/branches/1.9.x/subversion/tests/cmdline/svnlook_tests.py
subversion/branches/1.9.x/subversion/tests/libsvn_fs/fs-test.c
Propchange: subversion/branches/1.9.x/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Thu Mar 19 18:24:00 2015
@@ -89,4 +89,4 @@
/subversion/branches/verify-at-commit:1462039-1462408
/subversion/branches/verify-keep-going:1439280-1546110
/subversion/branches/wc-collate-path:1402685-1480384
-/subversion/trunk:1660545-1660547,1660549-1662901,1663003,1663183-1663184,1663338,1663347,1663374,1663450,1663697,1663706,1663749,1663791,1664078,1664080,1664084-1664085,1664187,1664191,1664200,1664344,1664476,1664480-1664481,1664483,1664507,1664520-1664521,1664523,1664526-1664527,1664531-1664532,1664588,1664927,1665164,1665611-1665612,1665845,1665850,1665852,1665886,1666270,1666272,1666690,1666851
+/subversion/trunk:1660545-1660547,1660549-1662901,1663003,1663183-1663184,1663338,1663347,1663374,1663450,1663697,1663706,1663738,1663749,1663791,1664078,1664080,1664084-1664085,1664187,1664191,1664200,1664344,1664476,1664480-1664481,1664483,1664507,1664520-1664521,1664523,1664526-1664527,1664531-1664532,1664588,1664927,1665164,1665611-1665612,1665845,1665850,1665852,1665886,1666270,1666272,1666690,1666851
Modified: subversion/branches/1.9.x/STATUS
URL: http://svn.apache.org/viewvc/subversion/branches/1.9.x/STATUS?rev=1667837&r1=1667836&r2=1667837&view=diff
==============================================================================
--- subversion/branches/1.9.x/STATUS (original)
+++ subversion/branches/1.9.x/STATUS Thu Mar 19 18:24:00 2015
@@ -178,27 +178,6 @@ Veto-blocked changes:
Approved changes:
=================
- * r1663738
- Stop exposing and prohibit changing internal txn props through FS API
- Justification:
- Prevents an implementation detail leak. Prohibits changing the
- internal behavior of our transactions via public API. Avoids a situation
- with the API function call discarding the data and falsely reporting
- success for "set" operations with 'svn:client-date' properties, i.e.,
- with the API telling us that the operation completed successfully when
- the change itself was not applied. (The last part is new-in-1.9.)
- Notes:
- While this is mostly an API correctness fix, there is a user-visible
- consequence of 'svnlook proplist' no longer leaking internal properties
- like svn:check-locks for transactions.
- Votes:
- +1: kotkov, rhuijben, stefan2
- +0: philip (the new behaviour is fine but so is the old behaviour.
- svn:client-date is internal so does not have to obey the
- rules that apply to user properties. The proplist "leak" is
- strictly a regression as it is no longer possible to determine
- whether the CHECK_LOCKS flag is set on a txn.)
-
* r1664193
Fix win32 resource generation for svnbench.exe
Justification:
Modified: subversion/branches/1.9.x/subversion/libsvn_fs/fs-loader.c
URL: http://svn.apache.org/viewvc/subversion/branches/1.9.x/subversion/libsvn_fs/fs-loader.c?rev=1667837&r1=1667836&r2=1667837&view=diff
==============================================================================
--- subversion/branches/1.9.x/subversion/libsvn_fs/fs-loader.c (original)
+++ subversion/branches/1.9.x/subversion/libsvn_fs/fs-loader.c Thu Mar 19 18:24:00 2015
@@ -916,26 +916,48 @@ svn_fs_list_transactions(apr_array_heade
return svn_error_trace(fs->vtable->list_transactions(names_p, fs, pool));
}
+static svn_boolean_t
+is_internal_txn_prop(const char *name)
+{
+ return strcmp(name, SVN_FS__PROP_TXN_CHECK_LOCKS) == 0 ||
+ strcmp(name, SVN_FS__PROP_TXN_CHECK_OOD) == 0 ||
+ strcmp(name, SVN_FS__PROP_TXN_CLIENT_DATE) == 0;
+}
+
svn_error_t *
svn_fs_txn_prop(svn_string_t **value_p, svn_fs_txn_t *txn,
const char *propname, apr_pool_t *pool)
{
+ if (is_internal_txn_prop(propname))
+ {
+ *value_p = NULL;
+ return SVN_NO_ERROR;
+ }
+
return svn_error_trace(txn->vtable->get_prop(value_p, txn, propname, pool));
}
svn_error_t *
svn_fs_txn_proplist(apr_hash_t **table_p, svn_fs_txn_t *txn, apr_pool_t *pool)
{
- return svn_error_trace(txn->vtable->get_proplist(table_p, txn, pool));
+ SVN_ERR(txn->vtable->get_proplist(table_p, txn, pool));
+
+ /* Don't give away internal transaction properties. */
+ svn_hash_sets(*table_p, SVN_FS__PROP_TXN_CHECK_LOCKS, NULL);
+ svn_hash_sets(*table_p, SVN_FS__PROP_TXN_CHECK_OOD, NULL);
+ svn_hash_sets(*table_p, SVN_FS__PROP_TXN_CLIENT_DATE, NULL);
+
+ return SVN_NO_ERROR;
}
svn_error_t *
svn_fs_change_txn_prop(svn_fs_txn_t *txn, const char *name,
const svn_string_t *value, apr_pool_t *pool)
{
- /* Silently drop attempts to modify the internal property. */
- if (!strcmp(name, SVN_FS__PROP_TXN_CLIENT_DATE))
- return SVN_NO_ERROR;
+ if (is_internal_txn_prop(name))
+ return svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
+ _("Attempt to modify internal transaction "
+ "property '%s'"), name);
return svn_error_trace(txn->vtable->change_prop(txn, name, value, pool));
}
@@ -946,25 +968,14 @@ svn_fs_change_txn_props(svn_fs_txn_t *tx
{
int i;
- /* Silently drop attempts to modify the internal property. */
for (i = 0; i < props->nelts; ++i)
{
svn_prop_t *prop = &APR_ARRAY_IDX(props, i, svn_prop_t);
- if (!strcmp(prop->name, SVN_FS__PROP_TXN_CLIENT_DATE))
- {
- apr_array_header_t *reduced_props
- = apr_array_make(pool, props->nelts - 1, sizeof(svn_prop_t));
-
- for (i = 0; i < props->nelts; ++i)
- {
- prop = &APR_ARRAY_IDX(props, i, svn_prop_t);
- if (strcmp(prop->name, SVN_FS__PROP_TXN_CLIENT_DATE))
- APR_ARRAY_PUSH(reduced_props, svn_prop_t) = *prop;
- }
- props = reduced_props;
- break;
- }
+ if (is_internal_txn_prop(prop->name))
+ return svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
+ _("Attempt to modify internal transaction "
+ "property '%s'"), prop->name);
}
return svn_error_trace(txn->vtable->change_props(txn, props, pool));
Modified: subversion/branches/1.9.x/subversion/tests/cmdline/svnlook_tests.py
URL: http://svn.apache.org/viewvc/subversion/branches/1.9.x/subversion/tests/cmdline/svnlook_tests.py?rev=1667837&r1=1667836&r2=1667837&view=diff
==============================================================================
--- subversion/branches/1.9.x/subversion/tests/cmdline/svnlook_tests.py (original)
+++ subversion/branches/1.9.x/subversion/tests/cmdline/svnlook_tests.py Thu Mar 19 18:24:00 2015
@@ -695,7 +695,6 @@ fp.close()"""
"Properties on '/A':\n",
' bogus_prop\n',
' svn:log\n', ' svn:author\n',
- ' svn:check-locks\n', # internal prop, not really expected
' bogus_rev_prop\n',
' svn:date\n',
' svn:txn-client-compat-version\n',
Modified: subversion/branches/1.9.x/subversion/tests/libsvn_fs/fs-test.c
URL: http://svn.apache.org/viewvc/subversion/branches/1.9.x/subversion/tests/libsvn_fs/fs-test.c?rev=1667837&r1=1667836&r2=1667837&view=diff
==============================================================================
--- subversion/branches/1.9.x/subversion/tests/libsvn_fs/fs-test.c (original)
+++ subversion/branches/1.9.x/subversion/tests/libsvn_fs/fs-test.c Thu Mar 19 18:24:00 2015
@@ -5208,29 +5208,6 @@ commit_timestamp(const svn_test_opts_t *
/* Commit that overwrites the specified svn:date. */
SVN_ERR(svn_fs_begin_txn(&txn, fs, rev, pool));
- {
- /* Setting the internal property doesn't enable svn:date behaviour. */
- apr_array_header_t *props = apr_array_make(pool, 3, sizeof(svn_prop_t));
- svn_prop_t prop, other_prop1, other_prop2;
- svn_string_t *val;
-
- prop.name = SVN_FS__PROP_TXN_CLIENT_DATE;
- prop.value = svn_string_create("1", pool);
- other_prop1.name = "foo";
- other_prop1.value = svn_string_create("fooval", pool);
- other_prop2.name = "bar";
- other_prop2.value = svn_string_create("barval", pool);
- APR_ARRAY_PUSH(props, svn_prop_t) = other_prop1;
- APR_ARRAY_PUSH(props, svn_prop_t) = prop;
- APR_ARRAY_PUSH(props, svn_prop_t) = other_prop2;
- SVN_ERR(svn_fs_change_txn_props(txn, props, pool));
- SVN_ERR(svn_fs_txn_prop(&val, txn, other_prop1.name, pool));
- SVN_TEST_ASSERT(val && !strcmp(val->data, other_prop1.value->data));
- SVN_ERR(svn_fs_txn_prop(&val, txn, other_prop2.name, pool));
- SVN_TEST_ASSERT(val && !strcmp(val->data, other_prop2.value->data));
-
- SVN_ERR(svn_fs_change_txn_prop(txn, prop.name, prop.value, pool));
- }
SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
SVN_ERR(svn_fs_make_dir(txn_root, "/bar", pool));
SVN_ERR(svn_fs_change_txn_prop(txn, SVN_PROP_REVISION_DATE, date, pool));
@@ -6741,6 +6718,70 @@ test_prop_and_text_rep_sharing_collision
return SVN_NO_ERROR;
}
+static svn_error_t *
+test_internal_txn_props(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ svn_fs_t *fs;
+ svn_fs_txn_t *txn;
+ svn_string_t *val;
+ svn_prop_t prop;
+ svn_prop_t internal_prop;
+ apr_array_header_t *props;
+ apr_hash_t *proplist;
+ svn_error_t *err;
+
+ SVN_ERR(svn_test__create_fs(&fs, "test-repo-internal-txn-props",
+ opts, pool));
+ SVN_ERR(svn_fs_begin_txn2(&txn, fs, 0,
+ SVN_FS_TXN_CHECK_LOCKS |
+ SVN_FS_TXN_CHECK_OOD |
+ SVN_FS_TXN_CLIENT_DATE, pool));
+
+ /* Ensure that we cannot read internal transaction properties. */
+ SVN_ERR(svn_fs_txn_prop(&val, txn, SVN_FS__PROP_TXN_CHECK_LOCKS, pool));
+ SVN_TEST_ASSERT(!val);
+ SVN_ERR(svn_fs_txn_prop(&val, txn, SVN_FS__PROP_TXN_CHECK_OOD, pool));
+ SVN_TEST_ASSERT(!val);
+ SVN_ERR(svn_fs_txn_prop(&val, txn, SVN_FS__PROP_TXN_CLIENT_DATE, pool));
+ SVN_TEST_ASSERT(!val);
+
+ SVN_ERR(svn_fs_txn_proplist(&proplist, txn, pool));
+ SVN_TEST_ASSERT(apr_hash_count(proplist) == 1);
+ val = svn_hash_gets(proplist, SVN_PROP_REVISION_DATE);
+ SVN_TEST_ASSERT(val);
+
+ /* We also cannot change or discard them. */
+ val = svn_string_create("Ooops!", pool);
+
+ err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CHECK_LOCKS, val, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+ err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CHECK_LOCKS, NULL, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+ err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CHECK_OOD, val, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+ err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CHECK_OOD, NULL, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+ err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CLIENT_DATE, val, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+ err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CLIENT_DATE, NULL, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+
+ prop.name = "foo";
+ prop.value = svn_string_create("bar", pool);
+ internal_prop.name = SVN_FS__PROP_TXN_CHECK_LOCKS;
+ internal_prop.value = svn_string_create("Ooops!", pool);
+
+ props = apr_array_make(pool, 2, sizeof(svn_prop_t));
+ APR_ARRAY_PUSH(props, svn_prop_t) = prop;
+ APR_ARRAY_PUSH(props, svn_prop_t) = internal_prop;
+
+ err = svn_fs_change_txn_props(txn, props, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+
+ return SVN_NO_ERROR;
+}
+
/* ------------------------------------------------------------------------ */
/* The test table. */
@@ -6872,6 +6913,8 @@ static struct svn_test_descriptor_t test
"test modify txn being written in FSFS"),
SVN_TEST_OPTS_PASS(test_prop_and_text_rep_sharing_collision,
"test property and text rep-sharing collision"),
+ SVN_TEST_OPTS_PASS(test_internal_txn_props,
+ "test setting and getting internal txn props"),
SVN_TEST_NULL
};