On Tue, Aug 4, 2020 at 5:25 PM Martin Lewis <martin.lewis....@gmail.com> wrote: > Duplicate options are currently overridden (only the last option is kept). > This leads to unexpected behavior when using long options. > > The patch adds support for long options in compliance with RFC 3396. > > Fixes #13136. > > function old new delta > udhcp_run_script 704 765 +61 > optitem_unset_env_and_free - 31 +31 > static.xmalloc_optname_optval 837 833 -4 > putenvp 60 52 -8 > ------------------------------------------------------------------------------ > (add/remove: 1/0 grow/shrink: 1/2 up/down: 92/-12) Total: 80 bytes > text data bss dec hex filename > 994091 16939 1872 1012902 f74a6 busybox_old > 994171 16939 1872 1012982 f74f6 busybox_unstripped > > Signed-off-by: Martin Lewis <martin.lewis....@gmail.com> > --- > networking/udhcp/dhcpc.c | 125 > ++++++++++++++++++++++++++++++++++------------- > networking/udhcp/dhcpc.h | 1 + > 2 files changed, 93 insertions(+), 33 deletions(-) > > diff --git a/networking/udhcp/dhcpc.c b/networking/udhcp/dhcpc.c > index 50dfead63..d4a7a825e 100644 > --- a/networking/udhcp/dhcpc.c > +++ b/networking/udhcp/dhcpc.c > @@ -115,6 +115,13 @@ enum { > > > /*** Script execution code ***/ > +struct dhcp_optitem > +{ > + unsigned len; > + uint8_t *data; > + char *env; > + uint8_t code; > +}; > > /* get a rough idea of how long an option will be (rounding up...) */ > static const uint8_t len_of_option_as_string[] ALIGN1 = { > @@ -186,15 +193,15 @@ static int good_hostname(const char *name) > #endif > > /* Create "opt_name=opt_value" string */ > -static NOINLINE char *xmalloc_optname_optval(uint8_t *option, const struct > dhcp_optflag *optflag, const char *opt_name) > +static NOINLINE char *xmalloc_optname_optval(const struct dhcp_optitem > *opt_item, const struct dhcp_optflag *optflag, const char *opt_name) > { > unsigned upper_length; > int len, type, optlen; > char *dest, *ret; > + uint8_t *option; > > - /* option points to OPT_DATA, need to go back to get OPT_LEN */ > - len = option[-OPT_DATA + OPT_LEN]; > - > + option = opt_item->data; > + len = opt_item->len; > type = optflag->flags & OPTION_TYPE_MASK; > optlen = dhcp_option_lengths[type]; > upper_length = len_of_option_as_string[type] > @@ -386,11 +393,50 @@ static NOINLINE char *xmalloc_optname_optval(uint8_t > *option, const struct dhcp_ > return ret; > } > > -static void putenvp(llist_t **envp, char *new_opt) > +/* Used by static options (interface, siaddr, etc) */ > +static void putenvp(char *new_opt) > { > + struct dhcp_optitem *opt_item; > + opt_item = xzalloc(sizeof(struct dhcp_optitem)); > + /* Set opt_item->code = 0, so it won't appear in concat_option's > lookup. */ > + opt_item->env = new_opt; > putenv(new_opt); > - log2(" %s", new_opt); > - llist_add_to(envp, new_opt); > + llist_add_to(&client_data.envp, opt_item); > +}
This removed logging of environment > +/* Support RFC3396 Long Encoded Options */ > +static struct dhcp_optitem *concat_option(uint8_t *data, uint8_t len, > uint8_t code) > +{ > + llist_t *item; > + struct dhcp_optitem *opt_item; > + unsigned new_len = len; > + > + /* Check if an option with the code already exists. > + * A possible optimization is to create a bitmap of all existing > options in the packet, > + * and iterate over the option list only if they exist. > + * This will result in bigger code, and because dhcp packets don't > have too many options it > + * shouldn't have a big impact on performance. > + */ > + for (item = client_data.envp; item != NULL; item = item->link) { > + opt_item = (struct dhcp_optitem *)item->data; > + if (opt_item->code == code) > + break; > + } > + > + if (item) { > + /* This is a duplicate, concatenate data according to RFC > 3396 */ > + new_len += opt_item->len; > + } else { > + /* This is a new option, add a new dhcp_optitem to the list */ > + opt_item = xzalloc(sizeof(struct dhcp_optitem)); > + opt_item->code = code; > + llist_add_to(&client_data.envp, opt_item); > + } > + > + opt_item->data = xrealloc(opt_item->data, new_len); /* xrealloc on > the first occurrence (NULL) will call malloc */ > + memcpy(opt_item->data + opt_item->len, data, len); > + opt_item->len = new_len; > + return opt_item; > } This will create a copy for every option, duplicated or not. Most options are not dups, I propose to track this status and only make copies as needed: most of the time, we won't make any: +static struct dhcp_optitem *concat_option(uint8_t *data, uint8_t len, uint8_t code) +{ + llist_t *item; + struct dhcp_optitem *opt_item; + + /* Check if an option with the code already exists. + * A possible optimization is to create a bitmap of all existing options in the packet, + * and iterate over the option list only if they exist. + * This will result in bigger code, and because dhcp packets don't have too many options it + * shouldn't have a big impact on performance. + */ + for (item = client_data.envp; item != NULL; item = item->link) { + opt_item = (struct dhcp_optitem *)item->data; + if (opt_item->code == code) { + /* This option was seen already, concatenate */ + uint8_t *new_data; + + new_data = xmalloc(len + opt_item->len); + memcpy( + mempcpy(new_data, opt_item->data, opt_item->len), + data, len + ); + opt_item->len += len; + if (opt_item->malloced) + free(opt_item->data); + opt_item->malloced = 1; + opt_item->data = new_data; + return opt_item; + } + } + + /* This is a new option, add a new dhcp_optitem to the list */ + opt_item = xzalloc(sizeof(*opt_item)); + opt_item->code = code; + /* opt_item->malloced = 0 */ + opt_item->data = data; + opt_item->len = len; + llist_add_to(&client_data.envp, opt_item); + return opt_item; } Applying with this change. Please test current git. _______________________________________________ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox