Julian Foad wrote on Thu, Mar 31, 2016 at 14:10:09 +0100:
> On 28 March 2016 at 14:49, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> > On 28 March 2016 at 16:37, Stefan Sperling <stsp_at_apache.org> wrote:
> >> On Mon, Mar 28, 2016 at 04:13:11PM +0300, Ivan Zhakov wrote:
> >>> On 20 March 2016 at 01:18, <danielsh_at_apache.org> 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
>
Received on 2016-04-01 07:36:37 CEST