OpenSSH Linux portable patch proposal

[Darren Tucker]

On Wed, Jun 3, 2015 at 8:01 AM, Ángel González <keisial at gmail.com> wrote:

> On 02/06/15 15:46, György Demarcsek Ifj. wrote:
>
[...]

> You also removed a number of trailing spaces from the files, which make
> the patch harder to read.
>

We should clean up the trailing spaces upstream (there's more than I would
have thought) but that churn can wait until after the upcoming release.  I
agree that having no-op whitespace changes in the diff doesn't help.

Anyway, as to the diff itself:

+  if (authctxt->last_auth_methods == NULL) {
+    authctxt->last_auth_methods = xcalloc(strlen(method) + 2,
sizeof(char));

sizeof(char) == 1 by definition.

+  } else {
+    am_copy = xstrdup(authctxt->last_auth_methods);
+    free(authctxt->last_auth_methods);
+    authctxt->last_auth_methods = xcalloc(strlen(am_copy) + strlen(method)
+ 2, sizeof(char));
+    strcpy(authctxt->last_auth_methods, am_copy);

strcpy (and strcat) are not bounds checked (probably safe in this
particular case but we consider their use to be bad practice).

+    free(am_copy);
+  }
+
+  strcat(authctxt->last_auth_methods, method);
+  if (authctxt->num_auth_methods != 1)
+    strcat(authctxt->last_auth_methods, ",");

This seems to be an extremely complicated way of saying "append the method
to a comma separated list" (although it'll have a trailing comma, which I'm
not sure is intended.  How about something like:

if (authctxt->last_auth_methods == NULL)
    authctxt->last_auth_methods = xstrdup(method);
else {
    am_copy = authctxt->last_auth_methods;
    xasprintf(&authctxt->last_auth_methods, "%s,%s", am_copy, method);
    free(am_copy);
}

-- 
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4  37C9 C982 80C7 8FF4 FA69
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.