Hi, On Fri, Jul 10, 2020 at 06:42:18PM +0200, Jan Just Keijser wrote: > On 08/07/20 10:24, Gert Doering wrote: > > Can I have a v4, please? :-) > V4:
Okay, here we go...
Generally speaking, it works now :-)
In the "ipconfig /all" output, I can now see a long list of DNS suffixes.
Together with a DNS search list on the LAN *and* block-outside-dns, this
seems to have interesting effects on Win10 - namely, it tries all the
search domains it has (on all interfaces), but it will try the "LAN"
search domains only via the "LAN" nameservers - and these are blocked,
so there might be interesting side effects if both are used togehter -
*or* it might be a way to get Win10 DNS to behave better even without
block-outside-DNS (by configuring a search domain on the TAP side).
Codewise, a few remarks...
> @@ -1145,6 +1146,19 @@ parse_hash_fingerprint(const char *str, int nbytes,
> int msglevel, struct gc_aren
> #ifndef ENABLE_SMALL
>
> static void
> +show_dhcp_option_list(const char *name, const char * const*array, int len)
> +{
> + int i;
> + for (i = 0; i < len; ++i)
> + {
> + msg(D_SHOW_PARMS, " %s[%d] = %s",
> + name,
> + i,
> + array[i] );
> + }
> +}
This is not "forbidden" by the style guide, but I think a more compact
form would increase readability - the old openvpn style of having
individual function calls spread over 10+ lines because each parameter
is getting its own line is not really something we do anymore. So:
> + msg(D_SHOW_PARMS, " %s[%d] = %s", name, i, array[i] );
... which is well below the 80 character limit and which I find more
readable, tbh.
> +/*
> + * RFC3397 states that multiple searchdomains are encoded as follows:
> + * - at start the length of the entire option is given
> + * - each subdomain is preceded by its length
> + * - each searchdomain is separated by a NUL character
> + * e.g. if you want "openvpn.net" and "duckduckgo.com" then you end up with
> + * 0x13 0x7 openvpn 0x3 net 0x00 0x0A duckduckgo 0x3 com 0x00
While at it, 0x1D :-)
> + */
> +static void
> +write_dhcp_search_str(struct buffer *buf, const int type, const char * const
> *str_array,
> + int array_len, bool *error)
> +{
> + char tmp_buf[256];
> + int i;
> + int len = 0;
> +
> + for (i=0; i < array_len; i++)
> + {
> + const char *ptr = str_array[i], *dotptr = str_array[i];
> + int j, k;
> +
> + msg(M_INFO, "Processing '%s'", ptr);
This line should not stay as it is - if you see it in the log, it's
mostly unclear "it's processing 'domain' - what for?", and M_INFO is
too high for unspecific info.
Most other DHCP activities do not log anything (unless error), so for
consistency I would just remove it. But if we keep it, it needs to
be more clear, like
> + msg(D_DHCP_OPT, "dhcp search domain '%s'", ptr);
or something like that.
> + if (strlen(ptr) + len + 1 > sizeof(tmp_buf))
> + {
> + *error = true;
> + msg(M_WARN, "write_dhcp_search_str: temp buffer overflow
> building DHCP options");
> + return;
> + }
> + /* Loop over all subdomains separated by a dot and replace the dot
> + with the length of the subdomain */
> + while ((dotptr = strchr(ptr, '.')) != NULL)
> + {
> + j = dotptr - ptr;
> + tmp_buf[len++] = j;
> + for (k=0; k < j; k++) tmp_buf[len++] = ptr[k];
Side note: this violate coding style - for(), as if(), mandates brackets.
> + for (k=0; k < j; k++)
{
tmp_buf[len++] = ptr[k];
}
> + ptr = dotptr + 1;
> + }
> +
> + /* Now do the remainder after the last dot */
> + j = strlen(ptr);
> + tmp_buf[len++] = j;
> + for (k=0; k < j; k++) tmp_buf[len++] = ptr[k];
same here.
> +
> + /* And close off with an extra NUL char */
> + tmp_buf[len++] = 0;
> + }
With these required brackets, the nested loops get long - and I do not
find them overly elegant anyway (one could do a memcpy(), which would
make more clear what is done, but it's still double-looping).
For this, I had the idea to code it as follows (mentioned yesterday, but
now actually written down) - single pass, no nested loops:
/* Loop over all subdomains separated by a dot and replace the dot
with the length of the subdomain */
/* label_length_pos points to the byte to be replaced by the length
* of the following domain label */
int label_length_pos = len++;
while(true)
{
if (*ptr == '.' || *ptr == '\0' )
{
tmp_buf[label_length_pos] = (len-label_length_pos)-1;
label_length_pos = len;
if (*ptr == '\0')
{
break;
}
}
tmp_buf[len++] = *ptr++;
}
/* And close off with an extra NUL char */
tmp_buf[len++] = '\0';
(I've actually tested this and it works :-) - wireshark and Win10 confirms
that the result is good - so if you want it, just use it for v5, if not,
leave it)
> +
> + if (!buf_safe(buf, 2 + len))
> + {
> + *error = true;
> + msg(M_WARN, "write_search_dhcp_str: buffer overflow building DHCP
> options");
> + return;
> + }
> + if (len > 255)
> + {
> + *error = true;
> + msg(M_WARN, "write_dhcp_search_str: search domain string must be <=
> 255 bytes");
> + return;
> + }
> +
> + buf_write_u8(buf, type);
> + buf_write_u8(buf, len);
> + for (i=0; i < len; i++) buf_write_u8(buf, tmp_buf[i]);
> +}
There's another set of brackets needed here... { buf_write_u8() }
But why use buf_write_u8() and another for() loop at all?
buf_write(buf, tmp_buf, len);
should do the job just fine...
So - getting close. Technological issues solved, now style issues
(which are much harder at times :-) ).
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
