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 };