| Message ID | 20200217082942.173-1-lstipakov@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] ssl.c: refactor tls_item_in_cipher_list() code | expand |
Hi, On 17/02/2020 09:29, Lev Stipakov wrote: > From: Lev Stipakov <lev@openvpn.net> > > - remove unneeded _orig variable which stores pointer > to tokenized string. Unlike strsep(), strtok() doesn't modify > pointer itself, only string, so it is safe to call free() on that pointer. > this makes sense. We can just get rid of tmp_ciphers_orig and directly free tmp_ciphers. > - "token" points to content inside tokenized string. It becomes > dangling pointer after freeing that string. Check its value before > free() call. To me this change is not necessary and the reason behind it is wrong. (It is also ugly from the style perspective :-P) > > Signed-off-by: Lev Stipakov <lev@openvpn.net> > --- > src/openvpn/ssl.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c > index e708fc93..8cba3a76 100644 > --- a/src/openvpn/ssl.c > +++ b/src/openvpn/ssl.c > @@ -1918,7 +1918,6 @@ bool > tls_item_in_cipher_list(const char *item, const char *list) > { > char *tmp_ciphers = string_alloc(list, NULL); > - char *tmp_ciphers_orig = tmp_ciphers; > > const char *token = strtok(tmp_ciphers, ":"); > while (token) > @@ -1929,9 +1928,11 @@ tls_item_in_cipher_list(const char *item, const char *list) > } > token = strtok(NULL, ":"); > } > - free(tmp_ciphers_orig); > > - return token != NULL; This expression simply checks that token contains a value different from NULL. It's like a simple "a != 0" - it doesn't matter where that address points to, because the pointer is not dereferenced. > + bool found = token != NULL; > + free(tmp_ciphers); > + > + return found; For the reason above I think this change is useless (And even a bit ugly :p) NAK. In the future I suggest splitting unrelated changes like these in different patches. Assuming we had merged both, at some point of them might need to be reverted (for whatever reason and due to both fixes being in one patch, we would not be able to do that. Cheers, > } > > void >
Hi, > In the future I suggest splitting unrelated changes like these in > different patches. > Ok, will send v2 with first change only. > This expression simply checks that token contains a value different from > NULL. It's like a simple "a != 0" - it doesn't matter where that address > points to, because the pointer is not dereferenced. While it works, I think it is cleared to have a flag, rather than check the address of a dangling pointer.
Hi, On 17/02/2020 10:17, Lev Stipakov wrote: > Hi, > >> In the future I suggest splitting unrelated changes like these in >> different patches. >> > > Ok, will send v2 with first change only. > >> This expression simply checks that token contains a value different from >> NULL. It's like a simple "a != 0" - it doesn't matter where that address >> points to, because the pointer is not dereferenced. > > While it works, I think it is cleared to have a flag, rather than check the > address of a dangling pointer. Yeah, I agree with this statement, but the change you proposed did not make the code any cleaner IMHO. How about adding a boolean found variable (like you did) initialized to false and set it to true inside the if-block, before the break? That would allow the casual reader to understand what is the exact condition that triggers the function to return true. Cheers,
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index e708fc93..8cba3a76 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1918,7 +1918,6 @@ bool tls_item_in_cipher_list(const char *item, const char *list) { char *tmp_ciphers = string_alloc(list, NULL); - char *tmp_ciphers_orig = tmp_ciphers; const char *token = strtok(tmp_ciphers, ":"); while (token) @@ -1929,9 +1928,11 @@ tls_item_in_cipher_list(const char *item, const char *list) } token = strtok(NULL, ":"); } - free(tmp_ciphers_orig); - return token != NULL; + bool found = token != NULL; + free(tmp_ciphers); + + return found; } void