You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jan Palus <jp...@fastmail.com> on 2018/07/11 11:46:35 UTC
[PATCH] Fix segfault when gnome keyring lookup fails
[[[
Fix segfault when subversion is built with gnome keyring support and keyring
password lookup fails.
* subversion/libsvn_subr/simple_providers.c
(svn_auth__simple_creds_cache_get): Initialize 'done' with FALSE since ie
password_get_gnome_keyring sets 'done' only to TRUE. In case of error
it leaves unintialized 'done' (usually non 0) and default_password with NULL
causing segfault at subsequent strcmp.
]]]
Index: subversion/libsvn_subr/simple_providers.c
===================================================================
--- subversion/libsvn_subr/simple_providers.c (revision 1835628)
+++ subversion/libsvn_subr/simple_providers.c (working copy)
@@ -206,7 +206,7 @@
{
if (have_passtype)
{
- svn_boolean_t done;
+ svn_boolean_t done = FALSE;
SVN_ERR(password_get(&done, &default_password, creds_hash,
realmstring, username, parameters,
Re: [PATCH] Fix segfault when gnome keyring lookup fails
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
>[...] committed the patch --- r1835760 --- and proposed it for
> backport to to 1.10.x [...]
This fix will be in 1.10.1, along with some other gnome keyring fixes.
- Julian
Re: [PATCH] Fix segfault when gnome keyring lookup fails
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Jan Palus wrote on Thu, 12 Jul 2018 20:23 +0200:
> On 12.07.2018 15:08, Daniel Shahaf wrote:
> > Thanks for the patch; however, I don't think that's the correct fix.
>
> To be honest I wasn't sure of that either but on the other hand I thought that
> having initialized state won't hurt.
>
Initializing the variable prevents the compiler from issueing "This
variable is used uninitialized" if some codepath uses that variable
before calling password_get() to initialize it. We tend to prefer
leaving variables uninitialized for this reason.
> > Does the following patch fix the problem? Speaking of which, could you explain
> > how to reproduce the problem?
>
> As expected the patch does fix the issue, however the question about reproducer
> is somewhat problematic. No matter what I try I cannot reproduce it when
> invoking subversion manually from command line (ie with empty environment). The
> problem manifests itself only when using automation script which involves
> gradle->svnant->native subversion binary. I've added basic debug statements and
> it appears following condition fails already:
...
> The main difference between invoking svn directly and through the script is
> value of uninitialized "done". In script it is always non-zero while invoked
> directly it's the opposite. If you manually force done to some random, non-zero
> value the reproducer goes down to:
>
> env -i svn co --no-auth-cache <url> --username user --password pass --non-interactive
Thanks. I'm not sure sure what's causing the observed difference in the
behaviour of the local variable 'done', but in any case the compiler is
within its rights to leave that variable uninitialized, so I've gone
ahead and committed the patch --- r1835760 --- and proposed it for
backport to to 1.10.x. It should be backported in time for 1.10.2
(possibly 1.10.1 but no guarantees).
Cheers,
Daniel
Re: [PATCH] Fix segfault when gnome keyring lookup fails
Posted by Philip Martin <ph...@codematters.co.uk>.
Jan Palus <jp...@fastmail.com> writes:
> On 12.07.2018 15:08, Daniel Shahaf wrote:
>> Thanks for the patch; however, I don't think that's the correct fix.
>
> To be honest I wasn't sure of that either but on the other hand I thought that
> having initialized state won't hurt.
>
>> Does the following patch fix the problem? Speaking of which, could
>> you explain
>> how to reproduce the problem?
>
> As expected the patch does fix the issue, however the question about reproducer
> is somewhat problematic.
A memory tool is one way to detect such problems. With valgrind I get:
==14195== Conditional jump or move depends on uninitialised value(s)
==14195== at 0x5A1C043: svn_auth__simple_creds_cache_get (simple_providers.c:246)
==14195== by 0xB938E79: simple_gnome_keyring_first_creds (gnome_keyring.c:437)
==14195== by 0x5A05BA6: svn_auth_first_credentials (auth.c:290)
==14195== by 0x6F40D56: svn_ra_serf__credentials_callback (util.c:1202)
==14195== by 0x9285093: serf__handle_basic_auth (auth_basic.c:97)
==14195== by 0x9284F34: handle_auth_headers (auth.c:200)
==14195== by 0x9284F34: dispatch_auth (auth.c:312)
==14195== by 0x9284F34: serf__handle_auth_response (auth.c:369)
==14195== by 0x927BE3F: handle_response (outgoing.c:929)
==14195== by 0x927BE3F: read_from_connection (outgoing.c:1136)
==14195== by 0x927BE3F: serf__process_connection (outgoing.c:1257)
==14195== by 0x927A73D: serf_event_trigger (context.c:231)
==14195== by 0x927A878: serf_context_run (context.c:305)
==14195== by 0x6F44680: svn_ra_serf__context_run (util.c:910)
==14195== by 0x6F419B2: svn_ra_serf__context_run_wait (util.c:981)
==14195== by 0x6F41A53: svn_ra_serf__context_run_one (util.c:1021)
--
Philip
Re: [PATCH] Fix segfault when gnome keyring lookup fails
Posted by Jan Palus <jp...@fastmail.com>.
On 12.07.2018 15:08, Daniel Shahaf wrote:
> Thanks for the patch; however, I don't think that's the correct fix.
To be honest I wasn't sure of that either but on the other hand I thought that
having initialized state won't hurt.
> Does the following patch fix the problem? Speaking of which, could you explain
> how to reproduce the problem?
As expected the patch does fix the issue, however the question about reproducer
is somewhat problematic. No matter what I try I cannot reproduce it when
invoking subversion manually from command line (ie with empty environment). The
problem manifests itself only when using automation script which involves
gradle->svnant->native subversion binary. I've added basic debug statements and
it appears following condition fails already:
if (!available_collection(non_interactive, pool))
return SVN_NO_ERROR;
which fails on first call:
service = secret_service_get_sync(SECRET_SERVICE_NONE, NULL, &gerror);
if (gerror || !service)
goto error_return;
with gerror->message being "Cannot autolaunch D-Bus without X11 $DISPLAY"
The main difference between invoking svn directly and through the script is
value of uninitialized "done". In script it is always non-zero while invoked
directly it's the opposite. If you manually force done to some random, non-zero
value the reproducer goes down to:
env -i svn co --no-auth-cache <url> --username user --password pass --non-interactive
Regards
Jan
Re: [PATCH] Fix segfault when gnome keyring lookup fails
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Jan Palus wrote on Wed, Jul 11, 2018 at 13:46:35 +0200:
> [[[
> Fix segfault when subversion is built with gnome keyring support and keyring
> password lookup fails.
>
> * subversion/libsvn_subr/simple_providers.c
> (svn_auth__simple_creds_cache_get): Initialize 'done' with FALSE since ie
> password_get_gnome_keyring sets 'done' only to TRUE. In case of error
> it leaves unintialized 'done' (usually non 0) and default_password with NULL
> causing segfault at subsequent strcmp.
Thanks for the patch; however, I don't think that's the correct fix.
The svn_auth__password_get_t requires implementations (= password_get_gnome_keyring())
to either set *DONE to something or return an error (of type svn_error_t*).
password_get_gnome_keyring() violates that API, so password_get_gnome_keyring()
should be fixed.
Does the following patch fix the problem? Speaking of which, could you explain
how to reproduce the problem?
Does anyone want to audit the other related functions (vertically —
gnome_keyring.c — and horizontally — other svn_auth__password_get_t
implementations) to see whether any of them is also affected?
[[[
Index: subversion/libsvn_auth_gnome_keyring/gnome_keyring.c
===================================================================
--- subversion/libsvn_auth_gnome_keyring/gnome_keyring.c (revision 1835745)
+++ subversion/libsvn_auth_gnome_keyring/gnome_keyring.c (working copy)
@@ -118,6 +118,8 @@ password_get_gnome_keyring(svn_boolean_t *done,
{
GError *gerror = NULL;
gchar *gpassword;
+
+ *done = FALSE;
if (!available_collection(non_interactive, pool))
return SVN_NO_ERROR;
@@ -129,6 +131,7 @@ password_get_gnome_keyring(svn_boolean_t *done,
NULL);
if (gerror)
{
+ /* ### TODO: return or log the error? */
g_error_free(gerror);
}
else if (gpassword)
]]]
Cheers,
Daniel
(I don't recall whether I build with gnome-keyring...)