| Message ID | 20200401102159.29157-1-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Fix OpenSSL 1.1.1 not using auto ecliptic curve selection | expand |
Hi, On 01/04/2020 12:21, Arne Schwabe wrote: > --- > src/openvpn/misc.c | 18 ++++++++++++++++++ > src/openvpn/misc.h | 13 +++++++++++++ > src/openvpn/ssl_mbedtls.c | 15 ++------------- > 3 files changed, 33 insertions(+), 13 deletions(-) > > diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c > index 1931149b..b375451f 100644 > --- a/src/openvpn/misc.c > +++ b/src/openvpn/misc.c > @@ -735,4 +735,22 @@ output_peer_info_env(struct env_set *es, const char *peer_info) > } > } > > +int get_num_elements(const char* string, char delimiter) > +{ > + int string_len = strlen(string); > + > + ASSERT(0 != string_len); > + > + int element_count = 1; > + /* Get number of ciphers */ > + for (int i = 0; i < string_len; i++) > + { > + if (string[i] == delimiter) > + { > + element_count++; > + } > + } while copying this code you are breaking the indentation. note that 2 blanks before the curly brackets. that's nt supposed to be there. > + > + return element_count; > +} > #endif /* P2MP_SERVER */ > diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h > index 991b7df2..0655b7fe 100644 > --- a/src/openvpn/misc.h > +++ b/src/openvpn/misc.h > @@ -175,4 +175,17 @@ void output_peer_info_env(struct env_set *es, const char *peer_info); > > #endif /* P2MP_SERVER */ > > +/** > + * Counts the number of delimiter in a string and returns > + * their number +1. I'd rephrase this sentence as: Returns the occurrences of 'delimiter' in 'string' +1. > This is typically used to find out the > + * number elements in a cipher string or similar that is separated by : like > + * > + * X25519:secp256r1:X448:secp512r1:secp384r1:brainpoolP384r1 > + * > + * @param string the string to work on > + * @param delimiter the delimiter to count, typically ':' > + * @return number of delimiter found + 1 I'd change to "occurrences of 'delimiter' +1" > + */ > +int > +get_num_elements(const char* string, char delimiter); > #endif /* ifndef MISC_H */ > diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c > index 0f0b035b..0e17e734 100644 > --- a/src/openvpn/ssl_mbedtls.c > +++ b/src/openvpn/ssl_mbedtls.c > @@ -289,33 +289,22 @@ void > tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers) > { > char *tmp_ciphers, *tmp_ciphers_orig, *token; > - int i, cipher_count; > - int ciphers_len; > > if (NULL == ciphers) > { > return; /* Nothing to do */ > - > } > - ciphers_len = strlen(ciphers); > > ASSERT(NULL != ctx); > - ASSERT(0 != ciphers_len); > > /* Get number of ciphers */ > - for (i = 0, cipher_count = 1; i < ciphers_len; i++) > - { > - if (ciphers[i] == ':') > - { > - cipher_count++; > - } > - } > + int cipher_count = get_num_elements(ciphers, ':'); > > /* Allocate an array for them */ > ALLOC_ARRAY_CLEAR(ctx->allowed_ciphers, int, cipher_count+1) > > /* Parse allowed ciphers, getting IDs */ > - i = 0; > + int i = 0; > tmp_ciphers_orig = tmp_ciphers = string_alloc(ciphers, NULL); > > token = strtok(tmp_ciphers, ":"); > Other than my little nitpicks above, the patch looks good. However, I have a question. Since you are refactoring this code and this is going to master/2.5, why not reimplementing the get_num_elements() function using strtok() ? Regards,
>> > > Other than my little nitpicks above, the patch looks good. > However, I have a question. > > Since you are refactoring this code and this is going to master/2.5, why > not reimplementing the get_num_elements() function using strtok() ? > strsep/strok have the disadvantage of modifying the passed string, requring copying the string. strok in addition breaks on recursive calls. Also the for loop is very to understand and from playing with godbolt (https://godbolt.org/z/RVtjVL) it looks that the compilers will optimise/vectorise this function so using C string function will probably be even slower. So the simple and easy to understand function is the best choice here IMHO. Will send a V2 with the formatting/description fixed. Arne
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c index 1931149b..b375451f 100644 --- a/src/openvpn/misc.c +++ b/src/openvpn/misc.c @@ -735,4 +735,22 @@ output_peer_info_env(struct env_set *es, const char *peer_info) } } +int get_num_elements(const char* string, char delimiter) +{ + int string_len = strlen(string); + + ASSERT(0 != string_len); + + int element_count = 1; + /* Get number of ciphers */ + for (int i = 0; i < string_len; i++) + { + if (string[i] == delimiter) + { + element_count++; + } + } + + return element_count; +} #endif /* P2MP_SERVER */ diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h index 991b7df2..0655b7fe 100644 --- a/src/openvpn/misc.h +++ b/src/openvpn/misc.h @@ -175,4 +175,17 @@ void output_peer_info_env(struct env_set *es, const char *peer_info); #endif /* P2MP_SERVER */ +/** + * Counts the number of delimiter in a string and returns + * their number +1. This is typically used to find out the + * number elements in a cipher string or similar that is separated by : like + * + * X25519:secp256r1:X448:secp512r1:secp384r1:brainpoolP384r1 + * + * @param string the string to work on + * @param delimiter the delimiter to count, typically ':' + * @return number of delimiter found + 1 + */ +int +get_num_elements(const char* string, char delimiter); #endif /* ifndef MISC_H */ diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c index 0f0b035b..0e17e734 100644 --- a/src/openvpn/ssl_mbedtls.c +++ b/src/openvpn/ssl_mbedtls.c @@ -289,33 +289,22 @@ void tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers) { char *tmp_ciphers, *tmp_ciphers_orig, *token; - int i, cipher_count; - int ciphers_len; if (NULL == ciphers) { return; /* Nothing to do */ - } - ciphers_len = strlen(ciphers); ASSERT(NULL != ctx); - ASSERT(0 != ciphers_len); /* Get number of ciphers */ - for (i = 0, cipher_count = 1; i < ciphers_len; i++) - { - if (ciphers[i] == ':') - { - cipher_count++; - } - } + int cipher_count = get_num_elements(ciphers, ':'); /* Allocate an array for them */ ALLOC_ARRAY_CLEAR(ctx->allowed_ciphers, int, cipher_count+1) /* Parse allowed ciphers, getting IDs */ - i = 0; + int i = 0; tmp_ciphers_orig = tmp_ciphers = string_alloc(ciphers, NULL); token = strtok(tmp_ciphers, ":");