Neels Janosch Hofmeyr wrote on Tue, 3 Jun 2008 at 03:48 +0200:
> There was a discussion on issue 1796 up to a week ago, in which a bunch
> of shortcomings of the same kind as 1796 have been revealed:
>
> The subversion server and client do not validate props in places where
> they should:
> - where the server receives props from a client out there. (#1796)
> - where the server reads props from the repository file system.
> - where the svn client reads props from a server out there.
> (Approval by kfogel)
>
> This patch starts by fixing the specific problems of issue 1796, only:
> - where the server receives props from a client out there. (#1796)
> , and limited only to the log message prop (SVN_PROP_REVISION_LOG).
>
> More patches, continuing in above list, are to follow.
>
> Also, in the threads about issue 1796 recently, some people asked for a
> way to reproduce 1796 without forging their svn client. Note that the C
> test included in this patch is a good way to do so. It may be
> illustrative to investigate the repository after the test has run, using
> current trunk: the corrupt data shows in the repository filesystem.
>
...
> Also note that this is my first "complex" patch to subversion,
It's also one of the first non-trivial patches I review.
> so please
> feel very free to tell me about anything I could have done better.
>
> Thanks!
>
>
> [[[
> Fix issue #1796: defective or malicious client can corrupt repository
> log messages.
> Also adding regression test for 1796.
^
1796 what?
>
> * subversion/include/private/svn_utf_private.h: Add this private header
> file and move the declaration of svn_utf__is_valid from
> libsvn_subr/utf_impl.h here, because this function is needed in
> libsvn_repos.
>
svn_utf__is_valid should be mentioned using the (symbol_name): syntax.
> * subversion/libsvn_subr/utf_impl.h: Include private/svn_utf_private.h.
> (svn_utf__is_valid): Move declaration away to svn_utf_private.h
> because this function is needed in libsvn_repos.
> (svn_utf__last_valid): Add comment to also see svn_utf__is_valid.
>
> * subversion/libsvn_repos/fs-wrap.c(validate_prop): Add two validations
Space before parenthesis.
> for SVN_PROP_REVISION_LOG's value. Validate UTF-8 encoding using
> svn_utf__is_valid, and validate consistent LF eol style by looking
> for and rejecting CR (\r) characters.
>
> * subversion/tests/libsvn_repos/repos-test.c
> (prop_validation): Add this regression test for issue 1796, which
> tries to commit two invalid log messages concerning UTF-8 and LF.
> (prop_validation_commit_with_revprop): Add this helper function for
> prop_validation, which runs a commit with a given revprop.
>
> Patch by: Neels Janosch Hofmeyr <neels_at_elego.de>
> Review by: Karl Fogel <kfogel_at_red-bean.com>
> Daniel Shahaf <d.s_at_daniel.shahaf.co.il>
> Stefan Sperling <stsp_at_elego.de>
> Branko Čibej <brane_at_xbc.nu>
You should list committers by their canonical usernames from HACKING:
kfogel, danielsh, stsp, etc. But since you haven't posted a version of
this patch before, it is inappropriate to list all these people as
reviewers: they haven't reviewed this patch. If you want to credit
the other people than direct reviewers, a parenthetical might work:
Review by: jrandom
(and here you mention jconstant)
> Found by: garrick_olson
According to HACKING, mentioning the issue number is sufficient. Not
that it hurts to credit someone :)
> ]]]
>
>
> Index: subversion/libsvn_repos/fs-wrap.c
> ===================================================================
> --- subversion/libsvn_repos/fs-wrap.c (revision 31559)
> +++ subversion/libsvn_repos/fs-wrap.c (working copy)
> @@ -28,6 +28,7 @@
> #include "svn_time.h"
> #include "repos.h"
> #include "svn_private_config.h"
> +#include "private/svn_utf_private.h"
>
>
> /*** Commit wrappers ***/
> @@ -169,6 +170,25 @@
> /* Validate "svn:" properties. */
> if (svn_prop_is_svn_prop(name) && value != NULL)
> {
> + /* validate log message (UTF-8/LF) */
> + if (strcmp(name, SVN_PROP_REVISION_LOG) == 0)
> + {
> + /* validate encoding of log message (UTF-8) */
I'd probably capitalize these two comments and make them complete
sentences.
> + if (svn_utf__is_valid(value->data, value->len) == FALSE)
> + return svn_error_create
> + (APR_EINVAL, NULL,
> + _("Cannot accept log message because it is not encoded in "
> + "UTF-8"));
> +
More specific error code (SVN_ERR_INVALID_UTF_8)?
> + /* Disallow inconsistent line ending style, by simply looking for
> + * carriage return characters ('\r'). */
> + if (strstr(value->data, "\r") != NULL)
strchr?
> + return svn_error_create
> + (SVN_ERR_IO_INCONSISTENT_EOL, NULL,
> + _("Cannot accept non-LF line endings "
> + "in log message"));
> + }
> +
Please also update svn_repos_fs_change_node_prop's docstring, which
claims that only SVN_PROP_REVISION_DATE is validated.
Hmm, that docstring (and validate_prop()'s docstring) also promise that
you'll return SVN_ERR_BAD_PROPERTY_VALUE. So you need to wrap your
errors by that error.
> /* "svn:date" should be a valid date. */
> if (strcmp(name, SVN_PROP_REVISION_DATE) == 0)
> {
> Index: subversion/tests/libsvn_repos/repos-test.c
> ===================================================================
> --- subversion/tests/libsvn_repos/repos-test.c (revision 31559)
> +++ subversion/tests/libsvn_repos/repos-test.c (working copy)
> @@ -2187,6 +2187,144 @@
>
>
>
> +/* Test if prop values received by the server are validated.
> + * These tests "send" property values to the server and diagnose the
> + * behaviour.
> + */
> +
> +/* Helper function that makes an arbitrary change to a repository and runs
> + * a commit with a specific revision property set to a certain value.
Should say that the property's name and value are obtained from PROP_KEY
and PROP_VAL.
> + * The filename argument names a file in the test repository to add in
In internal docstrings, capitalize parameter names, so s/filename/FILENAME/.
(Without that, when you say "filename" unquoted it is not obvious that
you're using it as a proper noun -- as a parameter name.)
> + * this commit, e.g. "/A/B/should_fail_1".
> + * If you run this function multiple times on the same repository,
> + * make sure to use differing filename arguments
Why?
> + * (e.g. increment a
> + * number every time you call this helper, as in "/A/B/should_fail_2",
> + * "/A/B/should_fail_3" etc)
> + */
> +static svn_error_t *
> +prop_validation_commit_with_revprop(const char *filename,
> + const void *prop_key,
> + apr_ssize_t prop_klen,
> + const void *prop_val,
> + svn_repos_t *repos,
Why are they "const void *"?
> + apr_pool_t *pool)
> +{
> + const svn_delta_editor_t *editor;
> + void *edit_baton;
> + void *root_baton;
> + void *file_baton;
> +
> + /* Prepare revision properties */
> + apr_hash_t *revprop_table = apr_hash_make(pool);
> +
> + /* Add the requested property */
> + apr_hash_set(revprop_table, prop_key, prop_klen, prop_val);
> +
> + /* Set usual author and log props, if not set already */
> + if (strcmp((const char*)prop_key, SVN_PROP_REVISION_AUTHOR) != 0)
Normally I'd suggest defining a local const char * variable and use it
(see how we normally use apr_hash_get), but see above.
> + {
> + apr_hash_set(revprop_table, SVN_PROP_REVISION_AUTHOR,
> + APR_HASH_KEY_STRING,
> + svn_string_create("plato", pool));
> + }
> +
> + if (strcmp((const char*)prop_key, SVN_PROP_REVISION_LOG) != 0)
> + {
> + apr_hash_set(revprop_table, SVN_PROP_REVISION_LOG,
> + APR_HASH_KEY_STRING,
> + svn_string_create("revision log", pool));
> + }
> +
> + /* Make an arbitrary change and commit using above values... */
> +
> + SVN_ERR(svn_repos_get_commit_editor5(&editor, &edit_baton, repos,
> + NULL, "file://test", "/",
> + revprop_table, NULL, NULL,
> + NULL, NULL, pool));
> +
> + SVN_ERR(editor->open_root(edit_baton, 1, pool, &root_baton));
^
Should this be 1 or 0? I'm not familiar with the editor or the C tests
infrastructure, but based on the docstrings I think this should be zero?
(I understand the code works, but I'd like to understand why.)
> +
> + SVN_ERR(editor->add_file( filename , root_baton, NULL,
^ ^
s/ //
> + SVN_INVALID_REVNUM, pool,
> + &file_baton));
> +
Start an edit but not finish it? (close_file, close_edit...) Why?
> + return SVN_NO_ERROR;
> +}
> +
> +
> +/* Expect failure of invalid commit in these cases:
> + * - log message contains invalid UTF-8 octet (issue 1796)
> + * - log message contains invalid linefeed style (non-LF) (issue 1796)
> + */
> +static svn_error_t *
> +prop_validation(const char **msg,
> + svn_boolean_t msg_only,
> + svn_test_opts_t *opts,
> + apr_pool_t *pool)
> +{
> + svn_error_t *err;
> + svn_repos_t *repos;
> + svn_fs_t *fs;
^^
Unused local variable.
> + const char non_utf8_string[5] = { 'a', 0xff, 'b', '\n', 0 };
> + const char *non_lf_string = "a\r\nb\n\rc\rd\n";
> + apr_pool_t *subpool = svn_pool_create(pool);
> +
> + *msg = "test if revprops are validated by repos";
> +
> + if (msg_only)
> + return SVN_NO_ERROR;
> +
> + /* Create a filesystem and repository. */
> + SVN_ERR(svn_test__create_repos(&repos, "test-repo-prop-validation",
> + opts, subpool));
> + fs = svn_repos_fs(repos);
> +
> +
> + /* Test an invalid commit log message: UTF-8 */
> + err = prop_validation_commit_with_revprop
> + ("/non_utf8_log_msg",
> + SVN_PROP_REVISION_LOG, APR_HASH_KEY_STRING,
> + svn_string_create(non_utf8_string, subpool),
> + repos, subpool);
> +
> + if (err == SVN_NO_ERROR)
> + return svn_error_create(SVN_ERR_TEST_FAILED, err,
> + "Failed to reject a log with invalid "
> + "UTF-8");
> + else
> + if (err->apr_err != APR_EINVAL)
> + return svn_error_create(SVN_ERR_TEST_FAILED, err,
> + "Expected APR_EINVAL, got another error.");
> + svn_error_clear(err);
> +
> +
> + /* Test an invalid commit log message: LF */
> + err = prop_validation_commit_with_revprop
> + ("/non_lf_log_msg",
> + SVN_PROP_REVISION_LOG, APR_HASH_KEY_STRING,
> + svn_string_create(non_lf_string, subpool),
> + repos, subpool);
> +
> + if (err == SVN_NO_ERROR)
> + return svn_error_create(SVN_ERR_TEST_FAILED, err,
> + "Failed to reject a log with inconsistent "
> + "line ending style");
> + else
> + if (err->apr_err != SVN_ERR_IO_INCONSISTENT_EOL)
> + return svn_error_create(SVN_ERR_TEST_FAILED, err,
> + "Expected SVN_ERR_IO_INCONSISTENT_EOL, "
> + "got another error.");
> + svn_error_clear(err);
> +
So svn_error_clear() runs for two paths: on the "correct" error and on
SVN_NO_ERROR.
> +
> + /* Done. */
> + svn_pool_destroy(subpool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> +
> /* The test table. */
>
> struct svn_test_descriptor_t test_funcs[] =
> @@ -2203,5 +2341,6 @@
> SVN_TEST_PASS(commit_continue_txn),
> SVN_TEST_PASS(node_location_segments),
> SVN_TEST_PASS(reporter_depth_exclude),
> + SVN_TEST_PASS(prop_validation),
> SVN_TEST_NULL
> };
>
Looks good.
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-03 08:23:23 CEST