Greg Stein wrote on Fri, Apr 01, 2016 at 04:38:00 -0500:
> On Fri, Apr 1, 2016 at 12:36 AM, Daniel <danielsh_at_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
Received on 2016-04-02 05:02:06 CEST