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