Daniel Shahaf <d.s_at_daniel.shahaf.co.il> writes:
> [[[
> Validate svn:date property values in libsvn_repos.
>
> Suggested by: sussman
>
> * subversion/libsvn_repos/fs-wrap.c
> (svn_time.h): Include.
> (validate_prop): Add optional 'value' and 'pool' parameters. Extend to
> validate the value for svn:* properties.
> (svn_repos_fs_change_node_prop, svn_repos_fs_change_txn_props,
> svn_repos_fs_change_rev_prop3):
> Pass pool and property value to validate_prop().
>
> * subversion/tests/cmdline/prop_tests.py
> (invalid_propvalues): New test.
> (test_list): Run new test.
> ]]]
>
> --- subversion/libsvn_repos/fs-wrap.c (revision 31118)
> +++ subversion/libsvn_repos/fs-wrap.c (working copy)
> @@ -146,17 +147,38 @@ svn_repos_fs_begin_txn_for_update(svn_fs_txn_t **t
> /*** Property wrappers ***/
>
> /* Validate that property NAME is valid for use in a Subversion
> - repository. */
> + repository. If given, validate VALUE as well (for "svn:" properties only).
> + Use POOL for temporary allocations.
> + */
As long as you're adjusting the doc string, you might as well say what
error is returned if the property value is not valid. Also, we're not
currently validating all "svn:" properties, only some of them -- well,
only one of them, in fact. You don't have to list them all in the doc
string, but at least it should say that validation *may* happen, rather
than that it will definitely happen.
> static svn_error_t *
> -validate_prop(const char *name)
> +validate_prop(const char *name, const svn_string_t *value, apr_pool_t *pool)
> {
> svn_prop_kind_t kind = svn_property_kind(NULL, name);
> +
> + /* Disallow setting non-regular properties. */
> if (kind != svn_prop_regular_kind)
> return svn_error_createf
> (SVN_ERR_REPOS_BAD_ARGS, NULL,
> _("Storage of non-regular property '%s' is disallowed through the "
> "repository interface, and could indicate a bug in your client"),
> name);
> +
> + /* Validate "svn:" properties. */
> + if (svn_prop_is_svn_prop(name) && value != NULL)
> + {
> + /* "svn:date" should a valid date. */
> + if (! strcmp(name, SVN_PROP_REVISION_DATE))
> + {
We generally use this idiom:
if (strcmp(name, SVN_PROP_REVISION_DATE) == 0)
(so that equality is signified by "==" rather than "!").
> + apr_time_t temp;
> + svn_error_t *err;
> +
> + err = svn_time_from_cstring(&temp, value->data, pool);
> + if (err)
> + return svn_error_create(SVN_ERR_BAD_PROPERTY_VALUE,
> + err, NULL);
> + }
> + }
> +
> return SVN_NO_ERROR;
> }
>
> @@ -169,7 +191,7 @@ svn_repos_fs_change_node_prop(svn_fs_root_t *root,
> apr_pool_t *pool)
> {
> /* Validate the property, then call the wrapped function. */
> - SVN_ERR(validate_prop(name));
> + SVN_ERR(validate_prop(name, value, pool));
> return svn_fs_change_node_prop(root, path, name, value, pool);
> }
Hmmm, doesn't this imply a change to svn_repos_fs_change_node_prop()'s
doc string? :-)
> @@ -182,7 +204,10 @@ svn_repos_fs_change_txn_props(svn_fs_txn_t *txn,
> int i;
>
> for (i = 0; i < txnprops->nelts; i++)
> - SVN_ERR(validate_prop(APR_ARRAY_IDX(txnprops, i, svn_prop_t).name));
> + {
> + svn_prop_t *prop = &APR_ARRAY_IDX(txnprops, i, svn_prop_t);
> + SVN_ERR(validate_prop(prop->name, prop->value, pool));
> + }
>
> return svn_fs_change_txn_props(txn, txnprops, pool);
> }
...and this a change to svn_repos_fs_change_txn_props()'s doc string?
> @@ -227,8 +252,9 @@ svn_repos_fs_change_rev_prop3(svn_repos_t *repos,
>
> if (readability == svn_repos_revision_access_full)
> {
> - SVN_ERR(validate_prop(name));
> + SVN_ERR(validate_prop(name, new_value, pool));
> SVN_ERR(svn_fs_revision_prop(&old_value, repos->fs, rev, name, pool));
> +
> if (! new_value)
> action = 'D';
> else if (! old_value)
...and this a change to svn_repos_fs_change_rev_prop3()'s doc string?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-12 04:57:33 CEST