On 2 April 2016 at 04:02, Daniel Shahaf <[email protected]> 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
