Julian Foad wrote on Thu, Mar 31, 2016 at 14:10:09 +0100: > On 28 March 2016 at 14:49, Ivan Zhakov <[email protected]> wrote: > > On 28 March 2016 at 16:37, Stefan Sperling <[email protected]> wrote: > >> On Mon, Mar 28, 2016 at 04:13:11PM +0300, Ivan Zhakov wrote: > >>> On 20 March 2016 at 01:18, <[email protected]> wrote: > >>> > { > >>> > if (err->apr_err == SVN_ERR_CANCELLED) > >>> > { > >>> > - svn_error_clear(err); > >>> > *may_save_plaintext = FALSE; > >>> > - return SVN_NO_ERROR; > >>> > + return err; > >>> Daniel, do you know what was the original idea behind ignoring the > >>> SVN_ERR_CANCELLED error? I see stsp committed the original code in > >>> r870804, so there's probably some rationale behind it. > >>> > >>> Stefan, do you remember any details? > >> > >> I don't think there was a special reason to ignore the error. > >> > >> The question is whether we want Ctrl-C to mean "no" at the plaintext > >> prompt, or whether it should abort the process. > >> > >> I agree with Daniel's patch. Ctrl-C should abort the process. > > > > OK. Thanks for clarification! I just wanted to double check that we're > > not missing something important. > > In that case, why use the "err = prompt(...; if err == CANCELLED ..." > construction? If we're returning an error, then the assignment to > "*may_save_plaintext" should not be required, right? (Neither this > function nor its two public callers promise to do anything special > when returning an error.) > > And so wouldn't it be equivalent and simpler to just write > > SVN_ERR(prompt(...)); > ? >
Looking at the surrounding code, that would makes sense: code simplification reduces the chance of future bugs. However, if we make this change, API callers that depend on the implemented (unpromised) behaviour — that is, API callers that assume the output parameter will be initialized even on error returns — will then decide whether to save the plaintext password to disk according to the value of uninitialized memory. That is: this change has potential security implications for third-party API users. Therefore, I'm tempted to treat this as an "incompatible" change: mention it in the 1.10 release notes and refrain from backporting it. Makes sense? Cheers, Daniel > - Julian >
