subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Julian Foad <julianf...@apache.org>
Subject Re: svn commit: r1735826 - in /subversion/trunk: ./ subversion/libsvn_subr/prompt.c
Date Sun, 03 Apr 2016 18:16:56 GMT
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.ยน  I do not consider that an acceptable result of a code simplification.
>
> 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
> ===================================================================
> --- 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 = NULL;
>    terminal_handle_t *terminal;
>
> +  *may_save_plaintext = FALSE; /* de facto API promise for 1.9.x and earlier */
> +
>    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 = prompt(&answer, prompt_string, FALSE, pb, pool);
> -      if (err)
> -        {
> -          if (err->apr_err == SVN_ERR_CANCELLED)
> -            {
> -              *may_save_plaintext = FALSE;
> -              return err;
> -            }
> -          else
> -            return err;
> -        }
> +      SVN_ERR(prompt(&answer, prompt_string, FALSE, pb, pool));
>        if (apr_strnatcasecmp(answer, _("yes")) == 0 ||
>            apr_strnatcasecmp(answer, _("y")) == 0)
>          {
> ]]]
>
> Cheers,
>
> Daniel

+1 on this reasoning and solution.

- Julian

Mime
View raw message