PATCH: udhcpd "option domain" multiple values

Gabriel L. Somlo somlo at cmu.edu
Fri Feb 2 01:23:04 UTC 2007


On Thu, Feb 01, 2007 at 11:08:20PM +0100, Denis Vlasenko wrote:
> You can overflow existing->data[OPT_LEN].

Yes, by exactly 1 :)

>         /* add it to an existing option */

...

>                 if (existing->data[OPT_LEN] + length <= 255) {
>                         existing->data = xrealloc(existing->data,
>                                         existing->data[OPT_LEN] + length + 3);
>                         if ((option->flags & TYPE_MASK) == OPTION_STRING) {
>                                 if (existing->data[OPT_LEN] + length >= 255)
>                                         return;

You already know that's never going to be more than 255 (see the if
statement at the beginning of the quote :) What you don't know is
whether the extra space I want to add will make it 256 instead of 255.
So, maybe do this:

>                               if (existing->data[OPT_LEN] + length + 1>= 255)
                                                                     ^^^^^
>                                         return;

The other choice would be to do this:

>                 if (existing->data[OPT_LEN] + length <= 254) {
                                                        ^^^^^^^
>                         existing->data = xrealloc(existing->data,
>                                         existing->data[OPT_LEN] + length + 3);
>                         if ((option->flags & TYPE_MASK) == OPTION_STRING) {

and drop the second check entirely. I'd almost vote for this, but then
we'd be depriving non-string options of one potential byte. Makes the
code cleaner, though... I guess you're the decider :)

Otherwise, it compiles cleanly, and looks ok to me. Won't be able to
test it until some 15 hours from now, though, since I'll have to
physically hook up a dhcp client to the box which is in my office...

Cheers,
Gabriel



More information about the busybox mailing list