On Tue, Aug 4, 2020 at 5:25 PM Martin Lewis <[email protected]> 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 <[email protected]>
> ---
> 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
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
