diff mbox series

[Openvpn-devel] ssl.c: refactor tls_item_in_cipher_list() code

Message ID 20200217082942.173-1-lstipakov@gmail.com
State Superseded
Headers
Series [Openvpn-devel] ssl.c: refactor tls_item_in_cipher_list() code |

Commit Message

Lev Stipakov Feb. 16, 2020, 9:29 p.m. UTC 
  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.

 - "token" points to content inside tokenized string. It becomes
dangling pointer after freeing that string. Check its value before
free() call.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpn/ssl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Antonio Quartulli Feb. 16, 2020, 9:44 p.m. UTC | #1 
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
>
Lev Stipakov Feb. 16, 2020, 10:17 p.m. UTC | #2 
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.
Antonio Quartulli Feb. 16, 2020, 10:18 p.m. UTC | #3 
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 mbox series

Patch

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