On 11 October 2016 at 22:11, Selva Nair <selva.n...@gmail.com> wrote:
> To nit-pick,

Yes, please!

> On Tue, Oct 11, 2016 at 3:35 PM, Steffan Karger <stef...@karger.me> wrote:
>>
>> +bool
>> +tls_check_ncp_cipher_list(const char *list) {
>> +  char *tmp_ciphers = string_alloc (list, NULL);
>> +  char *tmp_ciphers_orig = tmp_ciphers;
>> +  bool unsupported_cipher_found = false;
>> +
>> +  const char *token = strtok (tmp_ciphers, ":");
>> +  while (token)
>> +    {
>> +      if (!cipher_kt_get (translate_cipher_name_from_openvpn (token)))
>> +       {
>> +         msg (M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
>> +         unsupported_cipher_found = true;
>> +       }
>> +      token = strtok (NULL, ":");
>> +    }
>> +  free (tmp_ciphers_orig);
>
> the extra "tmp_ciphers_orig" is redundant. Just "free (tmp_ciphers)" will
> do.

Indeed, will fix.

>> +
>> +  return list && 0 < strlen(list) && !unsupported_cipher_found;
>
> Checking for list == NULL here looks too late. If list is null, so is
> tmp_ciphers and then the first call to strtok will not reset the internal
> state of strtok(). The behaviour of strtok then depends on whether any
> previous use of it was was iterated to the end or not.
>
> If list could be NULL, better check it and do an early return from the top.

list can not be NULL, but that should indeed be ASSERT()ed at the top,
instead of 'checked' here. Will fix too.

I'll send a v2 in a minute.

-Steffan

------------------------------------------------------------------------------
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

Reply via email to