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

Re: [Issue 4584] New - Non-canonical $HOME crashes GPG-agent support code

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Mon, 20 Jul 2015 13:45:26 +0100

I have a one-line fix for this issue, which I've tested by hand:

[[[
Index: subversion/libsvn_subr/gpg_agent.c
===================================================================
--- subversion/libsvn_subr/gpg_agent.c (revision 1688253)
+++ subversion/libsvn_subr/gpg_agent.c (working copy)
@@ -232,6 +232,7 @@ find_running_gpg_agent(int *new_sd, apr_
       if (!homedir)
         return SVN_NO_ERROR;

+ homedir = svn_dirent_canonicalize(homedir, pool);
       socket_name = svn_dirent_join_many(pool, homedir, ".gnupg",
                                          "S.gpg-agent", SVN_VA_NULL);
     }
]]]

I don't have an automated regression test. Should I go ahead and commit the
fix anyway, and propose for backport to 1.8 and 1.9?

(The bug was introduced in 1.8.11.)

This bug arose because svn_user_get_homedir() returns a non-canonical path
if the environment variable 'HOME' has a non-canonical value. Our APIs
should return canonical paths. A better fix would be to do so, perhaps like
this:

[[[
Index: subversion/include/svn_user.h
===================================================================
--- subversion/include/svn_user.h (revision 1688253)
+++ subversion/include/svn_user.h (working copy)
@@ -45,6 +45,8 @@
  * any necessary allocation, returning NULL on error.
  *
  * @since New in 1.4.
+ * @since 1.10 canonicalizes the result; before that it could return a
+ * non-canonical path.
  */
 const char *
 svn_user_get_homedir(apr_pool_t *pool);
Index: subversion/libsvn_subr/user.c
===================================================================
--- subversion/libsvn_subr/user.c (revision 1688253)
+++ subversion/libsvn_subr/user.c (working copy)
@@ -30,2 +30,3 @@
 #include "svn_utf.h"
+#include "svn_dirent_uri.h"

@@ -71,4 +71,4 @@ svn_user_get_name(apr_pool_t *pool)

-const char *
-svn_user_get_homedir(apr_pool_t *pool)
+static const char *
+user_get_homedir(apr_pool_t *pool)
 {
@@ -86,1 +87,12 @@ svn_user_get_homedir(apr_pool_t *pool)
 }
+
+const char *
+svn_user_get_homedir(apr_pool_t *pool)
+{
+ const char *homedir = user_get_homedir(pool);
+
+ if (homedir)
+ return svn_dirent_canonicalize(homedir, pool);
+
+ return NULL;
+}
Index: subversion/libsvn_subr/config_file.c
===================================================================
--- subversion/libsvn_subr/config_file.c (revision 1688253)
+++ subversion/libsvn_subr/config_file.c (working copy)
@@ -1407,7 +1407,7 @@ svn_config_get_user_config_path(const ch
     if (! homedir)
       return SVN_NO_ERROR;
     *path = svn_dirent_join_many(pool,
- svn_dirent_canonicalize(homedir, pool),
+ homedir,
                                SVN_CONFIG__USR_DIRECTORY, fname,
SVN_VA_NULL);
   }
 #endif /* WIN32 */
]]]

Alternatively, perhaps, bump the API to svn_user_get_homedir2() and leave
the non-canonical version as it is; but I think that's a worse idea.

What does anybody say?

- Julian

On 17 July 2015 at 18:27, <julianfoad_at_tigris.org> wrote:

> http://subversion.tigris.org/issues/show_bug.cgi?id=4584
> Issue #|4584
> Summary|Non-canonical $HOME crashes GPG-agent support code
>

> The 'svn' client can suffer an assertion failure during authentication to
> a server:
>
> svn: subversion/libsvn_subr/dirent_uri.c:1050: svn_dirent_join_many:
> Assertion
> `svn_dirent_is_canonical(base, pool)' failed.
>
> if (at least) the following conditions are true:
>
> * The value of $HOME [1] is non-canonical according to
> svn_dirent_is_canonical()
> but resolves to a valid home directory according to typical
> interpretations.
> For example, '/home/myname/' or '//home/myname/'.
>
> * GPG-agent support is enabled in the 'password-stores' config option.
>
> * The environment variable $GPG_AGENT_INFO is not set.
>
> * Some conditions related to the sequence in which the Subversion client
> gets
> authentication credentials. For example, it can be triggered after a
> correct
> password is entered at a prompt, or if the correct username and
> password are
> given on the command line and the authentication username is not equal
> to
> the OS username.
>
> The problem was seen and reported by a WANdisco customer running Subversion
> 1.8.11 and observing that this was a regression from 1.8.8. In this
> instance,
> the value of $HOME had a trailing slash.
>
> I found that the problem was introduced in Subversion 1.8.11, in the
> function
> find_running_gpg_agent() which calls:
>
> homedir = svn_user_get_homedir(pool);
> ... socket_name = svn_dirent_join_many(pool, homedir, ...);
>
> The only previous caller of svn_user_get_homedir() canonicalizes the result
> before passing it to svn_dirent_join_many(); this new code does not.
>
> [1] If $HOME is not set, then Subversion would instead use the home
> directory
> path reported by the operating system via APR. I have not confirmed
> whether that
> could be non-canonical.
>
Received on 2015-07-20 14:46:06 CEST

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