You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by da...@apache.org on 2016/03/19 23:18:35 UTC
svn commit: r1735826 - in /subversion/trunk: ./
subversion/libsvn_subr/prompt.c
Author: danielsh
Date: Sat Mar 19 22:18:35 2016
New Revision: 1735826
URL: http://svn.apache.org/viewvc?rev=1735826&view=rev
Log:
Merge r1735680 to trunk, which was accidentally committed to a different branch.
That revision's log message is:
------------------------------------------------------------------------
r1735680 | danielsh | 2016-03-18 21:10:34 +0000 (Fri, 18 Mar 2016) | 9 lines
Make SIGINT abort a commit, even at the interactive plaintext prompt.
(Issue #4624.)
Follow-up to r30730 (r870804).
Found by: Richlv
* subversion/libsvn_subr/prompt.c
(plaintext_prompt_helper): Propagate canncellations.
------------------------------------------------------------------------
Modified:
subversion/trunk/ (props changed)
subversion/trunk/subversion/libsvn_subr/prompt.c
Propchange: subversion/trunk/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Sat Mar 19 22:18:35 2016
@@ -2,6 +2,7 @@
/subversion/branches/1.5.x-r30215:870312
/subversion/branches/1.7.x-fs-verify:1146708,1161180
/subversion/branches/1.9-cache-improvements:1678948-1679863
+/subversion/branches/1.9.x:1735680
/subversion/branches/10Gb:1388102,1388163-1388190,1388195,1388202,1388205,1388211,1388276,1388362,1388375,1388394,1388636,1388639-1388640,1388643-1388644,1388654,1388720,1388789,1388795,1388801,1388805,1388807,1388810,1388816,1389044,1389276,1389289,1389662,1389867,1390017,1390209,1390216,1390407,1390409,1390414,1390419,1390955
/subversion/branches/atomic-revprop:965046-1000689
/subversion/branches/authzperf:1615360
Modified: subversion/trunk/subversion/libsvn_subr/prompt.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/prompt.c?rev=1735826&r1=1735825&r2=1735826&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/prompt.c (original)
+++ subversion/trunk/subversion/libsvn_subr/prompt.c Sat Mar 19 22:18:35 2016
@@ -831,9 +831,8 @@ plaintext_prompt_helper(svn_boolean_t *m
{
if (err->apr_err == SVN_ERR_CANCELLED)
{
- svn_error_clear(err);
*may_save_plaintext = FALSE;
- return SVN_NO_ERROR;
+ return err;
}
else
return err;
Re: svn commit: r1735826 - in /subversion/trunk: ./ subversion/libsvn_subr/prompt.c
Posted by Greg Stein <gs...@gmail.com>.
I see your commit has returned :-/
On Mon, Apr 4, 2016 at 6:06 AM, Daniel Shahaf <da...@apache.org> wrote:
> Greg Stein wrote on Sat, Apr 02, 2016 at 00:23:12 -0500:
> > On Fri, Apr 1, 2016 at 5:18 AM, Stefan Sperling <st...@elego.de> wrote:
> >
> > > On Fri, Apr 01, 2016 at 04:38:00AM -0500, Greg Stein wrote:
> > > > no no no ... we've always said that OUT parameters are not dependable
> > > when
> > > > an error occurs.
> > >
> > > Do our API docs mention this anywhere?
> > > Or was this information only passed around among SVN devs?
> > >
> >
> > Darn. http://subversion.apache.org/docs/community-guide/conventions.html
> > does not talk about it at all. Nor about our argument ordering
> conventions.
>
> Patch here:
> https://home.apache.org/~danielsh/28b4fc20e310ece9885af0dd3f5171db56c3384d.txt
>
> ldap is down so I can't commit it now. (Nor the
> plaintext_password_helper() patch from upthread)
>
>
>
Re: svn commit: r1735826 - in /subversion/trunk: ./
subversion/libsvn_subr/prompt.c
Posted by Daniel Shahaf <da...@apache.org>.
Greg Stein wrote on Sat, Apr 02, 2016 at 00:23:12 -0500:
> On Fri, Apr 1, 2016 at 5:18 AM, Stefan Sperling <st...@elego.de> wrote:
>
> > On Fri, Apr 01, 2016 at 04:38:00AM -0500, Greg Stein wrote:
> > > no no no ... we've always said that OUT parameters are not dependable
> > when
> > > an error occurs.
> >
> > Do our API docs mention this anywhere?
> > Or was this information only passed around among SVN devs?
> >
>
> Darn. http://subversion.apache.org/docs/community-guide/conventions.html
> does not talk about it at all. Nor about our argument ordering conventions.
Patch here: https://home.apache.org/~danielsh/28b4fc20e310ece9885af0dd3f5171db56c3384d.txt
ldap is down so I can't commit it now. (Nor the
plaintext_password_helper() patch from upthread)
Re: svn commit: r1735826 - in /subversion/trunk: ./ subversion/libsvn_subr/prompt.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Apr 1, 2016 at 5:18 AM, Stefan Sperling <st...@elego.de> wrote:
> On Fri, Apr 01, 2016 at 04:38:00AM -0500, Greg Stein wrote:
> > no no no ... we've always said that OUT parameters are not dependable
> when
> > an error occurs.
>
> Do our API docs mention this anywhere?
> Or was this information only passed around among SVN devs?
>
Darn. http://subversion.apache.org/docs/community-guide/conventions.html
does not talk about it at all. Nor about our argument ordering conventions.
Re: svn commit: r1735826 - in /subversion/trunk: ./
subversion/libsvn_subr/prompt.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Apr 01, 2016 at 04:38:00AM -0500, Greg Stein wrote:
> no no no ... we've always said that OUT parameters are not dependable when
> an error occurs.
Do our API docs mention this anywhere?
Or was this information only passed around among SVN devs?
I don't disagree. But we should clearly communicate such details in our
documentation to avoid surprises. We cannot assume that our API users
have implicit knowlege of #svn-dev conventions.
For instance, the requirement to canonicalize paths is clearly documented.
Re: svn commit: r1735826 - in /subversion/trunk: ./ subversion/libsvn_subr/prompt.c
Posted by Julian Foad <ju...@apache.org>.
On 2 April 2016 at 04:02, Daniel Shahaf <da...@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
Re: svn commit: r1735826 - in /subversion/trunk: ./ subversion/libsvn_subr/prompt.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Apr 1, 2016 at 10:02 PM, Daniel Shahaf <da...@apache.org> wrote:
> Greg Stein wrote on Fri, Apr 01, 2016 at 04:38:00 -0500:
> > On Fri, Apr 1, 2016 at 12:36 AM, Daniel <da...@apache.org> wrote:
> >
> > > ...
> > > 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.
> > >
> >
> > no no no ... we've always said that OUT parameters are not dependable
> when
> > an error occurs. I see no reason to change here.
>
> I don't dispute that. We do not promise to initialize the value of an
> output argument on error.
>
> > Especially no reason to claim an API change/errata.
>
> 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 this situation ... I would concur. The safe/conservative position is
appropriate when we're talking about passwords. Very good point.
Thx,
-g
Re: svn commit: r1735826 - in /subversion/trunk: ./
subversion/libsvn_subr/prompt.c
Posted by Daniel Shahaf <da...@apache.org>.
Greg Stein wrote on Fri, Apr 01, 2016 at 04:38:00 -0500:
> On Fri, Apr 1, 2016 at 12:36 AM, Daniel <da...@apache.org> wrote:
>
> > ...
> > 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.
> >
>
> no no no ... we've always said that OUT parameters are not dependable when
> an error occurs. I see no reason to change here.
I don't dispute that. We do not promise to initialize the value of an
output argument on error.
> Especially no reason to claim an API change/errata.
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
¹ "Might" because whether or not the failure mode materializes depends
on the actual value of the API consumer's uninitialized memory.
> >...
>
> Cheers,
> -g
Re: svn commit: r1735826 - in /subversion/trunk: ./ subversion/libsvn_subr/prompt.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Apr 1, 2016 at 12:36 AM, Daniel <da...@apache.org> wrote:
> ...
> 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.
>
no no no ... we've always said that OUT parameters are not dependable when
an error occurs. I see no reason to change here. Especially no reason to
claim an API change/errata.
>...
Cheers,
-g
Re: svn commit: r1735826 - in /subversion/trunk: ./
subversion/libsvn_subr/prompt.c
Posted by Daniel <da...@apache.org>.
Julian Foad wrote on Thu, Mar 31, 2016 at 14:10:09 +0100:
> On 28 March 2016 at 14:49, Ivan Zhakov <iv...@visualsvn.com> wrote:
> > On 28 March 2016 at 16:37, Stefan Sperling <st...@apache.org> wrote:
> >> On Mon, Mar 28, 2016 at 04:13:11PM +0300, Ivan Zhakov wrote:
> >>> On 20 March 2016 at 01:18, <da...@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
>
Re: svn commit: r1735826 - in /subversion/trunk: ./ subversion/libsvn_subr/prompt.c
Posted by Julian Foad <ju...@apache.org>.
On 28 March 2016 at 14:49, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 28 March 2016 at 16:37, Stefan Sperling <st...@apache.org> wrote:
>> On Mon, Mar 28, 2016 at 04:13:11PM +0300, Ivan Zhakov wrote:
>>> On 20 March 2016 at 01:18, <da...@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(...));
?
- Julian
Re: svn commit: r1735826 - in /subversion/trunk: ./ subversion/libsvn_subr/prompt.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 28 March 2016 at 16:37, Stefan Sperling <st...@apache.org> wrote:
> On Mon, Mar 28, 2016 at 04:13:11PM +0300, Ivan Zhakov wrote:
>> On 20 March 2016 at 01:18, <da...@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.
--
Ivan Zhakov
Re: svn commit: r1735826 - in /subversion/trunk: ./
subversion/libsvn_subr/prompt.c
Posted by Stefan Sperling <st...@apache.org>.
On Mon, Mar 28, 2016 at 04:13:11PM +0300, Ivan Zhakov wrote:
> On 20 March 2016 at 01:18, <da...@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.
Re: svn commit: r1735826 - in /subversion/trunk: ./ subversion/libsvn_subr/prompt.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 20 March 2016 at 01:18, <da...@apache.org> wrote:
> Author: danielsh
> Date: Sat Mar 19 22:18:35 2016
> New Revision: 1735826
>
> URL: http://svn.apache.org/viewvc?rev=1735826&view=rev
> Log:
> Merge r1735680 to trunk, which was accidentally committed to a different branch.
>
> That revision's log message is:
>
> ------------------------------------------------------------------------
> r1735680 | danielsh | 2016-03-18 21:10:34 +0000 (Fri, 18 Mar 2016) | 9 lines
>
> Make SIGINT abort a commit, even at the interactive plaintext prompt.
> (Issue #4624.)
>
> Follow-up to r30730 (r870804).
>
> Found by: Richlv
>
> * subversion/libsvn_subr/prompt.c
> (plaintext_prompt_helper): Propagate canncellations.
> ------------------------------------------------------------------------
>
[...]
> Modified: subversion/trunk/subversion/libsvn_subr/prompt.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/prompt.c?rev=1735826&r1=1735825&r2=1735826&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/prompt.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/prompt.c Sat Mar 19 22:18:35 2016
> @@ -831,9 +831,8 @@ plaintext_prompt_helper(svn_boolean_t *m
> {
> 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?
--
Ivan Zhakov