On Wed, Jun 14, 2023 at 02:30:44PM +0200, Claudio Jeker wrote:
> On Wed, Jun 14, 2023 at 12:37:35PM +0200, Theo Buehler wrote:
> > 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...
>
> It never pays out to be lazy. Fixed in diff below.
> Guess that saves around 990bytes of stack :)
>
> > 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?
> >
>
> From my understanding the idea is that we try to dump as much of the
> unknown playload as possible. The code before did that with this
> complicated logic:
> i < ((ssize_t)idstrlen - 1) && i < len
> I decided to just fill the buffer until strlcat() complains and exit the
> loop then.
I see. I'm fine with that. Given that that was broken the same way
since import...
I can't for the life of me figure out what that (ssize_t)sizeof(buf)
check could have been about. It's been there since import, too. It seems
alright to drop it.
ok tb
