Re: svn commit: r1239926 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h

[Daniel Shahaf]

Stefan Sperling wrote on Fri, Feb 03, 2012 at 02:07:59 +0100:
> On Thu, Feb 02, 2012 at 03:41:48PM -0800, Blair Zajac wrote:
> > It doesn't look like it's multithreaded safe to set, but should it be?
> 
> I am not sure.
> 
> Note that this isn't changing the environment of the currently running
> process. It takes effect in the child process that runs a hook.
> 
> The intended use is to set the hook environment once, after opening
> the repository with svn_repos_open().
> 

In that case, you could have revved svn_repos_open() to add a parameter;
that would have claified the intended usage and avoided the thread-safety
issues altogether.

> I've just committed some code that uses this from mod_dav_svn
> in r1239966. Maybe this example will make the intented use clear, and
> we can document behaviour WRT thread-safety or make the code thread-safe?

Re: svn commit: r1239966 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c repos.c

[Daniel Shahaf]

s...@apache.org wrote on Fri, Feb 03, 2012 at 01:02:08 -:
> +static const char *
> +SVNHooksEnv_cmd(cmd_parms *cmd, void *config, const char *arg1)
> +{
> +  apr_array_header_t *var;
> +
> +  var = svn_cstring_split(arg1, "=", TRUE, cmd->pool);
> +  if (var && var->nelts == 2)
> +{

With this code, if an envvar's value legitimately contains a '=', it'll
be silently skipped.  (Example:
putenv("config-option=servers:global:http-library=serf"))

Suggest:

const char *var, *val;
const char *equals = strchr(arg1, '=');

if (equals)
  /* edge case: equals == var. What to do? */
  var = apr_pstrndup(cmd->pool, arg1, equals - arg1);
else
  var = arg1;

if (equals)
  val = equals+1;
else
  val = "1";

> +  dir_conf_t *conf = config;
> +
> +  apr_hash_set(conf->hooks_env,
> +   APR_ARRAY_IDX(var, 0, const char *),
> +   APR_HASH_KEY_STRING,
> +   APR_ARRAY_IDX(var, 1, const char *));
> +}
> +
> +  return NULL;
> +}
> +
>  
>  /** Accessor functions for the module's configuration state **/
>  
> @@ -805,6 +830,15 @@ dav_svn__get_compression_level(void)
>return svn__compression_level;
>  }
>  
> +/* Copy the environment given as key/value pairs of ENV_HASH into
> + * an array of C strings allocated in RESULT_POOL.
> + * Use SCRATCH_POOL for temporary allocations. */
> +static const char **
> +env_from_env_hash(apr_hash_t *env_hash,
> +  apr_pool_t *result_pool,
> +  apr_pool_t *scratch_pool)
> +{
> +  apr_hash_index_t *hi;
> +  const char **env;
> +  const char **envp;
> +  
> +  env = apr_palloc(result_pool,
> +   sizeof(const char *) * (apr_hash_count(env_hash) + 1));
> +  envp = env;
> +  for (hi = apr_hash_first(scratch_pool, env_hash); hi; hi = 
> apr_hash_next(hi))
> +{
> +  *envp = apr_psprintf(result_pool, "%s=%s",

Heh.  So you parse an array of VAR=VAL lines into a hash, and then
unparse it back?  Why not carry it as an array (perhaps an APR array) of
"VAR=VAL" values?

(And yes, if someone does SVNHooksEnv "X=1" "X=2" and then gets X=1 in
their hooks, they really have only themselves to blame; but I still
wonder why we need the double-translation.)

> +   (const char *)svn__apr_hash_index_key(hi),
> +   (const char *)svn__apr_hash_index_val(hi));
> +  envp++;
> +}
> +  *envp = NULL;
>  
> +  return env;
> +}
>  
>  static dav_error *
>  get_resource(request_rec *r,

Cheers,

Daniel

Re: --non-interactive and keyrings

[Philip Martin]

Julian Foad  writes:

>   * Brings kwallet to the same behaviour as Gnome keyring.

I've realised there is another difference in the current behaviour.  The
way auth works is that Subversion records whether a particular provider
was used to store a particular password.  The KDE provider will only
prompt to open the wallet when the auth data indicates that KDE was used
to store a particular password.  The GNOME provider prompts to unlock the
keyring whenever any password is requested, before checking the auth
data to see if this particular password was stored in the keyring.

I don't see any advantage to the GNOME behaviour, it looks more like a
bug than a feature.

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com

Re: --non-interactive and keyrings

[Daniel Shahaf]

Philip Martin wrote on Fri, Feb 03, 2012 at 10:02:06 +:
> Julian Foad  writes:
> 
> >   * Brings kwallet to the same behaviour as Gnome keyring.
> 
> I've realised there is another difference in the current behaviour.  The
> way auth works is that Subversion records whether a particular provider
> was used to store a particular password.  The KDE provider will only
> prompt to open the wallet when the auth data indicates that KDE was used
> to store a particular password.  The GNOME provider prompts to unlock the
> keyring whenever any password is requested, before checking the auth
> data to see if this particular password was stored in the keyring.
> 
> I don't see any advantage to the GNOME behaviour, it looks more like a
> bug than a feature.

That behaviour is defensible.  "Why should any random app I run know
what passwords my keyring stores?"

Compare how Subversion does not disclose the names of directories one
doesn't have read access to.

> 
> -- 
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com

Re: svn commit: r1239966 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c repos.c

[Stefan Sperling]

On Fri, Feb 03, 2012 at 11:07:39AM +0200, Daniel Shahaf wrote:
> s...@apache.org wrote on Fri, Feb 03, 2012 at 01:02:08 -:
> > +static const char *
> > +SVNHooksEnv_cmd(cmd_parms *cmd, void *config, const char *arg1)
> > +{
> > +  apr_array_header_t *var;
> > +
> > +  var = svn_cstring_split(arg1, "=", TRUE, cmd->pool);
> > +  if (var && var->nelts == 2)
> > +{
> 
> With this code, if an envvar's value legitimately contains a '=', it'll
> be silently skipped.  (Example:
> putenv("config-option=servers:global:http-library=serf"))

Ah, you're right. It should require at least two elements and
combine the second element with any that follow.

> > +static const char **
> > +env_from_env_hash(apr_hash_t *env_hash,
> > +  apr_pool_t *result_pool,
> > +  apr_pool_t *scratch_pool)
> > +{
> > +  apr_hash_index_t *hi;
> > +  const char **env;
> > +  const char **envp;
> > +  
> > +  env = apr_palloc(result_pool,
> > +   sizeof(const char *) * (apr_hash_count(env_hash) + 1));
> > +  envp = env;
> > +  for (hi = apr_hash_first(scratch_pool, env_hash); hi; hi = 
> > apr_hash_next(hi))
> > +{
> > +  *envp = apr_psprintf(result_pool, "%s=%s",
> 
> Heh.  So you parse an array of VAR=VAL lines into a hash, and then
> unparse it back?  Why not carry it as an array (perhaps an APR array) of
> "VAR=VAL" values?

We need to parse it once anyway to verify the VAR=VAL syntax.
So this was mainly done to make sure we're using the right syntax
for the evecve() env parameter (see e.g. the putenv(3) man page).

After making this change I realised it would be nicer just let the
svn_repos API accept an apr_hash_t and perform the translation to
const char** internally. This would also make it trivial to have
svn_repos API calls to remove or add variables, or merge environments.
Merging might in fact be necessary if we want to support custom hook
environments over all RA protocols. Think about where users could define
custom hook env for file:// access? If it ends up being a config file
in the repository it makes no sense for mod_dav_svn and svnserve to ignore
that file...

Re: --non-interactive and keyrings

[Philip Martin]

Daniel Shahaf  writes:

> Philip Martin wrote on Fri, Feb 03, 2012 at 10:02:06 +:
>> Julian Foad  writes:
>> 
>> >   * Brings kwallet to the same behaviour as Gnome keyring.
>> 
>> I've realised there is another difference in the current behaviour.  The
>> way auth works is that Subversion records whether a particular provider
>> was used to store a particular password.  The KDE provider will only
>> prompt to open the wallet when the auth data indicates that KDE was used
>> to store a particular password.  The GNOME provider prompts to unlock the
>> keyring whenever any password is requested, before checking the auth
>> data to see if this particular password was stored in the keyring.
>> 
>> I don't see any advantage to the GNOME behaviour, it looks more like a
>> bug than a feature.
>
> That behaviour is defensible.  "Why should any random app I run know
> what passwords my keyring stores?"
>
> Compare how Subversion does not disclose the names of directories one
> doesn't have read access to.

Subversion does send the top-level names of trees that are excluded.

Even without that I'm not sure I understand. The user can also try
arbitrary names and get "access denied"; that would be similar to the
user trying arbitrary URLs to see whether it caused the keyring to be
unlocked.

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com

Re: --non-interactive and keyrings

[Daniel Shahaf]

Philip Martin wrote on Fri, Feb 03, 2012 at 10:38:49 +:
> Daniel Shahaf  writes:
> 
> > Philip Martin wrote on Fri, Feb 03, 2012 at 10:02:06 +:
> >> Julian Foad  writes:
> >> 
> >> >   * Brings kwallet to the same behaviour as Gnome keyring.
> >> 
> >> I've realised there is another difference in the current behaviour.  The
> >> way auth works is that Subversion records whether a particular provider
> >> was used to store a particular password.  The KDE provider will only
> >> prompt to open the wallet when the auth data indicates that KDE was used
> >> to store a particular password.  The GNOME provider prompts to unlock the
> >> keyring whenever any password is requested, before checking the auth
> >> data to see if this particular password was stored in the keyring.
> >> 
> >> I don't see any advantage to the GNOME behaviour, it looks more like a
> >> bug than a feature.
> >
> > That behaviour is defensible.  "Why should any random app I run know
> > what passwords my keyring stores?"
> >
> > Compare how Subversion does not disclose the names of directories one
> > doesn't have read access to.
> 
> Subversion does send the top-level names of trees that are excluded.
> 

Mike patched mod_dav_svn recently not to do that.  See for example
https://svn.apache.org/repos/infra/
(which has an 'infrastructure' child that isn't disclosed)

> Even without that I'm not sure I understand. The user can also try
> arbitrary names and get "access denied"; that would be similar to the
> user trying arbitrary URLs to see whether it caused the keyring to be
> unlocked.

Or to someone trying many usernames and seeing which of them cause the
server to prompt for a password, instead of an immediate "No such
username" error.  (The equivalent here would be to issue a password
prompt even for nonexistent usernames tried.)

*shrug*.  All I'm saying is that it's done, and it's defensible.
I don't want to get into an argument about which behaviour is more
correct or more secure.

> 
> -- 
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com

Re: --non-interactive and keyrings

[Philip Martin]

Daniel Shahaf  writes:

>> Subversion does send the top-level names of trees that are excluded.
>
> Mike patched mod_dav_svn recently not to do that.  See for example
> https://svn.apache.org/repos/infra/
> (which has an 'infrastructure' child that isn't disclosed)

That's only for some requests. In general the names are visible:

$ svn co https://svn.apache.org/repos/infra/ --depth immediates wc
$ sqlite3 wc/.svn/wc.db "select local_relpath,presence from nodes"
infrastructure|absent
apachecon|normal
websites|normal
|normal


-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com

Re: [RFC] Performing backport merge commits from cron

[Daniel Shahaf]

Julian Foad wrote on Thu, Feb 02, 2012 at 12:53:38 +:
> Daniel Shahaf wrote:
> >     Create a role account on svn-qavm.apache.org and have it run
> >     backport.pl once a day, at 4am UTC, _and commit the resulting
> >     merges_.
> 
> Sounds good to me.  +1.

Done.  Crontab errors go to stsp and me; either of us can disable or
change the configuration.

Re: --non-interactive and keyrings

[Philip Martin]

Daniel Shahaf  writes:

> *shrug*.  All I'm saying is that it's done, and it's defensible.
> I don't want to get into an argument about which behaviour is more
> correct or more secure.

There are so many switches that interact it's hard to know what
constitutes correct behaviour.

Suppose I have a password stored on disk.  If kwallet is enabled
Subversion will use the disk password, I won't get prompted and the
password will remain stored on disk.  If gnome-keyring is enabled I get
the prompt to unlock the keyring but the password itself gets read from
the disk and doesn't get stored in the keyring.  So despite the user
interaction with the keyring the password remains stored on disk, that's
a bit odd.

Worse, if I add --non-iteractive to command then kwallet will still use
the password cached on disk without prompting.  However gnome-keyring
will error out because it is unable to prompt for the keyring.  The
error message is "Could not authenticate to server" with neon and "GNOME
Keyring is locked" with serf and svnserve.  That's despite the fact that
the passowrd is on disk.  I suppose we could argue that this is a
configuration error on the part of the user.

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com

Re: --non-interactive and keyrings

[Daniel Shahaf]

Philip Martin wrote on Fri, Feb 03, 2012 at 12:59:47 +:
> Daniel Shahaf  writes:
> 
> > *shrug*.  All I'm saying is that it's done, and it's defensible.
> > I don't want to get into an argument about which behaviour is more
> > correct or more secure.
> 
> There are so many switches that interact it's hard to know what
> constitutes correct behaviour.
> 
> Suppose I have a password stored on disk.  If kwallet is enabled
> Subversion will use the disk password, I won't get prompted and the
> password will remain stored on disk.  If gnome-keyring is enabled I get
> the prompt to unlock the keyring but the password itself gets read from
> the disk and doesn't get stored in the keyring.  So despite the user
> interaction with the keyring the password remains stored on disk, that's
> a bit odd.
> 
> Worse, if I add --non-iteractive to command then kwallet will still use
> the password cached on disk without prompting.  However gnome-keyring
> will error out because it is unable to prompt for the keyring.  The
> error message is "Could not authenticate to server" with neon and "GNOME
> Keyring is locked" with serf and svnserve.  That's despite the fact that
> the passowrd is on disk.  I suppose we could argue that this is a
> configuration error on the part of the user.

Well, we could, had gnome-keyring not been enabled by default.

(For example, on Debian I'd run into the 'Password for (null) keyring'
issue when using the system's svn, despite never having enabled the
keyring or otherwise interacted with it; so I resorted to using my
self-built svn.  I suppose that proactively disabling the keyring daemon
from starting would have worked too.)

> 
> -- 
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com

Re: --non-interactive and keyrings

[Julian Foad]

Daniel Shahaf wrote:

> Philip Martin wrote:
>> [...] Subversion records whether a particular provider
>>  was used to store a particular password.  The KDE provider will only
>>  prompt to open the wallet when the auth data indicates that KDE was used
>>  to store a particular password.  The GNOME provider prompts to unlock the
>>  keyring whenever any password is requested, before checking the auth
>>  data to see if this particular password was stored in the keyring.
>> 
>>  I don't see any advantage to the GNOME behaviour, it looks more like a
>>  bug than a feature.
> 
> That behaviour is defensible.  "Why should any random app I run know
> what passwords my keyring stores?"

Hi Daniel.  I don't follow what you mean.  The 'providers' that Philip refers 
to are bits of Subversion code, not the KDE/Gnome APIs themselves.

- Julian

Re: --non-interactive and keyrings

[Philip Martin]

Julian Foad  writes:

> Daniel Shahaf wrote:
>
>> Philip Martin wrote:
>>> [...] Subversion records whether a particular provider
>>>  was used to store a particular password.  The KDE provider will only
>>>  prompt to open the wallet when the auth data indicates that KDE was used
>>>  to store a particular password.  The GNOME provider prompts to unlock the
>>>  keyring whenever any password is requested, before checking the auth
>>>  data to see if this particular password was stored in the keyring.
>>> 
>>>  I don't see any advantage to the GNOME behaviour, it looks more like a
>>>  bug than a feature.
>> 
>> That behaviour is defensible.  "Why should any random app I run know
>> what passwords my keyring stores?"
>
> Hi Daniel.  I don't follow what you mean.  The 'providers' that Philip
> refers to are bits of Subversion code, not the KDE/Gnome APIs
> themselves.

The KDE behaviour is a potential information leak.  A random app can use
the Subversion libraries to query a repo, if it can monitor whether
such a query causes the KDE prompt to appear then it can determine
whether or not the password for the repo is in the wallet.  Since GNOME
always prompts no such leak is possible.

It's not much of a leak, I'm not sure what one would do with the
information.

-- 
Philip

Re: Let's discuss about unicode compositions for filenames!

[Julian Foad]

Hiroaki Nakamura wrote:

>>>  It would be nice if we could normalize paths in the repository without
>>>  having to perform a dump/reload cycle, but I don't know how that 
>>> would  work in FSFS.
>> 
>>  It won't.  Changing the encoding increase the length (in bytes) of the
>>  string (in the dirents hash, for example), and thus change the offsets
>>  of the node-revs that are later in the file --- to which subsequent
>>  revisions, and the id's of those node-revs, refer.
> 
> Changes from NFD to NFC does not increase the length.
> The length will be same or smaller, not larger.

You may well be correct that NFC is never longer than NFD, but that's not the 
question.  The question is whether NFC may be longer than the current paths 
(which are not normalized to normalization form C or to form D).  And the 
answer is yes it may be longer.  See 
.


> Here I quote from
> http://svn.apache.org/repos/asf/subversion/trunk/notes/unicode-composition-for-filenames
>   > The proposed internal 'normal form' should be NFC, if only if
>   > it were because it's the most compact form of the two:  when
>   > allocating memory to store a conversion result, it won't be
>   > necessary (ever) to allocate more than the size of the input buffer.

That statement seems to be talking about converting between NFC and NFD, not 
from un-normalized to normalized.

- Julian

Re: 1.7.3 next week-ish?

[Hyrum K Wright]

On Thu, Feb 2, 2012 at 7:26 AM, Hyrum K Wright
 wrote:
> On Wed, Jan 25, 2012 at 2:47 PM, Hyrum K Wright
>  wrote:
>> On Wed, Jan 25, 2012 at 5:27 PM, Hyrum K Wright
>>  wrote:
>>> This mail is very non-committal, but from the looks of CHANGES and
>>> STATUS there are number of items which could potentially make up a
>>> 1.7.3 release.  It's been a couple of months since 1.7.2, so it
>>> appears that a 1.7.3 should come sooner rather than later.  Perhaps
>>> sometime next week would be reasonable?
>>>
>>> Although Stefan offered to help RM this release, it sounds like he's
>>> swamped with stuff, so I'll go ahead and do it, if nobody has any
>>> objections.
>>
>> The quick observation is that a number of the Windows folks will be
>> traveling next week, so I'll wait to roll the tarballs toward the end
>> of the week (Feb. 2-3) which means folks can test and sign with they
>> get back and we can release the following week sometime.
>>
>> In the meantime, keep reviewing stuff in STATUS!
>
> Reminder: I'll be rolling 1.7.3 tomorrow, so get please take a stroll
> through STATUS (if you haven't already) and help get things ready for
> the release.

Daniel pointed out on IRC that there is currently breakage on the
1.7.x over ra_serf:
http://ci.apache.org/builders/svn-slik-w2k3-x64-ra/builds/3698

I'm hesitant to roll a release until the cause of this problem has
been located, and a potential course of action identified.

-Hyrum


-- 

uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/

Re: 1.7.3 next week-ish?

[Johan Corveleyn]

On Fri, Feb 3, 2012 at 4:20 PM, Hyrum K Wright
 wrote:
> On Thu, Feb 2, 2012 at 7:26 AM, Hyrum K Wright
>  wrote:
>> On Wed, Jan 25, 2012 at 2:47 PM, Hyrum K Wright
>>  wrote:
>>> On Wed, Jan 25, 2012 at 5:27 PM, Hyrum K Wright
>>>  wrote:
 This mail is very non-committal, but from the looks of CHANGES and
 STATUS there are number of items which could potentially make up a
 1.7.3 release.  It's been a couple of months since 1.7.2, so it
 appears that a 1.7.3 should come sooner rather than later.  Perhaps
 sometime next week would be reasonable?

 Although Stefan offered to help RM this release, it sounds like he's
 swamped with stuff, so I'll go ahead and do it, if nobody has any
 objections.
>>>
>>> The quick observation is that a number of the Windows folks will be
>>> traveling next week, so I'll wait to roll the tarballs toward the end
>>> of the week (Feb. 2-3) which means folks can test and sign with they
>>> get back and we can release the following week sometime.
>>>
>>> In the meantime, keep reviewing stuff in STATUS!
>>
>> Reminder: I'll be rolling 1.7.3 tomorrow, so get please take a stroll
>> through STATUS (if you haven't already) and help get things ready for
>> the release.
>
> Daniel pointed out on IRC that there is currently breakage on the
> 1.7.x over ra_serf:
> http://ci.apache.org/builders/svn-slik-w2k3-x64-ra/builds/3698
>
> I'm hesitant to roll a release until the cause of this problem has
> been located, and a potential course of action identified.

There is also the recent backport (just yesterday) of r1207555 and
r1207808 (adding mod_dontdothat to build.conf), which makes my Windows
build fail [1]. To fix that, r1227900 should be backported as well. It
needs just one more vote.

[1] http://article.gmane.org/gmane.comp.version-control.subversion.devel/133597

-- 
Johan

Re: 1.7.3 next week-ish?

[Philip Martin]

Johan Corveleyn  writes:

>> I'm hesitant to roll a release until the cause of this problem has
>> been located, and a potential course of action identified.
>
> There is also the recent backport (just yesterday) of r1207555 and
> r1207808 (adding mod_dontdothat to build.conf), which makes my Windows
> build fail [1]. To fix that, r1227900 should be backported as well. It
> needs just one more vote.

Can you reproduce the regression test error?  svnrdump_tests.py 43 fails
with: svnrdump: E140001: Unrecognized record type in stream

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com

Re: svn commit: r1239695 - in /subversion/branches/1.7.x: ./ STATUS build.conf contrib/server-side/mod_dontdothat/ tools/server-side/mod_dontdothat/

[Paul Burba]

On Thu, Feb 2, 2012 at 6:10 PM, Bert Huijben  wrote:
>
>
>> -Original Message-
>> From: Johan Corveleyn [mailto:jcor...@gmail.com]
>> Sent: donderdag 2 februari 2012 14:36
>> To: Stephen Butler
>> Cc: dev@subversion.apache.org; rhuij...@apache.org; Stefan Sperling; C.
>> Michael Pilato
>> Subject: Re: svn commit: r1239695 - in /subversion/branches/1.7.x: ./
> STATUS
>> build.conf contrib/server-side/mod_dontdothat/ tools/server-
>> side/mod_dontdothat/
>
>> Thanks for confirming. I just proposed r1227900 for backport.
>
> I don't have this problem in my build, but you can add my +1 for
> backporting.

I also could build mod_dontdothat on 1.7 without r1227900.  Bert,
IIRC, like me, you build with static libraries (--disable-shared).  I
tried to build with shared libs and the build failed.  Johan, I assume
you are building with shared libs?

P.S. I gave r1227900 the third vote in STATUS.

Paul

>        Bert
>

RE: --non-interactive and keyrings

[Bert Huijben]

Can't they also use the kde library directly?

Is it our problem?

Bert Huijben (Cell phone)
From: Philip Martin
Sent: 3-2-2012 5:30
To: Julian Foad
Cc: Daniel Shahaf; dev@subversion.apache.org
Subject: Re: --non-interactive and keyrings
Julian Foad  writes:

> Daniel Shahaf wrote:
>
>> Philip Martin wrote:
>>> [...] Subversion records whether a particular provider
>>>  was used to store a particular password.  The KDE provider will only
>>>  prompt to open the wallet when the auth data indicates that KDE was used
>>>  to store a particular password.  The GNOME provider prompts to unlock the
>>>  keyring whenever any password is requested, before checking the auth
>>>  data to see if this particular password was stored in the keyring.
>>>
>>>  I don't see any advantage to the GNOME behaviour, it looks more like a
>>>  bug than a feature.
>>
>> That behaviour is defensible.  "Why should any random app I run know
>> what passwords my keyring stores?"
>
> Hi Daniel.  I don't follow what you mean.  The 'providers' that Philip
> refers to are bits of Subversion code, not the KDE/Gnome APIs
> themselves.

The KDE behaviour is a potential information leak.  A random app can use
the Subversion libraries to query a repo, if it can monitor whether
such a query causes the KDE prompt to appear then it can determine
whether or not the password for the repo is in the wallet.  Since GNOME
always prompts no such leak is possible.

It's not much of a leak, I'm not sure what one would do with the
information.

-- 
Philip

RE: svn commit: r1239695 - in /subversion/branches/1.7.x: ./ STATUS build.conf contrib/server-side/mod_dontdothat/ tools/server-side/mod_dontdothat/

[Bert Huijben]

My scripts build both ways on the buildbots and don't fail, but I don't
see a problem with the fix.

Bert Huijben (Cell phone)
From: Paul Burba
Sent: 3-2-2012 9:01
To: Bert Huijben
Cc: Johan Corveleyn; dev@subversion.apache.org
Subject: Re: svn commit: r1239695 - in /subversion/branches/1.7.x: ./
STATUS build.conf contrib/server-side/mod_dontdothat/
tools/server-side/mod_dontdothat/
On Thu, Feb 2, 2012 at 6:10 PM, Bert Huijben  wrote:
>
>
>> -Original Message-
>> From: Johan Corveleyn [mailto:jcor...@gmail.com]
>> Sent: donderdag 2 februari 2012 14:36
>> To: Stephen Butler
>> Cc: dev@subversion.apache.org; rhuij...@apache.org; Stefan Sperling; C.
>> Michael Pilato
>> Subject: Re: svn commit: r1239695 - in /subversion/branches/1.7.x: ./
> STATUS
>> build.conf contrib/server-side/mod_dontdothat/ tools/server-
>> side/mod_dontdothat/
>
>> Thanks for confirming. I just proposed r1227900 for backport.
>
> I don't have this problem in my build, but you can add my +1 for
> backporting.

I also could build mod_dontdothat on 1.7 without r1227900.  Bert,
IIRC, like me, you build with static libraries (--disable-shared).  I
tried to build with shared libs and the build failed.  Johan, I assume
you are building with shared libs?

P.S. I gave r1227900 the third vote in STATUS.

Paul

>        Bert
>

Re: --non-interactive and keyrings

[Philip Martin]

Philip Martin  writes:

> The KDE behaviour is a potential information leak.  A random app can use
> the Subversion libraries to query a repo, if it can monitor whether
> such a query causes the KDE prompt to appear then it can determine
> whether or not the password for the repo is in the wallet.  Since GNOME
> always prompts no such leak is possible.

Thinking about this a bit further, it's not really a leak at all.  The
information that is leaking is whether or not 'kwallet' is stored in the
.subversion/auth directory for a given repository.  But any application
that is capable of triggering the leak would also be capable of simply
reading the .subversion/auth files.

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com

Re: 1.7.3 next week-ish?

[Johan Corveleyn]

On Fri, Feb 3, 2012 at 5:08 PM, Philip Martin
 wrote:
> Johan Corveleyn  writes:
>
>>> I'm hesitant to roll a release until the cause of this problem has
>>> been located, and a potential course of action identified.
>>
>> There is also the recent backport (just yesterday) of r1207555 and
>> r1207808 (adding mod_dontdothat to build.conf), which makes my Windows
>> build fail [1]. To fix that, r1227900 should be backported as well. It
>> needs just one more vote.
>
> Can you reproduce the regression test error?  svnrdump_tests.py 43 fails
> with: svnrdump: E140001: Unrecognized record type in stream
>
> --
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com

No, svnrdump_tests.py all passes for me:

C:\research\svn\client_build\1.7.x>runtest -t svnrdump_tests.py
Testing Release configuration on local repository.
Running tests in svnrdump_tests.py [1/1]..success
Summary of test results:
  44 tests PASSED

-- 
Johan

Re: --non-interactive and keyrings

[Daniel Shahaf]

Philip Martin wrote on Fri, Feb 03, 2012 at 17:43:53 +:
> Philip Martin  writes:
> 
> > The KDE behaviour is a potential information leak.  A random app can use
> > the Subversion libraries to query a repo, if it can monitor whether
> > such a query causes the KDE prompt to appear then it can determine
> > whether or not the password for the repo is in the wallet.  Since GNOME
> > always prompts no such leak is possible.
> 
> Thinking about this a bit further, it's not really a leak at all.

Agreed.  We're left still with the original problem --- that kwallet
prompts for unlock even if it doesn't contain the password, but gkeyring
prompts for unlock regardless of whether it contains the password?

> The information that is leaking is whether or not 'kwallet' is stored
> in the .subversion/auth directory for a given repository.  But any
> application that is capable of triggering the leak would also be
> capable of simply reading the .subversion/auth files.
> 
> -- 
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com

Re: 1.7.3 next week-ish?

[Johan Corveleyn]

On Fri, Feb 3, 2012 at 7:16 PM, Johan Corveleyn  wrote:
> On Fri, Feb 3, 2012 at 5:08 PM, Philip Martin
>  wrote:
>> Johan Corveleyn  writes:
>>
 I'm hesitant to roll a release until the cause of this problem has
 been located, and a potential course of action identified.
>>>
>>> There is also the recent backport (just yesterday) of r1207555 and
>>> r1207808 (adding mod_dontdothat to build.conf), which makes my Windows
>>> build fail [1]. To fix that, r1227900 should be backported as well. It
>>> needs just one more vote.
>>
>> Can you reproduce the regression test error?  svnrdump_tests.py 43 fails
>> with: svnrdump: E140001: Unrecognized record type in stream
>>
>> --
>> uberSVN: Apache Subversion Made Easy
>> http://www.uberSVN.com
>
> No, svnrdump_tests.py all passes for me:
>
> C:\research\svn\client_build\1.7.x>runtest -t svnrdump_tests.py
> Testing Release configuration on local repository.
> Running tests in svnrdump_tests.py [1/1]..success
> Summary of test results:
>  44 tests PASSED

Doh, you obviously meant that I try it with serf, not with ra_local.
Yes, I can reproduce that: it fails in exactly the same way as the
svn-slik-w2k3-x64-ra buildbot. Sorry for any confusion.

See dav-fails.log in attachment.

This is with a Release build on Win XP (x86).

-- 
Johan


dav-fails.log
Description: Binary data

Re: svn commit: r1239695 - in /subversion/branches/1.7.x: ./ STATUS build.conf contrib/server-side/mod_dontdothat/ tools/server-side/mod_dontdothat/

[Johan Corveleyn]

On Fri, Feb 3, 2012 at 6:37 PM, Bert Huijben  wrote:
> My scripts build both ways on the buildbots and don't fail, but I don't
> see a problem with the fix.
>
> Bert Huijben (Cell phone)
> From: Paul Burba
> Sent: 3-2-2012 9:01
> To: Bert Huijben
> Cc: Johan Corveleyn; dev@subversion.apache.org
> Subject: Re: svn commit: r1239695 - in /subversion/branches/1.7.x: ./
> STATUS build.conf contrib/server-side/mod_dontdothat/
> tools/server-side/mod_dontdothat/
> On Thu, Feb 2, 2012 at 6:10 PM, Bert Huijben  wrote:
>>
>>
>>> -Original Message-
>>> From: Johan Corveleyn [mailto:jcor...@gmail.com]
>>> Sent: donderdag 2 februari 2012 14:36
>>> To: Stephen Butler
>>> Cc: dev@subversion.apache.org; rhuij...@apache.org; Stefan Sperling; C.
>>> Michael Pilato
>>> Subject: Re: svn commit: r1239695 - in /subversion/branches/1.7.x: ./
>> STATUS
>>> build.conf contrib/server-side/mod_dontdothat/ tools/server-
>>> side/mod_dontdothat/
>>
>>> Thanks for confirming. I just proposed r1227900 for backport.
>>
>> I don't have this problem in my build, but you can add my +1 for
>> backporting.
>
> I also could build mod_dontdothat on 1.7 without r1227900.  Bert,
> IIRC, like me, you build with static libraries (--disable-shared).  I
> tried to build with shared libs and the build failed.  Johan, I assume
> you are building with shared libs?
>
> P.S. I gave r1227900 the third vote in STATUS.
>

Thanks for the votes.

Yes, I'm building with shared libs (no --disable-shared). In fact,
here is my Makefile in attachment. I've been using this one since the
first day I got involved in svn, and it has served me well ...
(credits go to danielsh by the way :-), his posting of this Makefile
to the users list got the ball rolling for me).

Maybe you guys can spot something in there that's different, or somehow related?

-- 
Johan


Makefile
Description: Binary data

broke the shims (was: svn commit: r1240370 ...)

[Greg Stein]

Hyrum,

Per our discussion, I updated what I could, but not everything (and I
ran no shims tests). Since I completely disabled the calls to
set_props() and set_text(), I know the shims are broken. I don't think
the changes to fix that part will be too hard.

It also looks like I broke some calls to build() by passing PROPS with
an action other than ACTION_PROPSET.

In any case... your turn now :-)

Cheers,
-g

On Fri, Feb 3, 2012 at 17:07,   wrote:
> Author: gstein
> Date: Fri Feb  3 22:07:31 2012
> New Revision: 1240370
>
> URL: http://svn.apache.org/viewvc?rev=1240370&view=rev
> Log:
> Update the Ev2 API, per recent discussions. This introduces the
> alter_* callbacks to atomically perform all changes to a node (rather
> than the old paired-call approach).
>
> Documentation for the changes is incomplete, for a future revision.
>
> The compatibility shims have not been fully-updated.
>...

Re: broke the shims (was: svn commit: r1240370 ...)

[Hyrum K Wright]

No problem.  I'll take a look at it over the weekend.

-Hyrum

On Fri, Feb 3, 2012 at 4:15 PM, Greg Stein  wrote:
> Hyrum,
>
> Per our discussion, I updated what I could, but not everything (and I
> ran no shims tests). Since I completely disabled the calls to
> set_props() and set_text(), I know the shims are broken. I don't think
> the changes to fix that part will be too hard.
>
> It also looks like I broke some calls to build() by passing PROPS with
> an action other than ACTION_PROPSET.
>
> In any case... your turn now :-)
>
> Cheers,
> -g
>
> On Fri, Feb 3, 2012 at 17:07,   wrote:
>> Author: gstein
>> Date: Fri Feb  3 22:07:31 2012
>> New Revision: 1240370
>>
>> URL: http://svn.apache.org/viewvc?rev=1240370&view=rev
>> Log:
>> Update the Ev2 API, per recent discussions. This introduces the
>> alter_* callbacks to atomically perform all changes to a node (rather
>> than the old paired-call approach).
>>
>> Documentation for the changes is incomplete, for a future revision.
>>
>> The compatibility shims have not been fully-updated.
>>...



-- 

uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/

Re: svn commit: r1239695 - in /subversion/branches/1.7.x: ./ STATUS build.conf contrib/server-side/mod_dontdothat/ tools/server-side/mod_dontdothat/

[Paul Burba]

On Fri, Feb 3, 2012 at 4:02 PM, Johan Corveleyn  wrote:
> On Fri, Feb 3, 2012 at 6:37 PM, Bert Huijben  wrote:
>> My scripts build both ways on the buildbots and don't fail, but I don't
>> see a problem with the fix.
>>
>> Bert Huijben (Cell phone)
>> From: Paul Burba
>> Sent: 3-2-2012 9:01
>> To: Bert Huijben
>> Cc: Johan Corveleyn; dev@subversion.apache.org
>> Subject: Re: svn commit: r1239695 - in /subversion/branches/1.7.x: ./
>> STATUS build.conf contrib/server-side/mod_dontdothat/
>> tools/server-side/mod_dontdothat/
>> On Thu, Feb 2, 2012 at 6:10 PM, Bert Huijben  wrote:
>>>
>>>
 -Original Message-
 From: Johan Corveleyn [mailto:jcor...@gmail.com]
 Sent: donderdag 2 februari 2012 14:36
 To: Stephen Butler
 Cc: dev@subversion.apache.org; rhuij...@apache.org; Stefan Sperling; C.
 Michael Pilato
 Subject: Re: svn commit: r1239695 - in /subversion/branches/1.7.x: ./
>>> STATUS
 build.conf contrib/server-side/mod_dontdothat/ tools/server-
 side/mod_dontdothat/
>>>
 Thanks for confirming. I just proposed r1227900 for backport.
>>>
>>> I don't have this problem in my build, but you can add my +1 for
>>> backporting.
>>
>> I also could build mod_dontdothat on 1.7 without r1227900.  Bert,
>> IIRC, like me, you build with static libraries (--disable-shared).  I
>> tried to build with shared libs and the build failed.  Johan, I assume
>> you are building with shared libs?
>>
>> P.S. I gave r1227900 the third vote in STATUS.
>>
>
> Thanks for the votes.
>
> Yes, I'm building with shared libs (no --disable-shared). In fact,
> here is my Makefile in attachment. I've been using this one since the
> first day I got involved in svn, and it has served me well ...
> (credits go to danielsh by the way :-), his posting of this Makefile
> to the users list got the ball rolling for me).
>
> Maybe you guys can spot something in there that's different, or somehow 
> related?


Hi Johan,

To clarify what I meant: I *was* able to replicate your 1.7.x build
problem when building shared libs.  Then with r1227900 merged I was
able to build shared libs successfully.

Paul

Commit Hooks Asynchronous?

[Cory Finger]

Hello, I was just curious. I am developing a script that involves both a
pre-commit hook and a post commit hook.

The post-commit hook opens a file, reads a file, and saves a file. I was
wondering, do I need to set up a lock-file system to ensure that 2 people
committing at the same time will not cause a problem in the file writing
process? Or do the commit hooks run asynchronously?

Re: Commit Hooks Asynchronous?

[Trent Nelson]

On Fri, Feb 03, 2012 at 02:42:49PM -0800, Cory Finger wrote:
> Hello, I was just curious. I am developing a script that involves both
> a pre-commit hook and a post commit hook.
>
> The post-commit hook opens a file, reads a file, and saves a file. I
> was wondering, do I need to set up a lock-file system to ensure that 2
> people committing at the same time will not cause a problem in the
> file writing process? Or do the commit hooks run asynchronously?

Assuming you're writing to the same file name during each hook
invocation, yes, you'll definitely need to introduce locking.

Alternatively, you could write to a revision-specific file during
post-commit, then have a little daemon that writes to the intended
file sequentially, as files become available.

If the time it takes you to process a commit is proportional to the
size of a commit, your post-commit hooks will finish out of order.

For example:
Post commit for rev 6 (30MB delta) is running.
Post commit for rev 7 (1 prop change) starts, finishes.
Post commit for rev 6 finishes a minute later.

Unless post-commit r7 logic depends on r6, I'd highly recommend
the daemon approach over locking.

Trent.

Re: Commit Hooks Asynchronous?

[Cory Finger]

Is there a way in subversion to disable the ability of the post-commit hook
to run concurrently?

Say Jim and Bob both commit a file at nearly the same time, I would like to
disable the asynchronous capabilities of commit hooks so that

Jim's commit hooks process, then Bob's commit hooks process.

Re: Commit Hooks Asynchronous?

[Trent Nelson]

On Fri, Feb 03, 2012 at 04:11:56PM -0800, Cory Finger wrote:
> Is there a way in subversion to disable the ability of the post-commit
> hook to run concurrently?
>
> Say Jim and Bob both commit a file at nearly the same time, I would
> like to disable the asynchronous capabilities of commit hooks so that
>
> Jim's commit hooks process, then Bob's commit hooks process.

There's no way of doing that out-of-the-box, as far as I know.

You *can* manually enforce sequential access by blocking from within
the context of a start-commit or pre-commit hook, but... well... it
turns into the mother of all race conditions pretty quickly.

I've got a hook framework* that analyzes commits; pre-commits are
analyzed to see if they should be blocked based on a set of rules,
and post-commits are used to update metadata about the commit which
is stored in the said post-commit's revision property.

The revprop metadata is inherently sequential; r6 depends on r5,
etc.  This design decision bit me in the ass a few months later
because of the asynchronous nature of hooks; I was getting into
situations where r6's post-commit would run before I'd finish
processing r5.

The end result was masses of ugly knee-jerk locking code, which I've
included below as a testament to how fiddly it can be trying to
enforce order with commits.

[*]: framework is called 'Enversion' (Enterprise Subversion).  I'm in
 the process of getting it ready to open source under an Apache 2.0
 license.  I mention it in this thread if you're interested in what
 it attempts to do:


http://mail-archives.apache.org/mod_mbox/subversion-dev/201110.mbox/%3c4e9315db.20...@snakebite.org%3E

Regards,

Trent.


Hideous locking code below.  Note that this isn't indicative of the
general code quality ;-)
--
def _init_evn(self):
self.is_rev_for_empty_repo = self.is_rev and self.rev == 0
self.is_rev_for_first_commit = self.is_rev and self.rev == 1
self.is_txn_for_first_commit = self.is_txn and self.base_rev == 0
self.is_normal_rev_or_txn = (
(self.is_txn and self.base_rev >= 1) or
(self.is_rev and self.rev > 1)
)

# Test mutually-exclusive invariants.
assert (
(self.is_rev_for_empty_repo and (
not self.is_txn_for_first_commit and
not self.is_rev_for_first_commit and
not self.is_normal_rev_or_txn
)) or (self.is_rev_for_first_commit and (
not self.is_rev_for_empty_repo and
not self.is_txn_for_first_commit and
not self.is_normal_rev_or_txn
)) or (self.is_txn_for_first_commit and (
not self.is_rev_for_empty_repo and
not self.is_rev_for_first_commit and
not self.is_normal_rev_or_txn
)) or (self.is_normal_rev_or_txn and (
not self.is_rev_for_empty_repo and
not self.is_rev_for_first_commit and
not self.is_txn_for_first_commit
))
)

rc0 = self.r0_revprop_conf

if self.is_rev_for_empty_repo and 'version' not in rc0:
rc0.last_rev = 0
rc0.version = ESVN_VERSION

version = self._load_evn_revprop_int('version', 1)
if version != ESVN_VERSION:
self.die(e.VersionMismatch % (ESVN_VERSION, version))

if version == 1:
self._init_evn_v1()
else:
raise UnexpectedCodePath

def _init_evn_v1(self):

if self.is_rev_for_empty_repo:
return

if self.is_txn_for_first_commit or self.is_rev_for_first_commit:
assert self.base_rev == 0
k = Dict()
if self.is_txn:
k.base_rev = 0
else:
k.rev = 1
c = self.rconf(**k)
self.__roots = (c, {})
return

assert self.base_rev > 0
if self.is_rev:
assert self.rev >= 2
else:
assert self.base_rev >= 1

max_revlock_waits = self.conf.get('general', 'max-revlock-waits')

# Quick sanity check of last_rev to make sure it's not higher than the
# highest rev of the repository.
self._reload_last_rev()
highest_rev = svn.fs.youngest_rev(self.fs, self.pool)
if self.last_rev > highest_rev:
self.die(e.LastRevTooHigh % (self.last_rev, highest_rev))

if self.is_rev and isinstance(self, RepositoryHook):
with open(self.rev_lockfile, 'w') as f:
f.write(str(os.getpid()))
f.flush()
f.close()

if self.good_last_rev and self.good_base_rev_roots:
self._last_rev_and_base_rev_roots_are_good()
return

dbg = self.log.debug
a = (str(self.rev_or_txn), self.base_rev, self.last_rev)
dbg('rev_or

Re: Commit Hooks Asynchronous?

[Branko Čibej]

On 04.02.2012 01:11, Cory Finger wrote:
> Is there a way in subversion to disable the ability of the post-commit hook
> to run concurrently?
>
> Say Jim and Bob both commit a file at nearly the same time, I would like to
> disable the asynchronous capabilities of commit hooks so that
>
> Jim's commit hooks process, then Bob's commit hooks process.
>

Please move this conversation to the users@ list.

And there's nothing stopping you from using an external locking
mechanism to serialize your commit hooks.

-- Brane