oops forgot to cc the list..
---------- Forwarded message ----------
From: Selva Nair <selva.n...@gmail.com>
Date: Sat, Dec 2, 2017 at 10:16 PM
Subject: Re: [Openvpn-devel] [PATCH v2] ifconfig-ipv6(-push): allow using
hostnames
To: Antonio Quartulli <a...@unstable.cc>
Hi,
On Sat, Dec 2, 2017 at 9:25 PM, Antonio Quartulli <a...@unstable.cc> wrote:
> Hi,
>
> On 03/12/17 04:27, Selva Nair wrote:
>
> >> + /* we need to modify the hostname received as input, but we don't
> >> want to
> >> + * touch it directly as it might be a constant string.
> >> + *
> >> + * Therefore, we clone the string here and free it at the end of
> the
> >> + * function */
> >> + var_host = strdup(hostname);
> >> + ASSERT(var_host);
> >>
> >
> > I think ASSERT should be used only to catch errors in coding logic,
> > not for plausible runtime errors like this. Especially since this happens
> > on the server, no reason to terminate the process here.
> >
> > Instead, log an error (M_NONFTAL|M_ERRNO will do nicely) and return -1
> > or goto out. The option parser will log a generic warning, but still
> useful
> > to log here using M_ERRNO for more specific info.
> >
>
> I agree. I am also not a big fan of ASSERT(). I think I had copy/pasted
> this chunk of code from somewhere else (and consider that this patch is
> quite old even though I have re-sent it now).
>
> I will get rid of the ASSERTs.
>
> > Alternatively one could use a buffer on the stack -- easy to do as good
> > old dns names are limited in size (255 octets max?) and the current
> option
> > parser also limits the argument passed here to < 255 bytes. But if we
> > support
> > internationalized domain names (currently we do not, do we?) and if the
> line
> > length in option parser is ever increased, a much larger buffer would be
> > needed.
>
> Are you sure we have a limit of 255 octects? I am not so sure. Anyway,
> this is not extremely important for now.
>
Yes, not important for this patch and stdup is anyway fine as long as we do
not
ASSERT.
The 255 byte limit comes from the config file or option parser which reads
the
input from ccd or client-connect script output. The length of a line is
limited
to #define OPTION_LINE_SIZE 256 in options.h
> >
> >
> >> +
> >> + /* check if this hostname has a /bits suffix */
> >> + sep = strchr(var_host , '/');
> >> + if (sep)
> >> + {
> >> + bits = strtoul(sep + 1, &endp, 10);
> >
> >
> > There are a number of such type coercions in the patch
> > (ulong to uint8_t, size_t to unit8_t etc.) that some compilers (aka
> MSVC:)
> > may warn about, but I do not personally care. All are safe and deliberate
> > except for the nitpicking below.
> >
> > + if ((*endp != '\0') || (bits > max_bits))
> >
> >
> > That (bits > max_bits) check will not catch many input errors, as the
> > input is already truncated to uint8_t. For example, /255 will be flagged
> > as an error, but /256 will pass as 0.
> >
>
> These are probably all copy/paste from other parts of the code. So the
> logic here is also used in other parts of the code. If we believe this
> is not the proper way to handle these cases, I'd suggest to take a note
> and fix these behaviors with a dedicated patch.
>
Please bear with me: Quoting from the patch:
- char *sep, *endp;
- int bits;
- struct in6_addr t_network;
So, the original code had bits defined as int, so why change to unit8_t?
Changing the original int to unsigned int would've made sense
as we are parsing using strtoul and returning the result in an unsigned int.
But uint8_t brings in hardly any gain and causes all those regressions and
type conversions I pointed out..
> Though its not possible to catch all input errors, keeping bits as
> unsigned
> > int (instead of uint8_t) may be better here. That'll also match netbits
> > in the function signature, return value of strtoul etc..
Regards,
Selva
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
