subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel <danie...@apache.org>
Subject Re: svn commit: r1735826 - in /subversion/trunk: ./ subversion/libsvn_subr/prompt.c
Date Fri, 01 Apr 2016 05:36:34 GMT
Julian Foad wrote on Thu, Mar 31, 2016 at 14:10:09 +0100:
> On 28 March 2016 at 14:49, Ivan Zhakov <ivan@visualsvn.com> wrote:
> > On 28 March 2016 at 16:37, Stefan Sperling <stsp@apache.org> wrote:
> >> On Mon, Mar 28, 2016 at 04:13:11PM +0300, Ivan Zhakov wrote:
> >>> On 20 March 2016 at 01:18,  <danielsh@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
> 

Mime
View raw message