Hi, On 03/12/17 11:39, Selva Nair wrote: > 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
oh ok. thanks for showing this.
>
>
>>>
>>>
>>>> +
>>>> + /* 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..
right. I am converting bits to unsigned long (which is what is returned
by strtoul()) and left max_bits as uint8_t.
Cheers,
--
Antonio Quartulli
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ 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
