You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Ignaz Birnstingl <ig...@gmail.com> on 2011/08/19 14:15:48 UTC
[PATCH] apr_pstrdup value returned by apr_env_get
apr_env_get should pstrdup the value it returns - as it does according
to its documentation.
Index: misc/unix/env.c
===================================================================
--- misc/unix/env.c (revision 1159605)
+++ misc/unix/env.c (working copy)
@@ -37,7 +37,7 @@
char *val = getenv(envvar);
if (!val)
return APR_ENOENT;
- *value = val;
+ *value = apr_pstrdup(pool, val);
return APR_SUCCESS;
#else
--
Ignaz
Re: Fwd: [PATCH] apr_pstrdup value returned by apr_env_get
Posted by Ignaz Birnstingl <ig...@gmail.com>.
Bojan,
I'm totally aware of that and that's what I said in previous mails.
However there is IMHO no way to make this API thread-safe since it
uses a non-thread-safe API in the first place. Suppose we would use
internal serialization by using an APR mutex inside all APR
environment functions, that would still not prevent some other thread
from directly calling C's getenv().
--
Ignaz
2011/9/13 Bojan Smojver <bo...@rexursive.com>:
> ------- Original message -------
>>
>> From: Ignaz Birnstingl <ig...@gmail.com>
>
>> Now if I use APR
>> (pseudo code)
>> char *foo = apr_env_get("foo");
>> char *bar = apr_env_get("bar");
>> if apr_env_get would pstrdup the string returned by getenv I could
>> compare foo with bar, like this if (strcmp(foo, bar) == 0) ...
>
> That is not necessarily true either. Suppose another thread called
> getenv("foo") just after getenv() and before strdup() in apr_env_get() of
> your thread. You would then get "foo" in your copy as well, because you'd be
> copying that same static buffer.
>
> --
> Bojan
Re: Fwd: [PATCH] apr_pstrdup value returned by apr_env_get
Posted by Bojan Smojver <bo...@rexursive.com>.
------- Original message -------
> From: Ignaz Birnstingl <ig...@gmail.com>
>Now if I use APR
> (pseudo code)
> char *foo = apr_env_get("foo");
> char *bar = apr_env_get("bar");
> if apr_env_get would pstrdup the string returned by getenv I could
> compare foo with bar, like this if (strcmp(foo, bar) == 0) ...
That is not necessarily true either. Suppose another thread called
getenv("foo") just after getenv() and before strdup() in apr_env_get() of
your thread. You would then get "foo" in your copy as well, because you'd
be copying that same static buffer.
--
Bojan
Re: Fwd: [PATCH] apr_pstrdup value returned by apr_env_get
Posted by Ignaz Birnstingl <ig...@gmail.com>.
I have to apologize, I confused the English terms.
I meant that getenv returns a pointer to static storage on some
platforms like the z/OS USS and calling it twice from the same thread
returns the same pointer. For instance, suppose the environment
variables foo and bar are both set, then after
char *foo = getenv("foo");
char *bar = getenv("bar");
foo will always be the same as bar (i.e. foo == bar). Now if I use APR
(pseudo code)
char *foo = apr_env_get("foo");
char *bar = apr_env_get("bar");
if apr_env_get would pstrdup the string returned by getenv I could
compare foo with bar, like this if (strcmp(foo, bar) == 0) ...
Sorry again for my ignorance, I should have looked up the terms first.
--
Ignaz
2011/9/12 Ignaz Birnstingl <ig...@gmail.com>:
>> [un]setenv/putenv are never thread safe. You are confusing the issues,
>> if the environment is volatile after threads are created the code is
>> broken.
> You are right, they are never thread-safe. However if apr_env_get
> would pstrdup the returned value that would at least make it
> reentrant.
>
>> Your 'fix' has a race condition and is therefore not the solution.
> That's what I meant with "the documentation should probably state that
> the function is not thread-safe and requires external serialization."
> But at second thought thread-safety is documented nowhere for APR so
> this can be omitted.
>
Re: Fwd: [PATCH] apr_pstrdup value returned by apr_env_get
Posted by Ignaz Birnstingl <ig...@gmail.com>.
> [un]setenv/putenv are never thread safe. You are confusing the issues,
> if the environment is volatile after threads are created the code is
> broken.
You are right, they are never thread-safe. However if apr_env_get
would pstrdup the returned value that would at least make it
reentrant.
> Your 'fix' has a race condition and is therefore not the solution.
That's what I meant with "the documentation should probably state that
the function is not thread-safe and requires external serialization."
But at second thought thread-safety is documented nowhere for APR so
this can be omitted.
Re: Fwd: [PATCH] apr_pstrdup value returned by apr_env_get
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 9/12/2011 2:38 AM, Ignaz Birnstingl wrote:
> I guess the patch didn't make it to trunk so I'll restate why I think
> this should make it to future versions of APR:
> -) The documentation says about argument pool: "where to allocate
> value and any temporary storage from". If *value is not allocated from
> pool then manipulating the data pointed to it can have any kinds of
> side-effects.
> -) The single UNIX specification states that getenv() "need not be
> reentrant" and in fact it is not on some platforms, like the z/OS USS.
> Thus apr_env_get is not reentrant on some platforms. Even with my
> patch the documentation should probably state that the function is not
> thread-safe and requires external serialization.
[un]setenv/putenv are never thread safe. You are confusing the issues,
if the environment is volatile after threads are created the code is
broken.
Your 'fix' has a race condition and is therefore not the solution.
Fwd: [PATCH] apr_pstrdup value returned by apr_env_get
Posted by Ignaz Birnstingl <ig...@gmail.com>.
I guess the patch didn't make it to trunk so I'll restate why I think
this should make it to future versions of APR:
-) The documentation says about argument pool: "where to allocate
value and any temporary storage from". If *value is not allocated from
pool then manipulating the data pointed to it can have any kinds of
side-effects.
-) The single UNIX specification states that getenv() "need not be
reentrant" and in fact it is not on some platforms, like the z/OS USS.
Thus apr_env_get is not reentrant on some platforms. Even with my
patch the documentation should probably state that the function is not
thread-safe and requires external serialization.
Thanks,
Ignaz
---------- Forwarded message ----------
From: Ignaz Birnstingl <ig...@gmail.com>
Date: 2011/8/19
Subject: [PATCH] apr_pstrdup value returned by apr_env_get
To: dev@apr.apache.org
apr_env_get should pstrdup the value it returns - as it does according
to its documentation.
Index: misc/unix/env.c
===================================================================
--- misc/unix/env.c (revision 1159605)
+++ misc/unix/env.c (working copy)
@@ -37,7 +37,7 @@
char *val = getenv(envvar);
if (!val)
return APR_ENOENT;
- *value = val;
+ *value = apr_pstrdup(pool, val);
return APR_SUCCESS;
#else
--
Ignaz