> From: Denys Vlasenko
> Sent: Sunday, October 17, 2010 4:56 PM
> To: Vladislav Grishenko
> Subject: Re: [PATCH] udhcpc: fixed option_ip_pair parsing
>
> On Sunday 17 October 2010 12:37, Vladislav Grishenko wrote:
> > > > revealed incorrect OPTION_IP_PAIR implementation, which doesn't
> > > > respect option length and causes erroneous classful routes,
> > > > composed from garbage or first bytes from the next DHCP packet
> option.
> > > > Patch is attached.
> > >
> > > Good catch!
> > > I propose the fix which is less mind-bending: remove all +/-4 tricks
> > > in OPTION_IP_PAIR (they are hard to follow and that's why they
> > > contained a sublte bug), and add ip_ofs variable which may be used to
> advance "option"
> > > pointer in OPTION_IP:
> > >
> > > while (len >= optlen) {
> > > + unsigned ip_ofs = 0;
> > >
> > > switch (type) {
> > > case OPTION_IP_PAIR:
> > > dest += sprint_nip(dest, "", option);
> > > *dest++ = '/';
> > > - option += 4;
> > > - optlen = 4;
> > > + ip_ofs = 4;
> > > /* fall through */
> > > case OPTION_IP:
> > > - dest += sprint_nip(dest, "", option);
> > > + dest += sprint_nip(dest, "", option +
> > > + ip_ofs);
> > > break;
> > >
> > >
> >
> > Yep, it looks cute.
> > Denys, do we really need to zero ip_opt in each loop? Option type remains
> const in this function.
>
> As it coded now, clever compiler can figure out that ip_ofs is never used past
> "case OPTION_IP", and can reuse its CPU register for something else.
> With ip_ofs moved out-of-loop, compiler will not be able to understand this,
> and will retain it for the entire loop body.
I see.
Just have to note that bloatcheck shows +64 now with ip_ofs variable vs +8
without with our mipsel-uclibc-gcc 4.3.5
Best Regards, theMIROn
ICQ: 303357
Skype: the.miron
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
