Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20160402030203.GA4984@tarsus.local2>
References: <20160319221835.DAED23A0051@svn01-us-west.apache.org>
 <CABw-3Yf=_p2yt=-pJ3Vq8qw3UYjpbyyZbmC=h-RJv0NNroHwHA@mail.gmail.com>
 <20160328133749.GA84858@jim.stsp.name>
 <CABw-3YeEi3SzNUv_G3S3_YgtGbFP2Jh-PVajSqyhA=Vz6Enc5w@mail.gmail.com>
 <CAEcU=1ZtytJOTRAQffW-2mLKV60BYUQHc2KibtCfebihRJ8NcA@mail.gmail.com>
 <20160401053634.GA6626@tarsus.local2>
 <CABD8fLVx=81Et_bkGOq7JPCo6vqTnr-u7QtY+zieXnqpxnnRVg@mail.gmail.com>
 <20160402030203.GA4984@tarsus.local2>
From: Julian Foad <julianfoad@apache.org>
Date: Sun, 3 Apr 2016 19:16:56 +0100
Message-ID: 
 <CAEcU=1Yx7AtgM4rtf3Ud3N=Gm_QOjoP4GLi1Ro_kCgAZsqbzKw@mail.gmail.com>
Subject: Re: svn commit: r1735826 - in /subversion/trunk: ./
 subversion/libsvn_subr/prompt.c
To: Daniel Shahaf <danielsh@apache.org>
Cc: Greg Stein <gstein@gmail.com>, dev <dev@subversion.apache.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On 2 April 2016 at 04:02, Daniel Shahaf <danielsh@apache.org> wrote:
[...]
> Suppose an API consumer's code assumes the output variable would be
> initialized on error.  (Yes, it is a bug for the API consumer to make
> that assumption.)  Making the change Julian suggested might cause users
> of that API consumer to have their passwords stored in plain text on
> disk.=C2=B9  I do not consider that an acceptable result of a code simpli=
fication.
>
> In general, I have no problem with changing unpromised behaviour, so
> long as the promised behaviour is unaffected: if changing an unpromised
> behaviour breaks third-party code that relied on undocumented
> implementation details, then the authors of that code get to keep both
> pieces.  (They should have been using stable APIs.)
>
> However, in this case I prefer to take a more conservative approach,
> since I don't think "You get to keep both pieces" is an acceptable
> answer to a user who asks us why we consciously introduced a security
> hole while simplifying the code.
>
> As you say, that effectively means promising to continue initializing
> that *one particular output parameter* for 1.9.x and earlier.
>
> So, in short, I'd prefer this patch:
>
> [[[
> Index: subversion/libsvn_subr/prompt.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- subversion/libsvn_subr/prompt.c     (revision 1737454)
> +++ subversion/libsvn_subr/prompt.c     (working copy)
> @@ -814,6 +814,8 @@ plaintext_prompt_helper(svn_boolean_t *may_save_pl
>    const char *config_path =3D NULL;
>    terminal_handle_t *terminal;
>
> +  *may_save_plaintext =3D FALSE; /* de facto API promise for 1.9.x and e=
arlier */
> +
>    if (pb)
>      SVN_ERR(svn_config_get_user_config_path(&config_path, pb->config_dir=
,
>                                              SVN_CONFIG_CATEGORY_SERVERS,=
 pool));
> @@ -826,17 +828,7 @@ plaintext_prompt_helper(svn_boolean_t *may_save_pl
>
>    do
>      {
> -      svn_error_t *err =3D prompt(&answer, prompt_string, FALSE, pb, poo=
l);
> -      if (err)
> -        {
> -          if (err->apr_err =3D=3D SVN_ERR_CANCELLED)
> -            {
> -              *may_save_plaintext =3D FALSE;
> -              return err;
> -            }
> -          else
> -            return err;
> -        }
> +      SVN_ERR(prompt(&answer, prompt_string, FALSE, pb, pool));
>        if (apr_strnatcasecmp(answer, _("yes")) =3D=3D 0 ||
>            apr_strnatcasecmp(answer, _("y")) =3D=3D 0)
>          {
> ]]]
>
> Cheers,
>
> Daniel

+1 on this reasoning and solution.

- Julian