On Wed, 08 Mar 2023 09:02:08 -0600, joshua stein wrote:
> In the non-fail case, done is set to NULL and then free()d.
> free(NULL) is legal but maybe worth removing?
Please leave this as-is. I don't think it is worth appeasing
cppcheck in this case.
> diff --git usr.bin/ssh/scp.c usr.bin/ssh/scp.c
> index f0f09bba623..acb7bd8a8a1 100644
> --- usr.bin/ssh/scp.c
> +++ usr.bin/ssh/scp.c
> @@ -935,19 +935,21 @@ brace_expand(const char *pattern, char ***patternsp, si
> ze_t *npatternsp)
> *npatternsp = ndone;
> done = NULL;
> ndone = 0;
> ret = 0;
> fail:
> for (i = 0; i < nactive; i++)
> free(active[i]);
> free(active);
> - for (i = 0; i < ndone; i++)
> - free(done[i]);
> - free(done);
> + if (done) {
> + for (i = 0; i < ndone; i++)
> + free(done[i]);
> + free(done);
> + }
> return ret;
> }
>
> static struct sftp_conn *
> do_sftp_connect(char *host, char *user, int port, char *sftp_direct,
> int *reminp, int *remoutp, int *pidp)
> {
> if (sftp_direct == NULL) {
>
>
> grp == NULL fatal()s, so remove the ternary operations that will
> never be the conditionals they aspire to be.
OK
> diff --git usr.bin/ssh/sshpty.c usr.bin/ssh/sshpty.c
> index faf8960cfb5..690263a8cf3 100644
> --- usr.bin/ssh/sshpty.c
> +++ usr.bin/ssh/sshpty.c
> @@ -143,8 +143,8 @@ pty_setowner(struct passwd *pw, const char *tty)
> grp = getgrnam("tty");
> if (grp == NULL)
> fatal("no tty group");
> - gid = (grp != NULL) ? grp->gr_gid : pw->pw_gid;
> - mode = (grp != NULL) ? 0620 : 0600;
> + gid = grp->gr_gid;
> + mode = 0620;
>
> /*
> * Change owner and mode of the tty as required.
>
>
> These parentheses checking the result of an assignment were
> confusing, so move them.
OK. This will result in encode_dest_constraint() and
parse_dest_constraint() returning an SSH_ERR_* instead of 1 on error
but that seems like an improvement. It won't affect the caller's
logic as far as I can tell. I would wait for an OK from djm@ though.
> diff --git usr.bin/ssh/authfd.c usr.bin/ssh/authfd.c
> index 4b81b385637..05011f8c5c9 100644
> --- usr.bin/ssh/authfd.c
> +++ usr.bin/ssh/authfd.c
> @@ -489,8 +489,8 @@ encode_dest_constraint(struct sshbuf *m, const struct des
> t_constraint *dc)
>
> if ((b = sshbuf_new()) == NULL)
> return SSH_ERR_ALLOC_FAIL;
> - if ((r = encode_dest_constraint_hop(b, &dc->from) != 0) ||
> - (r = encode_dest_constraint_hop(b, &dc->to) != 0) ||
> + if ((r = encode_dest_constraint_hop(b, &dc->from)) != 0 ||
> + (r = encode_dest_constraint_hop(b, &dc->to)) != 0 ||
> (r = sshbuf_put_string(b, NULL, 0)) != 0) /* reserved */
> goto out;
> if ((r = sshbuf_put_stringb(m, b)) != 0)
> diff --git usr.bin/ssh/readconf.c usr.bin/ssh/readconf.c
> index e9d3a756896..81456c9b6d3 100644
> --- usr.bin/ssh/readconf.c
> +++ usr.bin/ssh/readconf.c
> @@ -602,7 +602,7 @@ match_cfg_line(Options *options, char **condition, struct
> passwd *pw,
> }
> arg = criteria = NULL;
> this_result = 1;
> - if ((negate = attrib[0] == '!'))
> + if ((negate = (attrib[0] == '!')))
> attrib++;
> /* Criterion "all" has no argument and must appear alone */
> if (strcasecmp(attrib, "all") == 0) {
> diff --git usr.bin/ssh/ssh-agent.c usr.bin/ssh/ssh-agent.c
> index 0e4d7f675ab..de1cdb049a2 100644
> --- usr.bin/ssh/ssh-agent.c
> +++ usr.bin/ssh/ssh-agent.c
> @@ -1010,8 +1010,8 @@ parse_dest_constraint(struct sshbuf *m, struct dest_con
> straint *dc)
> error_fr(r, "parse");
> goto out;
> }
> - if ((r = parse_dest_constraint_hop(frombuf, &dc->from) != 0) ||
> - (r = parse_dest_constraint_hop(tobuf, &dc->to) != 0))
> + if ((r = parse_dest_constraint_hop(frombuf, &dc->from)) != 0 ||
> + (r = parse_dest_constraint_hop(tobuf, &dc->to)) != 0)
> goto out; /* already logged */
> if (elen != 0) {
> error_f("unsupported extensions (len %zu)", elen);
