You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2015/07/20 14:45:26 UTC
Re: [Issue 4584] New - Non-canonical $HOME crashes GPG-agent support code
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, <ju...@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.
>
Re: [Issue 4584] New - Non-canonical $HOME crashes GPG-agent support code
Posted by Julian Foad <ju...@btopenworld.com>.
On 20 July 2015, I (Julian Foad) wrote:
[...]
> The minimal fix is committed to trunk in r1691928. Note that this
> fixes the *only* caller that exists so far that didn't canonicalize
> the resulting path.
>
> A better fix, but one less suitable for back-porting, is committed in
> r1691952. That makes svn_user_get_homedir() always return a canonical
> path.
[...]
> I still intend to nominate the minimal fix r1691928 for backport to
> 1.8.x and 1.9.x.
The fix r1691928 was included in release 1.9.1 and is now approved for
inclusion in the next 1.8.x release.
- Julian
Re: [Issue 4584] New - Non-canonical $HOME crashes GPG-agent support code
Posted by Julian Foad <ju...@btopenworld.com>.
Bert said on IRC: "julianf: I would recommend fixing the api
implementation (to follow our common rule: all passed paths are
canonical, unless documented otherwise) and just backporting the fix."
The minimal fix is committed to trunk in r1691928. Note that this
fixes the *only* caller that exists so far that didn't canonicalize
the resulting path.
A better fix, but one less suitable for back-porting, is committed in
r1691952. That makes svn_user_get_homedir() always return a canonical
path.
I tried writing a unit test for it, and came up with the attached
'test_svn_user_get_homedir.patch'.
I don't like it much: it messes with the 'HOME' variable in the
process's environment, and although it makes some attempt to avoid
parallel execution and tries to reset the variable afterwards, it
doesn't do so in all cases. Notably it errors out if the 'HOME' env
var is unset to start with.
Do we want a unit test something like this, and if so how should I
make it more Good?
I still intend to nominate the minimal fix r1691928 for backport to
1.8.x and 1.9.x.
- Julian
On 20 July 2015 at 14:05, Julian Foad <ju...@btopenworld.com> wrote:
> I (Julian Foad) wrote:
>> I have a one-line fix for this issue, which I've tested by hand:
>> [...]
>> 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?
>
> Of course, we would like to have a regression test. Can anyone help
> me write one? I took a quick look in
> subversion/tests/libsvn_subr/auth-test.c for inspiration and am a bit
> lost where to start at that level.
>
> If we go for a 'proper' fix, we could write a unit test for
> svn_user_get_homedir(), which would be much easier than a regression
> test for the authentication scenario with GPG-agent.
>
> - Julian
Re: [Issue 4584] New - Non-canonical $HOME crashes GPG-agent support code
Posted by Julian Foad <ju...@btopenworld.com>.
I (Julian Foad) wrote:
> I have a one-line fix for this issue, which I've tested by hand:
> [...]
> 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?
Of course, we would like to have a regression test. Can anyone help
me write one? I took a quick look in
subversion/tests/libsvn_subr/auth-test.c for inspiration and am a bit
lost where to start at that level.
If we go for a 'proper' fix, we could write a unit test for
svn_user_get_homedir(), which would be much easier than a regression
test for the authentication scenario with GPG-agent.
- Julian