You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2012/02/06 16:46:54 UTC
svn commit: r1241050 - /subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Author: stsp
Date: Mon Feb 6 15:46:54 2012
New Revision: 1241050
URL: http://svn.apache.org/viewvc?rev=1241050&view=rev
Log:
* subversion/mod_dav_svn/mod_dav_svn.c
(SVNHooksEnv_cmd): Handle environment variables with values containing '='.
While here, dup strings referenced from the hash table into the hash
table's pool for safety.
Modified:
subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=1241050&r1=1241049&r2=1241050&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
+++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Mon Feb 6 15:46:54 2012
@@ -538,14 +538,36 @@ SVNHooksEnv_cmd(cmd_parms *cmd, void *co
apr_array_header_t *var;
var = svn_cstring_split(arg1, "=", TRUE, cmd->pool);
- if (var && var->nelts == 2)
+ if (var && var->nelts >= 2)
{
dir_conf_t *conf = config;
+ const char *name;
+ const char *val;
- apr_hash_set(conf->hooks_env,
- APR_ARRAY_IDX(var, 0, const char *),
- APR_HASH_KEY_STRING,
- APR_ARRAY_IDX(var, 1, const char *));
+ name = apr_pstrdup(apr_hash_pool_get(conf->hooks_env),
+ APR_ARRAY_IDX(var, 0, const char *));
+
+ /* Special case for values which contain '='. */
+ if (var->nelts > 2)
+ {
+ svn_stringbuf_t *buf;
+ int i;
+
+ buf = svn_stringbuf_create(APR_ARRAY_IDX(var, 1, const char *),
+ cmd->pool);
+ for (i = 2; i < var->nelts; i++)
+ {
+ svn_stringbuf_appendbyte(buf, '=');
+ svn_stringbuf_appendcstr(buf, APR_ARRAY_IDX(var, i, const char *));
+ }
+
+ val = apr_pstrdup(apr_hash_pool_get(conf->hooks_env), buf->data);
+ }
+ else
+ val = apr_pstrdup(apr_hash_pool_get(conf->hooks_env),
+ APR_ARRAY_IDX(var, 1, const char *));
+
+ apr_hash_set(conf->hooks_env, name, APR_HASH_KEY_STRING, val);
}
return NULL;
Re: svn commit: r1241050 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 06, 2012 at 12:39:27PM +0000, Julian Foad wrote:
> Daniel Shahaf wrote:
>
> > Daniel Shahaf wrote on Mon, Feb 06, 2012 at 18:20:28 +0200:
> >> Stefan Sperling wrote on Mon, Feb 06, 2012 at 17:12:32 +0100:
> >> > On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
> >> > > This still strips whitespace around ='s in the value:
> >> > > SVNHooksEnv "name = x = y"
> >> > > will result in
> >> > > setenv("name", "x=y", 1)
> >> > > whereas I believe it should result in
> >> > > setenv("name", "x = y", 1)
> >> > > (and, to be honest, I'd be happy with
> >> > > setenv("name ", " x = y", 1)
> >> > > as well).
> >> > >
> >> > > WDYT? How should it behave?
> >> >
> >> > I agree.
> >> > would telling svn_cstring_split() to no strip whitespace suffice?
> >>
> >> I assume that should result in the third setenv() case above, so +1.
> >
> > Ping? trunk@HEAD still strips whitespace around equal signs in the value.
>
> My tuppence-worth? I agree that the current behaviour as stated above is wrong. Unless there is precedent to the contrary, I think it should do no stripping at all. If you can find precedent for some stripping in such a setting, then follow the precedent. Note that it's not only possible to strip spaces before and/or after the first '=' character, but also before "name" and/or after "x = y".
This option string is parsed by HTTPD.
The API provided by HTTPD is quite bad for this use case.
This is work in progress, and the above behaviour will be changed.
See http://subversion.tigris.org/issues/show_bug.cgi?id=4113
In the long term, the argument to the SVNHooksEnv option will be changed
anyway. It will become a path to a file which contains a list of environment
variables and their values. That file will then be parsed by our own config
parsing code.
Re: svn commit: r1241050 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 06, 2012 at 12:39:27PM +0000, Julian Foad wrote:
> Daniel Shahaf wrote:
>
> > Daniel Shahaf wrote on Mon, Feb 06, 2012 at 18:20:28 +0200:
> >> Stefan Sperling wrote on Mon, Feb 06, 2012 at 17:12:32 +0100:
> >> > On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
> >> > > This still strips whitespace around ='s in the value:
> >> > > SVNHooksEnv "name = x = y"
> >> > > will result in
> >> > > setenv("name", "x=y", 1)
> >> > > whereas I believe it should result in
> >> > > setenv("name", "x = y", 1)
> >> > > (and, to be honest, I'd be happy with
> >> > > setenv("name ", " x = y", 1)
> >> > > as well).
> >> > >
> >> > > WDYT? How should it behave?
> >> >
> >> > I agree.
> >> > would telling svn_cstring_split() to no strip whitespace suffice?
> >>
> >> I assume that should result in the third setenv() case above, so +1.
> >
> > Ping? trunk@HEAD still strips whitespace around equal signs in the value.
>
> My tuppence-worth? I agree that the current behaviour as stated above is wrong. Unless there is precedent to the contrary, I think it should do no stripping at all. If you can find precedent for some stripping in such a setting, then follow the precedent. Note that it's not only possible to strip spaces before and/or after the first '=' character, but also before "name" and/or after "x = y".
This option string is parsed by HTTPD.
The API provided by HTTPD is quite bad for this use case.
This is work in progress, and the above behaviour will be changed.
See http://subversion.tigris.org/issues/show_bug.cgi?id=4113
In the long term, the argument to the SVNHooksEnv option will be changed
anyway. It will become a path to a file which contains a list of environment
variables and their values. That file will then be parsed by our own config
parsing code.
Re: svn commit: r1241050 - /subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Shahaf wrote:
> Daniel Shahaf wrote on Mon, Feb 06, 2012 at 18:20:28 +0200:
>> Stefan Sperling wrote on Mon, Feb 06, 2012 at 17:12:32 +0100:
>> > On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
>> > > This still strips whitespace around ='s in the value:
>> > > SVNHooksEnv "name = x = y"
>> > > will result in
>> > > setenv("name", "x=y", 1)
>> > > whereas I believe it should result in
>> > > setenv("name", "x = y", 1)
>> > > (and, to be honest, I'd be happy with
>> > > setenv("name ", " x = y", 1)
>> > > as well).
>> > >
>> > > WDYT? How should it behave?
>> >
>> > I agree.
>> > would telling svn_cstring_split() to no strip whitespace suffice?
>>
>> I assume that should result in the third setenv() case above, so +1.
>
> Ping? trunk@HEAD still strips whitespace around equal signs in the value.
My tuppence-worth? I agree that the current behaviour as stated above is wrong. Unless there is precedent to the contrary, I think it should do no stripping at all. If you can find precedent for some stripping in such a setting, then follow the precedent. Note that it's not only possible to strip spaces before and/or after the first '=' character, but also before "name" and/or after "x = y".
- Julian
Re: svn commit: r1241050 - /subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Shahaf wrote:
> Daniel Shahaf wrote on Mon, Feb 06, 2012 at 18:20:28 +0200:
>> Stefan Sperling wrote on Mon, Feb 06, 2012 at 17:12:32 +0100:
>> > On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
>> > > This still strips whitespace around ='s in the value:
>> > > SVNHooksEnv "name = x = y"
>> > > will result in
>> > > setenv("name", "x=y", 1)
>> > > whereas I believe it should result in
>> > > setenv("name", "x = y", 1)
>> > > (and, to be honest, I'd be happy with
>> > > setenv("name ", " x = y", 1)
>> > > as well).
>> > >
>> > > WDYT? How should it behave?
>> >
>> > I agree.
>> > would telling svn_cstring_split() to no strip whitespace suffice?
>>
>> I assume that should result in the third setenv() case above, so +1.
>
> Ping? trunk@HEAD still strips whitespace around equal signs in the value.
My tuppence-worth? I agree that the current behaviour as stated above is wrong. Unless there is precedent to the contrary, I think it should do no stripping at all. If you can find precedent for some stripping in such a setting, then follow the precedent. Note that it's not only possible to strip spaces before and/or after the first '=' character, but also before "name" and/or after "x = y".
- Julian
Re: svn commit: r1241050 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Daniel Shahaf <da...@elego.de>.
Daniel Shahaf wrote on Mon, Feb 06, 2012 at 18:20:28 +0200:
> Stefan Sperling wrote on Mon, Feb 06, 2012 at 17:12:32 +0100:
> > On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
> > > This still strips whitespace around ='s in the value:
> > > SVNHooksEnv "name = x = y"
> > > will result in
> > > setenv("name", "x=y", 1)
> > > whereas I believe it should result in
> > > setenv("name", "x = y", 1)
> > > (and, to be honest, I'd be happy with
> > > setenv("name ", " x = y", 1)
> > > as well).
> > >
> > > WDYT? How should it behave?
> >
> > I agree.
> > would telling svn_cstring_split() to no strip whitespace suffice?
>
> I assume that should result in the third setenv() case above, so +1.
Ping? trunk@HEAD still strips whitespace around equal signs in the value.
Re: svn commit: r1241050 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Daniel Shahaf <da...@elego.de>.
Daniel Shahaf wrote on Mon, Feb 06, 2012 at 18:20:28 +0200:
> Stefan Sperling wrote on Mon, Feb 06, 2012 at 17:12:32 +0100:
> > On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
> > > This still strips whitespace around ='s in the value:
> > > SVNHooksEnv "name = x = y"
> > > will result in
> > > setenv("name", "x=y", 1)
> > > whereas I believe it should result in
> > > setenv("name", "x = y", 1)
> > > (and, to be honest, I'd be happy with
> > > setenv("name ", " x = y", 1)
> > > as well).
> > >
> > > WDYT? How should it behave?
> >
> > I agree.
> > would telling svn_cstring_split() to no strip whitespace suffice?
>
> I assume that should result in the third setenv() case above, so +1.
Ping? trunk@HEAD still strips whitespace around equal signs in the value.
Re: svn commit: r1241050 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Daniel Shahaf <da...@elego.de>.
Stefan Sperling wrote on Mon, Feb 06, 2012 at 17:12:32 +0100:
> On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
> > This still strips whitespace around ='s in the value:
> > SVNHooksEnv "name = x = y"
> > will result in
> > setenv("name", "x=y", 1)
> > whereas I believe it should result in
> > setenv("name", "x = y", 1)
> > (and, to be honest, I'd be happy with
> > setenv("name ", " x = y", 1)
> > as well).
> >
> > WDYT? How should it behave?
>
> I agree.
> would telling svn_cstring_split() to no strip whitespace suffice?
I assume that should result in the third setenv() case above, so +1.
Re: svn commit: r1241050 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Daniel Shahaf <da...@elego.de>.
Stefan Sperling wrote on Mon, Feb 06, 2012 at 17:12:32 +0100:
> On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
> > This still strips whitespace around ='s in the value:
> > SVNHooksEnv "name = x = y"
> > will result in
> > setenv("name", "x=y", 1)
> > whereas I believe it should result in
> > setenv("name", "x = y", 1)
> > (and, to be honest, I'd be happy with
> > setenv("name ", " x = y", 1)
> > as well).
> >
> > WDYT? How should it behave?
>
> I agree.
> would telling svn_cstring_split() to no strip whitespace suffice?
I assume that should result in the third setenv() case above, so +1.
Re: svn commit: r1241050 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
> This still strips whitespace around ='s in the value:
> SVNHooksEnv "name = x = y"
> will result in
> setenv("name", "x=y", 1)
> whereas I believe it should result in
> setenv("name", "x = y", 1)
> (and, to be honest, I'd be happy with
> setenv("name ", " x = y", 1)
> as well).
>
> WDYT? How should it behave?
I agree.
would telling svn_cstring_split() to no strip whitespace suffice?
Re: svn commit: r1241050 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote:
> This still strips whitespace around ='s in the value:
> SVNHooksEnv "name = x = y"
> will result in
> setenv("name", "x=y", 1)
> whereas I believe it should result in
> setenv("name", "x = y", 1)
> (and, to be honest, I'd be happy with
> setenv("name ", " x = y", 1)
> as well).
>
> WDYT? How should it behave?
I agree.
would telling svn_cstring_split() to no strip whitespace suffice?
Re: svn commit: r1241050 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Daniel Shahaf <da...@elego.de>.
This still strips whitespace around ='s in the value:
SVNHooksEnv "name = x = y"
will result in
setenv("name", "x=y", 1)
whereas I believe it should result in
setenv("name", "x = y", 1)
(and, to be honest, I'd be happy with
setenv("name ", " x = y", 1)
as well).
WDYT? How should it behave?
stsp@apache.org wrote on Mon, Feb 06, 2012 at 15:46:54 -0000:
> Author: stsp
> Date: Mon Feb 6 15:46:54 2012
> New Revision: 1241050
>
> URL: http://svn.apache.org/viewvc?rev=1241050&view=rev
> Log:
> * subversion/mod_dav_svn/mod_dav_svn.c
> (SVNHooksEnv_cmd): Handle environment variables with values containing '='.
> While here, dup strings referenced from the hash table into the hash
> table's pool for safety.
>
> Modified:
> subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
>
> Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=1241050&r1=1241049&r2=1241050&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
> +++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Mon Feb 6 15:46:54 2012
> @@ -538,14 +538,36 @@ SVNHooksEnv_cmd(cmd_parms *cmd, void *co
> apr_array_header_t *var;
>
> var = svn_cstring_split(arg1, "=", TRUE, cmd->pool);
> - if (var && var->nelts == 2)
> + if (var && var->nelts >= 2)
> {
> dir_conf_t *conf = config;
> + const char *name;
> + const char *val;
>
> - apr_hash_set(conf->hooks_env,
> - APR_ARRAY_IDX(var, 0, const char *),
> - APR_HASH_KEY_STRING,
> - APR_ARRAY_IDX(var, 1, const char *));
> + name = apr_pstrdup(apr_hash_pool_get(conf->hooks_env),
> + APR_ARRAY_IDX(var, 0, const char *));
> +
> + /* Special case for values which contain '='. */
> + if (var->nelts > 2)
> + {
> + svn_stringbuf_t *buf;
> + int i;
> +
> + buf = svn_stringbuf_create(APR_ARRAY_IDX(var, 1, const char *),
> + cmd->pool);
> + for (i = 2; i < var->nelts; i++)
> + {
> + svn_stringbuf_appendbyte(buf, '=');
> + svn_stringbuf_appendcstr(buf, APR_ARRAY_IDX(var, i, const char *));
> + }
> +
> + val = apr_pstrdup(apr_hash_pool_get(conf->hooks_env), buf->data);
> + }
> + else
> + val = apr_pstrdup(apr_hash_pool_get(conf->hooks_env),
> + APR_ARRAY_IDX(var, 1, const char *));
> +
> + apr_hash_set(conf->hooks_env, name, APR_HASH_KEY_STRING, val);
> }
>
> return NULL;
>
>
Re: svn commit: r1241050 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Daniel Shahaf <da...@elego.de>.
This still strips whitespace around ='s in the value:
SVNHooksEnv "name = x = y"
will result in
setenv("name", "x=y", 1)
whereas I believe it should result in
setenv("name", "x = y", 1)
(and, to be honest, I'd be happy with
setenv("name ", " x = y", 1)
as well).
WDYT? How should it behave?
stsp@apache.org wrote on Mon, Feb 06, 2012 at 15:46:54 -0000:
> Author: stsp
> Date: Mon Feb 6 15:46:54 2012
> New Revision: 1241050
>
> URL: http://svn.apache.org/viewvc?rev=1241050&view=rev
> Log:
> * subversion/mod_dav_svn/mod_dav_svn.c
> (SVNHooksEnv_cmd): Handle environment variables with values containing '='.
> While here, dup strings referenced from the hash table into the hash
> table's pool for safety.
>
> Modified:
> subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
>
> Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=1241050&r1=1241049&r2=1241050&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
> +++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Mon Feb 6 15:46:54 2012
> @@ -538,14 +538,36 @@ SVNHooksEnv_cmd(cmd_parms *cmd, void *co
> apr_array_header_t *var;
>
> var = svn_cstring_split(arg1, "=", TRUE, cmd->pool);
> - if (var && var->nelts == 2)
> + if (var && var->nelts >= 2)
> {
> dir_conf_t *conf = config;
> + const char *name;
> + const char *val;
>
> - apr_hash_set(conf->hooks_env,
> - APR_ARRAY_IDX(var, 0, const char *),
> - APR_HASH_KEY_STRING,
> - APR_ARRAY_IDX(var, 1, const char *));
> + name = apr_pstrdup(apr_hash_pool_get(conf->hooks_env),
> + APR_ARRAY_IDX(var, 0, const char *));
> +
> + /* Special case for values which contain '='. */
> + if (var->nelts > 2)
> + {
> + svn_stringbuf_t *buf;
> + int i;
> +
> + buf = svn_stringbuf_create(APR_ARRAY_IDX(var, 1, const char *),
> + cmd->pool);
> + for (i = 2; i < var->nelts; i++)
> + {
> + svn_stringbuf_appendbyte(buf, '=');
> + svn_stringbuf_appendcstr(buf, APR_ARRAY_IDX(var, i, const char *));
> + }
> +
> + val = apr_pstrdup(apr_hash_pool_get(conf->hooks_env), buf->data);
> + }
> + else
> + val = apr_pstrdup(apr_hash_pool_get(conf->hooks_env),
> + APR_ARRAY_IDX(var, 1, const char *));
> +
> + apr_hash_set(conf->hooks_env, name, APR_HASH_KEY_STRING, val);
> }
>
> return NULL;
>
>