You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2008/03/26 03:40:44 UTC
Is svn_config_get() not thread-safe?
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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Is svn_config_get() not thread-safe?
Posted by Branko Čibej <br...@xbc.nu>.
Karl Fogel wrote:
> "David Glasser" <gl...@davidglasser.net> writes:
>
>>> 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.
>>>
>> Well, specifically it's "the next call to svn_config_get() with the
>> same svn_config_t". Does that make it any better?
>>
>
> Well, the doc string should state that too, but no, it doesn't nmake the
> problem go away. It remains the case that there is no thread-safe way
> to extract a value -- two different threads could have the same config
> object, there's no rule against that (nor should there be).
>
Yes there should be. We've been extremely lax in specifying our thread
isolation level, except for stating that our API is "thread-safe". AFAIK
we explicitly state that two threads can't use the same top-level APR
pool at the same time to call our APIs. An svn_config_t is "just" a
wrapper for a pool in this respect.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Is svn_config_get() not thread-safe?
Posted by Karl Fogel <kf...@red-bean.com>.
"David Glasser" <gl...@davidglasser.net> writes:
>> 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.
>
> Well, specifically it's "the next call to svn_config_get() with the
> same svn_config_t". Does that make it any better?
Well, the doc string should state that too, but no, it doesn't nmake the
problem go away. It remains the case that there is no thread-safe way
to extract a value -- two different threads could have the same config
object, there's no rule against that (nor should there be).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Is svn_config_get() not thread-safe?
Posted by David Glasser <gl...@davidglasser.net>.
On Tue, Mar 25, 2008 at 8:40 PM, Karl Fogel <kf...@red-bean.com> wrote:
> 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.
Well, specifically it's "the next call to svn_config_get() with the
same svn_config_t". Does that make it any better?
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org