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