Hello,
On Mon, Mar 29, 2021 at 11:56:40AM +0000, Schreilechner, Dominik wrote:
> </snip>
>
> > > I think the changes in ip_insertoptions() can be dropped completely,
> > > because the if-statement uses ip-ip_hl, which might not be initialized.
> >
> > yes, you are right, I think we should rather go for this tweak:
> >
> > --------8<---------------8<---------------8<------------------8<--------
> >
> > diff --git a/sys/netinet/ip_output.c b/sys/netinet/ip_output.c
> > index e19b744fdf3..5c1222c0c86 100644
> > --- a/sys/netinet/ip_output.c
> > +++ b/sys/netinet/ip_output.c
> > @@ -767,7 +767,7 @@ ip_insertoptions(struct mbuf *m, struct mbuf *opt, int
> > *phlen)
> > return (m); /* XXX should fail */
> >
> > /* check if options will fit to IP header */
> > - if ((optlen + (ip->ip_hl << 2)) > (0x0f << 2)) {
> > + if ((optlen + (sizeof (struct ip))) > (0x0f << 2)) {
> > *phlen = sizeof (struct ip);
> > return (m);
> > }
> > --------8<---------------8<---------------8<------------------8<--------
>
> Looks good for me. Just one more remark. In all other failure-cases in
> ip_insertoptions() the old mbuf is returned without setting phlen.
> Maybe, for the sake of consistence, always or never set phlen in a
> failure-case?
> ip_output() and icmp_send() do initialize (p)hlen before calling
> ip_insertoptions(). So it is not strictly necessary to set it within
> ip_insertoptions().
>
I plan to look at failure cases over Easter break (hopefully).
I think we should just drop the response packet if we fail to
add options. I'd like to have a better story for 'XXX should fail'.
I'll keep your not on my mind, when I'll get to it.
thanks and
regards
sashan
