Re: ssh nits
On Thu, 9 Mar 2023 at 06:50, joshua stein wrote: > On Thu, 09 Mar 2023 at 06:41:50 +1100, Darren Tucker wrote: > > This seems to be one too many parens? ie > > if (negate = (attrib[0] == '!')) > > clang warns if there's not the extra set of parens in case it's an > accidental = instead of ==. ok dtucker for this one. "no objection" to the sshpty.c one. -- Darren Tucker (dtucker at dtucker.net) GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA Good judgement comes with experience. Unfortunately, the experience usually comes from bad judgement.
Re: ssh nits
On Thu, 9 Mar 2023, Darren Tucker wrote:
> On Thu, 9 Mar 2023 at 02:09, joshua stein wrote:
> > cppcheck found these, are they worth fixing?
> >
> > In the non-fail case, done is set to NULL and then free()d.
> > free(NULL) is legal but maybe worth removing?
>
> ssh uses this pattern a lot, and I agree with millert that it's not
> worth changing.
>
> char *thing = NULL;
> [lots of stuff that might set thing]
> if (maybe()) {
> free(thing);
> thing = something;
> }
> [more stuff]
> free(thing)
>
> We actually went through and changed all the "if (thing) free(thing)"
> instances to drop the "if" some time ago.
>
> > grp == NULL fatal()s, so remove the ternary operations that will
> > never be the conditionals they aspire to be.
>
> That's true in OpenBSD, but not in -portable where the check is a
> debug only. That said, there's already a diff in this code block
> that'll stop future syncs from applying cleanly so I don't mind either
> way.
>
> > + if ((r = encode_dest_constraint_hop(b, >from)) != 0 ||
> > + (r = encode_dest_constraint_hop(b, >to)) != 0 ||
>
> I'll wait for djm to comment on this one.
>
> > + if ((negate = (attrib[0] == '!')))
>
> This seems to be one too many parens? ie
> if (negate = (attrib[0] == '!'))
>
> > - if ((r = parse_dest_constraint_hop(frombuf, >from) != 0) ||
> > - (r = parse_dest_constraint_hop(tobuf, >to) != 0))
> > + if ((r = parse_dest_constraint_hop(frombuf, >from)) != 0 ||
> > + (r = parse_dest_constraint_hop(tobuf, >to)) != 0)
>
> also djm.
ok djm for these
Re: ssh nits
On Thu, 09 Mar 2023 at 06:41:50 +1100, Darren Tucker wrote: > > + if ((negate = (attrib[0] == '!'))) > > This seems to be one too many parens? ie > if (negate = (attrib[0] == '!')) clang warns if there's not the extra set of parens in case it's an accidental = instead of ==. /usr/src/usr.bin/ssh/ssh/../readconf.c:605:14: warning: using the result of an assignment as a condition without parentheses [-Wparentheses] if (negate = (attrib[0] == '!'))
Re: ssh nits
On Thu, 9 Mar 2023 at 02:09, joshua stein wrote:
> cppcheck found these, are they worth fixing?
>
> In the non-fail case, done is set to NULL and then free()d.
> free(NULL) is legal but maybe worth removing?
ssh uses this pattern a lot, and I agree with millert that it's not
worth changing.
char *thing = NULL;
[lots of stuff that might set thing]
if (maybe()) {
free(thing);
thing = something;
}
[more stuff]
free(thing)
We actually went through and changed all the "if (thing) free(thing)"
instances to drop the "if" some time ago.
> grp == NULL fatal()s, so remove the ternary operations that will
> never be the conditionals they aspire to be.
That's true in OpenBSD, but not in -portable where the check is a
debug only. That said, there's already a diff in this code block
that'll stop future syncs from applying cleanly so I don't mind either
way.
> + if ((r = encode_dest_constraint_hop(b, >from)) != 0 ||
> + (r = encode_dest_constraint_hop(b, >to)) != 0 ||
I'll wait for djm to comment on this one.
> + if ((negate = (attrib[0] == '!')))
This seems to be one too many parens? ie
if (negate = (attrib[0] == '!'))
> - if ((r = parse_dest_constraint_hop(frombuf, >from) != 0) ||
> - (r = parse_dest_constraint_hop(tobuf, >to) != 0))
> + if ((r = parse_dest_constraint_hop(frombuf, >from)) != 0 ||
> + (r = parse_dest_constraint_hop(tobuf, >to)) != 0)
also djm.
--
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
Re: ssh nits
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, >from) != 0) ||
> - (r = encode_dest_constraint_hop(b, >to) != 0) ||
> + if ((r = encode_dest_constraint_hop(b, >from)) != 0 ||
> + (r = encode_dest_constraint_hop(b, >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, >from) != 0) ||
> - (r = parse_dest_constraint_hop(tobuf, >to) != 0))
> + if ((r = parse_dest_constraint_hop(frombuf, >from)) != 0 ||
> + (r = parse_dest_constraint_hop(tobuf, >to)) != 0)
> goto out; /* already logged */
> if (elen != 0) {
> error_f("unsupported extensions (len %zu)", elen);
ssh nits
cppcheck found these, are they worth fixing?
In the non-fail case, done is set to NULL and then free()d.
free(NULL) is legal but maybe worth removing?
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,
size_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.
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.
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
dest_constraint *dc)
if ((b = sshbuf_new()) == NULL)
return SSH_ERR_ALLOC_FAIL;
- if ((r = encode_dest_constraint_hop(b, >from) != 0) ||
- (r = encode_dest_constraint_hop(b, >to) != 0) ||
+ if ((r = encode_dest_constraint_hop(b, >from)) != 0 ||
+ (r = encode_dest_constraint_hop(b, >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_constraint *dc)
error_fr(r, "parse");
goto out;
}
- if ((r = parse_dest_constraint_hop(frombuf, >from) != 0) ||
- (r = parse_dest_constraint_hop(tobuf, >to) != 0))
+ if ((r = parse_dest_constraint_hop(frombuf, >from)) != 0 ||
+ (r = parse_dest_constraint_hop(tobuf, >to)) != 0)
goto out; /* already logged */
if (elen != 0) {
error_f("unsupported extensions (len %zu)", elen);
