Return-Path: X-Original-To: apmail-subversion-commits-archive@minotaur.apache.org Delivered-To: apmail-subversion-commits-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 1308B10A15 for ; Fri, 1 Nov 2013 23:14:00 +0000 (UTC) Received: (qmail 15995 invoked by uid 500); 1 Nov 2013 23:13:59 -0000 Delivered-To: apmail-subversion-commits-archive@subversion.apache.org Received: (qmail 15976 invoked by uid 500); 1 Nov 2013 23:13:59 -0000 Mailing-List: contact commits-help@subversion.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@subversion.apache.org Delivered-To: mailing list commits@subversion.apache.org Received: (qmail 15969 invoked by uid 99); 1 Nov 2013 23:13:59 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 01 Nov 2013 23:13:59 +0000 X-ASF-Spam-Status: No, hits=-2000.0 required=5.0 tests=ALL_TRUSTED X-Spam-Check-By: apache.org Received: from [140.211.11.4] (HELO eris.apache.org) (140.211.11.4) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 01 Nov 2013 23:13:58 +0000 Received: from eris.apache.org (localhost [127.0.0.1]) by eris.apache.org (Postfix) with ESMTP id 46DE72388831; Fri, 1 Nov 2013 23:13:38 +0000 (UTC) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: svn commit: r1538083 - in /subversion/trunk/subversion: include/private/svn_subr_private.h libsvn_repos/config_pool.c libsvn_subr/config_file.c tests/libsvn_repos/repos-test.c Date: Fri, 01 Nov 2013 23:13:38 -0000 To: commits@subversion.apache.org From: stefan2@apache.org X-Mailer: svnmailer-1.0.9 Message-Id: <20131101231338.46DE72388831@eris.apache.org> X-Virus-Checked: Checked by ClamAV on apache.org Author: stefan2 Date: Fri Nov 1 23:13:37 2013 New Revision: 1538083 URL: http://svn.apache.org/r1538083 Log: Our svn_config_t struct contains tempoary string buffers that will be modified even in r/o access mode. Thus, it is inherently no thead-safe. Introduce a private API function that will create shallow copies, i.e. share the expensive config tree but use separate temp buffers. * subversion/include/private/svn_subr_private.h (svn_config__shallow_copy): declare new private API function * subversion/libsvn_subr/config_file.c (svn_config__shallow_copy): implement * subversion/libsvn_repos/config_pool.c (return_config_ref): return shallow copies to ensure the config can be used safely from multiple threads * subversion/tests/libsvn_repos/repos-test.c (test_config_pool): since the config pool always returns a new root struct we need to check whether the actual config tree is the same Modified: subversion/trunk/subversion/include/private/svn_subr_private.h subversion/trunk/subversion/libsvn_repos/config_pool.c subversion/trunk/subversion/libsvn_subr/config_file.c subversion/trunk/subversion/tests/libsvn_repos/repos-test.c Modified: subversion/trunk/subversion/include/private/svn_subr_private.h URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_subr_private.h?rev=1538083&r1=1538082&r2=1538083&view=diff ============================================================================== --- subversion/trunk/subversion/include/private/svn_subr_private.h (original) +++ subversion/trunk/subversion/include/private/svn_subr_private.h Fri Nov 1 23:13:37 2013 @@ -485,6 +485,16 @@ svn_config__is_expanded(svn_config_t *cf const char *section, const char *option); +/* Return a shallow copy of SCR in POOL. If SRC is read-only, different + * shallow copies may be used from different threads. + * + * Any single r/o svn_config_t or shallow copy is not thread-safe because + * it contains shared buffers for tempoary data. + */ +svn_config_t * +svn_config__shallow_copy(svn_config_t *src, + apr_pool_t *pool); + /** @} */ #ifdef __cplusplus Modified: subversion/trunk/subversion/libsvn_repos/config_pool.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/config_pool.c?rev=1538083&r1=1538082&r2=1538083&view=diff ============================================================================== --- subversion/trunk/subversion/libsvn_repos/config_pool.c (original) +++ subversion/trunk/subversion/libsvn_repos/config_pool.c Fri Nov 1 23:13:37 2013 @@ -227,7 +227,7 @@ return_config_ref(config_ref_t *config, svn_atomic_inc(&config->config_pool->used_config_count); apr_pool_cleanup_register(pool, config, config_ref_cleanup, apr_pool_cleanup_null); - return config->cfg; + return svn_config__shallow_copy(config->cfg, pool); } /* Set *CFG to the configuration with a parsed textual matching CHECKSUM. Modified: subversion/trunk/subversion/libsvn_subr/config_file.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/config_file.c?rev=1538083&r1=1538082&r2=1538083&view=diff ============================================================================== --- subversion/trunk/subversion/libsvn_subr/config_file.c (original) +++ subversion/trunk/subversion/libsvn_subr/config_file.c Fri Nov 1 23:13:37 2013 @@ -469,6 +469,26 @@ svn_config__is_read_only(svn_config_t *c return cfg->read_only; } +svn_config_t * +svn_config__shallow_copy(svn_config_t *src, + apr_pool_t *pool) +{ + svn_config_t *cfg = apr_palloc(pool, sizeof(*cfg)); + + cfg->sections = src->sections; + cfg->pool = pool; + + /* r/o configs are fully expanded and don't need the x_pool anymore */ + cfg->x_pool = src->read_only ? NULL : svn_pool_create(pool); + cfg->x_values = src->x_values; + cfg->tmp_key = svn_stringbuf_create_empty(pool); + cfg->tmp_value = svn_stringbuf_create_empty(pool); + cfg->section_names_case_sensitive = src->section_names_case_sensitive; + cfg->option_names_case_sensitive = src->option_names_case_sensitive; + cfg->read_only = src->read_only; + + return cfg; +} svn_error_t * Modified: subversion/trunk/subversion/tests/libsvn_repos/repos-test.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_repos/repos-test.c?rev=1538083&r1=1538082&r2=1538083&view=diff ============================================================================== --- subversion/trunk/subversion/tests/libsvn_repos/repos-test.c (original) +++ subversion/trunk/subversion/tests/libsvn_repos/repos-test.c Fri Nov 1 23:13:37 2013 @@ -39,6 +39,9 @@ #include "svn_version.h" #include "private/svn_repos_private.h" +/* be able to look into svn_config_t */ +#include "../../libsvn_subr/config_impl.h" + #include "../svn_test_fs.h" #include "dir-delta-editor.h" @@ -3311,7 +3314,8 @@ test_config_pool(const svn_test_opts_t * const char *repo_name = "test-repo-config-pool"; svn_repos_t *repos; svn_stringbuf_t *cfg_buffer1, *cfg_buffer2; - svn_config_t *cfg, *cfg1, *cfg2; + svn_config_t *cfg; + apr_hash_t *sections1, *sections2; int i; svn_boolean_t bvalue; svn_fs_txn_t *txn; @@ -3367,7 +3371,7 @@ test_config_pool(const svn_test_opts_t * /* requesting a config over and over again should return the same (even though it is not being referenced) */ - cfg1 = NULL; + sections1 = NULL; for (i = 0; i < 4; ++i) { SVN_ERR(svn_repos__config_pool_get( @@ -3377,10 +3381,10 @@ test_config_pool(const svn_test_opts_t * pool), subpool)); - if (cfg1 == NULL) - cfg1 = cfg; + if (sections1 == NULL) + sections1 = cfg->sections; else - SVN_TEST_ASSERT(cfg == cfg1); + SVN_TEST_ASSERT(cfg->sections == sections1); svn_pool_clear(subpool); } @@ -3396,13 +3400,13 @@ test_config_pool(const svn_test_opts_t * pool), subpool)); - SVN_TEST_ASSERT(cfg == cfg1); + SVN_TEST_ASSERT(cfg->sections == sections1); svn_pool_clear(subpool); } /* reading a different configuration should return a different pointer */ - cfg2 = NULL; + sections2 = NULL; for (i = 0; i < 2; ++i) { SVN_ERR(svn_repos__config_pool_get( @@ -3412,12 +3416,12 @@ test_config_pool(const svn_test_opts_t * pool), subpool)); - if (cfg2 == NULL) - cfg2 = cfg; + if (sections2 == NULL) + sections2 = cfg->sections; else - SVN_TEST_ASSERT(cfg == cfg2); + SVN_TEST_ASSERT(cfg->sections == sections2); - SVN_TEST_ASSERT(cfg1 != cfg2); + SVN_TEST_ASSERT(sections1 != sections2); svn_pool_clear(subpool); } @@ -3441,7 +3445,7 @@ test_config_pool(const svn_test_opts_t * repo_root_url, "dir/config", pool), subpool)); - SVN_TEST_ASSERT(cfg == cfg1); + SVN_TEST_ASSERT(cfg->sections == sections1); svn_pool_clear(subpool); /* create another in-repo config */ @@ -3459,7 +3463,7 @@ test_config_pool(const svn_test_opts_t * repo_root_url, "dir/config", pool), subpool)); - SVN_TEST_ASSERT(cfg == cfg2); + SVN_TEST_ASSERT(cfg->sections == sections2); svn_pool_clear(subpool); /* reading the copied config should still give cfg1 */ @@ -3469,7 +3473,7 @@ test_config_pool(const svn_test_opts_t * "another-dir/config", pool), subpool)); - SVN_TEST_ASSERT(cfg == cfg1); + SVN_TEST_ASSERT(cfg->sections == sections1); svn_pool_clear(subpool); /* once again: repeated reads. This triggers a different code path. */ @@ -3478,14 +3482,14 @@ test_config_pool(const svn_test_opts_t * repo_root_url, "dir/config", pool), subpool)); - SVN_TEST_ASSERT(cfg == cfg2); + SVN_TEST_ASSERT(cfg->sections == sections2); SVN_ERR(svn_repos__config_pool_get(&cfg, config_pool, svn_path_url_add_component2( repo_root_url, "another-dir/config", pool), subpool)); - SVN_TEST_ASSERT(cfg == cfg1); + SVN_TEST_ASSERT(cfg->sections == sections1); svn_pool_clear(subpool); /* access paths that don't exist */ @@ -3501,7 +3505,7 @@ test_config_pool(const svn_test_opts_t * /* as long as we keep a reference to a config, clearing the config pool should not invalidate that reference */ - SVN_ERR(svn_repos__config_pool_get(&cfg1, config_pool, + SVN_ERR(svn_repos__config_pool_get(&cfg, config_pool, svn_dirent_join(wrk_dir, "config-pool-test1.cfg", pool), @@ -3510,7 +3514,7 @@ test_config_pool(const svn_test_opts_t * for (i = 0; i < 64000; ++i) apr_pcalloc(config_pool_pool, 80); - SVN_ERR(svn_config_get_bool(cfg1, &bvalue, "booleans", "true3", FALSE)); + SVN_ERR(svn_config_get_bool(cfg, &bvalue, "booleans", "true3", FALSE)); SVN_TEST_ASSERT(bvalue); return SVN_NO_ERROR;