[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Is svn_config_get() not thread-safe?

From: Karl Fogel <kfogel_at_red-bean.com>
Date: Tue, 25 Mar 2008 23:40:44 -0400

Read the third paragraph of this doc string and tell me there isn't
something wrong here:

   /** Find the value of a (@a section, @a option) pair in @a cfg, set @a
    * *valuep to the value.
    *
    * If @a cfg is @c NULL, just sets @a *valuep to @a default_value. If
    * the value does not exist, expand and return @a default_value. @a
    * default_value can be NULL.
    *
    * The returned value will be valid at least until the next call to
    * svn_config_get(), or for the lifetime of @a default_value. It is
    * safest to consume the returned value immediately.
    *
    * This function may change @a cfg by expanding option values.
    */
   void svn_config_get(svn_config_t *cfg, const char **valuep,
                       const char *section, const char *option,
                       const char *default_value);

The implementation bears out the doc string's claim. (See the code
below, with notes from me marked with "###".) But this is bogosity:
another thread could call svn_config_get() and wipe out *valuep before
the first caller ever has a chance to "consume" (e.g., copy) the value.

Do we need svn_config_get2()?

   void
   svn_config_get(svn_config_t *cfg, const char **valuep,
                  const char *section, const char *option,
                  const char *default_value)
   {
     if (cfg)
       {
         cfg_section_t *sec;
         cfg_option_t *opt = find_option(cfg, section, option, &sec);
         if (opt != NULL)
           {
             make_string_from_option(valuep, cfg, sec, opt, NULL);
             ### Okay, if you trace into make_string_from_option()
             ### you'll see some tricky pool dances, but ultimately
             ### everything works out and *valuep is safe and
             ### permanent -- which means the lifetime caveat given in
             ### svn_config_get()'s doc string is unnecessary for this
             ### case. Now let's look at the opt == NULL case below...
           }
         else
           {
             apr_pool_t *tmp_pool = svn_pool_create(cfg->x_pool);
             const char *x_default;
             expand_option_value(cfg, sec, default_value, &x_default, tmp_pool);
             if (x_default)
               {
                 svn_stringbuf_set(cfg->tmp_value, x_default);
                 *valuep = cfg->tmp_value->data;
                 ### Ah, this is where the lifetime caveat comes into
                 ### play. Even though x_default is allocated in
                 ### tmp_pool (which is about to be destroyed), we then
                 ### memcpy it into cfg->tmp_value and thence to *valuep.
                 ### Thus the data is safe until the next call to
                 ### svn_config_get(), just as documented.
               }
             else
               *valuep = default_value;
             svn_pool_destroy(tmp_pool);
           }
       }
     else
       {
         *valuep = default_value;
       }
   }

-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-03-26 04:40:55 CET

This is an archived mail posted to the Subversion Dev mailing list.