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);