On Wed, Jun 14, 2023 at 11:38:15AM +0200, Claudio Jeker wrote:
> On Wed, Jun 14, 2023 at 11:10:52AM +0200, Theo Buehler wrote:
> > On Wed, Jun 14, 2023 at 10:44:23AM +0200, Claudio Jeker wrote:
> > > There is no real need to have print_host() with the extra arguments.
> > > So convert the last remaining print_host() calls to use print_addr().
> > > I'm not entierly sure how to really test all these code paths but the
> > > changes are failry simple.
> >
> > Thanks for beating me to it...
> >
> > In ikev2_pld_cp() you need to garbage collect buf and replace its
> > remaining uses by the appropriate print_addr().
>
> Oh boy, I missed the fact that this double prints the same info.
> I moved the first log_debug as default: of the switch statement.
Yeah, that makes sense.
> > In ikev2_print_id() it would seem more appropriate to use strlcat()
> > instead of strlcpy() in the switch and and reserve the special dance
> > of adjusting idstr and idstrlen for the /* XXX test */ path, but that
> > should be done in a separate diff.
>
> Oh boy, this is even worse than the above case. WTF!
Indeed.
> So many things are utterly bad in this function and the XXX test path is
> very broken. I tried to fix this up a bit.
Should s4 and s6 not be struct sockaddr_in{,6} on the stack and get a
treatment similar to start4 and start6 in ikev2_pld_ts()? This buf thing
seems completely nuts. The weird length check in the FQDN case could be
done with a simple comparison to BUFSIZ and then buf could become a
local 3-byte buffer in the for loop of the XXX test case.
However, I completely understand if you don't want to touch more of this
mess than you already did...
Just one thing, otherwise your diff is ok:
> @@ -6980,9 +6975,11 @@ ikev2_print_id(struct iked_id *id, char
> break;
> default:
> /* XXX test */
> - for (i = 0; i < ((ssize_t)idstrlen - 1) && i < len; i++)
> - snprintf(idstr + i, idstrlen - i,
> - "%02x", ptr[i]);
> + for (i = 0; i < len; i++) {
> + snprintf(buf, sizeof(buf), "%02x", ptr[i]);
> + if (strlcat(idstr, buf, idstrlen) >= idstrlen)
Should this not return -1 like the other cases?
> + break;
> + }
> break;
> }
