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

Re: svn commit: r1735826 - in /subversion/trunk: ./ subversion/libsvn_subr/prompt.c

From: Julian Foad <julianfoad_at_apache.org>
Date: Sun, 3 Apr 2016 19:16:56 +0100

On 2 April 2016 at 04:02, Daniel Shahaf <danielsh_at_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
Received on 2016-04-03 20:17:19 CEST

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