[Openvpn-devel] [PATCH] cleanup: Remove faulty env processing functions

[Openvpn-devel] [PATCH] cleanup: Remove faulty env processing functions

[David Sommerseth]

The env_set_add_to_environmenti() and env_set_remove_from_environment()
functions where not used in the code at all and they would cause an
ASSERT() in setenv_str_ex() later on, as it would not allow the
struct env_set *es pointer to be NULL (misc.c:807).

Signed-off-by: David Sommerseth <davids@...515...>
---
 src/openvpn/misc.c | 51 ---------------------------------------------------
 src/openvpn/misc.h |  4 ----
 2 files changed, 55 deletions(-)

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index a2f45b6..68d0687 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -701,57 +701,6 @@ env_set_inherit(struct env_set *es, const struct env_set *src)
     }
 }
 
-void
-env_set_add_to_environment(const struct env_set *es)
-{
-    if (es)
-    {
-        struct gc_arena gc = gc_new();
-        const struct env_item *e;
-
-        e = es->list;
-
-        while (e)
-        {
-            const char *name;
-            const char *value;
-
-            if (deconstruct_name_value(e->string, &name, &value, &gc))
-            {
-                setenv_str(NULL, name, value);
-            }
-
-            e = e->next;
-        }
-        gc_free(&gc);
-    }
-}
-
-void
-env_set_remove_from_environment(const struct env_set *es)
-{
-    if (es)
-    {
-        struct gc_arena gc = gc_new();
-        const struct env_item *e;
-
-        e = es->list;
-
-        while (e)
-        {
-            const char *name;
-            const char *value;
-
-            if (deconstruct_name_value(e->string, &name, &value, &gc))
-            {
-                setenv_del(NULL, name);
-            }
-
-            e = e->next;
-        }
-        gc_free(&gc);
-    }
-}
 
 /* add/modify/delete environmental strings */
 
diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
index 16be621..009767f 100644
--- a/src/openvpn/misc.h
+++ b/src/openvpn/misc.h
@@ -161,10 +161,6 @@ void env_set_print(int msglevel, const struct env_set *es);
 
 void env_set_inherit(struct env_set *es, const struct env_set *src);
 
-void env_set_add_to_environment(const struct env_set *es);
-
-void env_set_remove_from_environment(const struct env_set *es);
-
 /* Make arrays of strings */
 
 const char **make_env_array(const struct env_set *es,
-- 
2.11.0

Re: [Openvpn-devel] [PATCH] cleanup: Remove faulty env processing functions

[@ 2017-02-25 9:42 UTC (permalink / raw)]

[-- Attachment #1: Type: text/plain, Size: 3091 bytes --]

there are two another unused functions in misc.c

[src/openvpn/misc.c:661]: (style) The function 'env_set_print' is never
used.
[src/openvpn/misc.c:895]: (style) The function 'setenv_int_i' is never used.

2017-02-25 7:02 GMT+05:00 David Sommerseth <davids@...515...>:

> The env_set_add_to_environmenti() and env_set_remove_from_environment()
> functions where not used in the code at all and they would cause an
> ASSERT() in setenv_str_ex() later on, as it would not allow the
> struct env_set *es pointer to be NULL (misc.c:807).
>
> Signed-off-by: David Sommerseth <davids@...515...>
> ---
>  src/openvpn/misc.c | 51 ------------------------------
> ---------------------
>  src/openvpn/misc.h |  4 ----
>  2 files changed, 55 deletions(-)
>
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index a2f45b6..68d0687 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -701,57 +701,6 @@ env_set_inherit(struct env_set *es, const struct
> env_set *src)
>      }
>  }
>
> -void
> -env_set_add_to_environment(const struct env_set *es)
> -{
> -    if (es)
> -    {
> -        struct gc_arena gc = gc_new();
> -        const struct env_item *e;
> -
> -        e = es->list;
> -
> -        while (e)
> -        {
> -            const char *name;
> -            const char *value;
> -
> -            if (deconstruct_name_value(e->string, &name, &value, &gc))
> -            {
> -                setenv_str(NULL, name, value);
> -            }
> -
> -            e = e->next;
> -        }
> -        gc_free(&gc);
> -    }
> -}
> -
> -void
> -env_set_remove_from_environment(const struct env_set *es)
> -{
> -    if (es)
> -    {
> -        struct gc_arena gc = gc_new();
> -        const struct env_item *e;
> -
> -        e = es->list;
> -
> -        while (e)
> -        {
> -            const char *name;
> -            const char *value;
> -
> -            if (deconstruct_name_value(e->string, &name, &value, &gc))
> -            {
> -                setenv_del(NULL, name);
> -            }
> -
> -            e = e->next;
> -        }
> -        gc_free(&gc);
> -    }
> -}
>
>  /* add/modify/delete environmental strings */
>
> diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
> index 16be621..009767f 100644
> --- a/src/openvpn/misc.h
> +++ b/src/openvpn/misc.h
> @@ -161,10 +161,6 @@ void env_set_print(int msglevel, const struct env_set
> *es);
>
>  void env_set_inherit(struct env_set *es, const struct env_set *src);
>
> -void env_set_add_to_environment(const struct env_set *es);
> -
> -void env_set_remove_from_environment(const struct env_set *es);
> -
>  /* Make arrays of strings */
>
>  const char **make_env_array(const struct env_set *es,
> --
> 2.11.0
>
>
> ------------------------------------------------------------
> ------------------
> 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
>

[-- Attachment #2: Type: text/html, Size: 4337 bytes --]

Re: [Openvpn-devel] [PATCH] cleanup: Remove faulty env processing functions

[David Sommerseth]

[-- Attachment #1.1: Type: text/plain, Size: 5145 bytes --]

On 25/02/17 10:42, Илья Шипицин wrote:
> there are two another unused functions in misc.c
> 
> [src/openvpn/misc.c:661]: (style) The function 'env_set_print' is never
> used.

This one is useful for debugging purposes, so I let this one stay.  I
did consider an #if 0 around it, to make it more explicit.  But I
thought we have enough of those.  It could make some compiler warnings
go away though disabling it.

> [src/openvpn/misc.c:895]: (style) The function 'setenv_int_i' is never used.

Ahh, that one I didn't check for.  I'll have a closer look if that is
something we should let it survive or take that one out too.

The most troubling issue with those two functions in the patch is that
if they were to be used, it would actually make OpenVPN halt and exit
due to the ASSERT() check not being fulfilled later on in the call
chain.  setenv_str() calls setenv_str_ex() which checks if the pointer
to a struct set_env is NULL or not.  If it is NULL, it stops.  And these
two functions ends up calling setenv_str_ex() with
struct env_set *es = NULL;


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc


> 2017-02-25 7:02 GMT+05:00 David Sommerseth <davids@...515...
> <mailto:davids@...515...>>:
> 
>     The env_set_add_to_environmenti() and env_set_remove_from_environment()
>     functions where not used in the code at all and they would cause an
>     ASSERT() in setenv_str_ex() later on, as it would not allow the
>     struct env_set *es pointer to be NULL (misc.c:807).
> 
>     Signed-off-by: David Sommerseth <davids@...515...
>     <mailto:davids@...515...>>
>     ---
>      src/openvpn/misc.c | 51
>     ---------------------------------------------------
>      src/openvpn/misc.h |  4 ----
>      2 files changed, 55 deletions(-)
> 
>     diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
>     index a2f45b6..68d0687 100644
>     --- a/src/openvpn/misc.c
>     +++ b/src/openvpn/misc.c
>     @@ -701,57 +701,6 @@ env_set_inherit(struct env_set *es, const
>     struct env_set *src)
>          }
>      }
> 
>     -void
>     -env_set_add_to_environment(const struct env_set *es)
>     -{
>     -    if (es)
>     -    {
>     -        struct gc_arena gc = gc_new();
>     -        const struct env_item *e;
>     -
>     -        e = es->list;
>     -
>     -        while (e)
>     -        {
>     -            const char *name;
>     -            const char *value;
>     -
>     -            if (deconstruct_name_value(e->string, &name, &value, &gc))
>     -            {
>     -                setenv_str(NULL, name, value);
>     -            }
>     -
>     -            e = e->next;
>     -        }
>     -        gc_free(&gc);
>     -    }
>     -}
>     -
>     -void
>     -env_set_remove_from_environment(const struct env_set *es)
>     -{
>     -    if (es)
>     -    {
>     -        struct gc_arena gc = gc_new();
>     -        const struct env_item *e;
>     -
>     -        e = es->list;
>     -
>     -        while (e)
>     -        {
>     -            const char *name;
>     -            const char *value;
>     -
>     -            if (deconstruct_name_value(e->string, &name, &value, &gc))
>     -            {
>     -                setenv_del(NULL, name);
>     -            }
>     -
>     -            e = e->next;
>     -        }
>     -        gc_free(&gc);
>     -    }
>     -}
> 
>      /* add/modify/delete environmental strings */
> 
>     diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
>     index 16be621..009767f 100644
>     --- a/src/openvpn/misc.h
>     +++ b/src/openvpn/misc.h
>     @@ -161,10 +161,6 @@ void env_set_print(int msglevel, const struct
>     env_set *es);
> 
>      void env_set_inherit(struct env_set *es, const struct env_set *src);
> 
>     -void env_set_add_to_environment(const struct env_set *es);
>     -
>     -void env_set_remove_from_environment(const struct env_set *es);
>     -
>      /* Make arrays of strings */
> 
>      const char **make_env_array(const struct env_set *es,
>     --
>     2.11.0
> 
> 
>     ------------------------------------------------------------------------------
>     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
>     <mailto:Openvpn-devel@lists.sourceforge.net>
>     https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>     <https://lists.sourceforge.net/lists/listinfo/openvpn-devel>
> 
> 
> 
> 
> ------------------------------------------------------------------------------
> 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
> 





[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Openvpn-devel] [PATCH] cleanup: Remove faulty env processing functions

[@ 2017-02-25 13:45 UTC (permalink / raw)]

[-- Attachment #1: Type: text/plain, Size: 5680 bytes --]

2017-02-25 16:49 GMT+05:00 David Sommerseth <
openvpn@...2080...>:

> On 25/02/17 10:42, Илья Шипицин wrote:
> > there are two another unused functions in misc.c
> >
> > [src/openvpn/misc.c:661]: (style) The function 'env_set_print' is never
> > used.
>
> This one is useful for debugging purposes, so I let this one stay.  I
> did consider an #if 0 around it, to make it more explicit.  But I
> thought we have enough of those.  It could make some compiler warnings
> go away though disabling it.
>


it is good idea to put "#if 0" around debug items (that are not used
generally)

[src/openvpn/error.c:824]: (error) Null pointer dereference


>
> > [src/openvpn/misc.c:895]: (style) The function 'setenv_int_i' is never
> used.
>
> Ahh, that one I didn't check for.  I'll have a closer look if that is
> something we should let it survive or take that one out too.
>
> The most troubling issue with those two functions in the patch is that
> if they were to be used, it would actually make OpenVPN halt and exit
> due to the ASSERT() check not being fulfilled later on in the call
> chain.  setenv_str() calls setenv_str_ex() which checks if the pointer
> to a struct set_env is NULL or not.  If it is NULL, it stops.  And these
> two functions ends up calling setenv_str_ex() with
> struct env_set *es = NULL;
>
>
> --
> kind regards,
>
> David Sommerseth
> OpenVPN Technologies, Inc
>
>
> > 2017-02-25 7:02 GMT+05:00 David Sommerseth <davids@...515...
> > <mailto:davids@...515...>>:
> >
> >     The env_set_add_to_environmenti() and env_set_remove_from_
> environment()
> >     functions where not used in the code at all and they would cause an
> >     ASSERT() in setenv_str_ex() later on, as it would not allow the
> >     struct env_set *es pointer to be NULL (misc.c:807).
> >
> >     Signed-off-by: David Sommerseth <davids@...515...
> >     <mailto:davids@...515...>>
> >     ---
> >      src/openvpn/misc.c | 51
> >     ---------------------------------------------------
> >      src/openvpn/misc.h |  4 ----
> >      2 files changed, 55 deletions(-)
> >
> >     diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> >     index a2f45b6..68d0687 100644
> >     --- a/src/openvpn/misc.c
> >     +++ b/src/openvpn/misc.c
> >     @@ -701,57 +701,6 @@ env_set_inherit(struct env_set *es, const
> >     struct env_set *src)
> >          }
> >      }
> >
> >     -void
> >     -env_set_add_to_environment(const struct env_set *es)
> >     -{
> >     -    if (es)
> >     -    {
> >     -        struct gc_arena gc = gc_new();
> >     -        const struct env_item *e;
> >     -
> >     -        e = es->list;
> >     -
> >     -        while (e)
> >     -        {
> >     -            const char *name;
> >     -            const char *value;
> >     -
> >     -            if (deconstruct_name_value(e->string, &name, &value,
> &gc))
> >     -            {
> >     -                setenv_str(NULL, name, value);
> >     -            }
> >     -
> >     -            e = e->next;
> >     -        }
> >     -        gc_free(&gc);
> >     -    }
> >     -}
> >     -
> >     -void
> >     -env_set_remove_from_environment(const struct env_set *es)
> >     -{
> >     -    if (es)
> >     -    {
> >     -        struct gc_arena gc = gc_new();
> >     -        const struct env_item *e;
> >     -
> >     -        e = es->list;
> >     -
> >     -        while (e)
> >     -        {
> >     -            const char *name;
> >     -            const char *value;
> >     -
> >     -            if (deconstruct_name_value(e->string, &name, &value,
> &gc))
> >     -            {
> >     -                setenv_del(NULL, name);
> >     -            }
> >     -
> >     -            e = e->next;
> >     -        }
> >     -        gc_free(&gc);
> >     -    }
> >     -}
> >
> >      /* add/modify/delete environmental strings */
> >
> >     diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
> >     index 16be621..009767f 100644
> >     --- a/src/openvpn/misc.h
> >     +++ b/src/openvpn/misc.h
> >     @@ -161,10 +161,6 @@ void env_set_print(int msglevel, const struct
> >     env_set *es);
> >
> >      void env_set_inherit(struct env_set *es, const struct env_set *src);
> >
> >     -void env_set_add_to_environment(const struct env_set *es);
> >     -
> >     -void env_set_remove_from_environment(const struct env_set *es);
> >     -
> >      /* Make arrays of strings */
> >
> >      const char **make_env_array(const struct env_set *es,
> >     --
> >     2.11.0
> >
> >
> >     ------------------------------------------------------------
> ------------------
> >     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
> >     <mailto:Openvpn-devel@lists.sourceforge.net>
> >     https://lists.sourceforge.net/lists/listinfo/openvpn-devel
> >     <https://lists.sourceforge.net/lists/listinfo/openvpn-devel>
> >
> >
> >
> >
> > ------------------------------------------------------------
> ------------------
> > 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
> >
>
>
>
>
>

[-- Attachment #2: Type: text/html, Size: 8338 bytes --]

Re: [Openvpn-devel] [PATCH] cleanup: Remove faulty env processing functions

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

On Sat, Feb 25, 2017 at 06:45:36PM +0500, Илья Шипицин wrote:
> 2017-02-25 16:49 GMT+05:00 David Sommerseth <
> openvpn@...2080...>:
> 
> > On 25/02/17 10:42, Илья Шипицин wrote:
> > > there are two another unused functions in misc.c
> > >
> > > [src/openvpn/misc.c:661]: (style) The function 'env_set_print' is never
> > > used.
> >
> > This one is useful for debugging purposes, so I let this one stay.  I
> > did consider an #if 0 around it, to make it more explicit.  But I
> > thought we have enough of those.  It could make some compiler warnings
> > go away though disabling it.
> >
> 
> 
> it is good idea to put "#if 0" around debug items (that are not used
> generally)

Not sure if there is something similar in the code right now, but wouldn't be
better to have this kind of code surrounded by some "#ifdef DEBUG" that can be
enabled upon configuration ?

Cheers,


-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Openvpn-devel] [PATCH] cleanup: Remove faulty env processing functions

[@ 2017-02-25 15:28 UTC (permalink / raw)]

[-- Attachment #1: Type: text/plain, Size: 1442 bytes --]

2017-02-25 19:36 GMT+05:00 Antonio Quartulli <a@...2181...>:

> On Sat, Feb 25, 2017 at 06:45:36PM +0500, Илья Шипицин wrote:
> > 2017-02-25 16:49 GMT+05:00 David Sommerseth <
> > openvpn@...2080...>:
> >
> > > On 25/02/17 10:42, Илья Шипицин wrote:
> > > > there are two another unused functions in misc.c
> > > >
> > > > [src/openvpn/misc.c:661]: (style) The function 'env_set_print' is
> never
> > > > used.
> > >
> > > This one is useful for debugging purposes, so I let this one stay.  I
> > > did consider an #if 0 around it, to make it more explicit.  But I
> > > thought we have enough of those.  It could make some compiler warnings
> > > go away though disabling it.
> > >
> >
> >
> > it is good idea to put "#if 0" around debug items (that are not used
> > generally)
>
> Not sure if there is something similar in the code right now, but wouldn't
> be
> better to have this kind of code surrounded by some "#ifdef DEBUG" that
> can be
> enabled upon configuration ?
>

this might be surrounded with "#ifdef ENABLE_DEBUG":

[src/openvpn/otime.c:104]: (style) The function 'tv_string_abs' is never
used.

(and several more "never used" functions as well)

but for "[src/openvpn/error.c:824]: (error) Null pointer dereference", I'm
afraid we should guard it with "#if 0", otherwise static analyzer will
continue complain


> Cheers,
>
>
> --
> Antonio Quartulli
>

[-- Attachment #2: Type: text/html, Size: 2359 bytes --]

Re: [Openvpn-devel] [PATCH] cleanup: Remove faulty env processing functions

[David Sommerseth]

[-- Attachment #1.1: Type: text/plain, Size: 855 bytes --]

On 25/02/17 16:28, Илья Шипицин wrote:
>
> but for "[src/openvpn/error.c:824]: (error) Null pointer dereference",
> I'm afraid we should guard it with "#if 0", otherwise static analyzer
> will continue complain

No.  That function in error.c:824, crash(), which is only enabled with
ENABLE_DEBUG is explicitly written that way to crash OpenVPN when
called.  So that is purely a false positive, as the static analyzer
doesn't expect such nasty code to be there with a purpose.  But
triggering a crash like that can have some use case in some special
situations.

Normal builds should generally not be built with ENABLE_DEBUG, that is a
intended as a "developer mode".

We should generally avoid '#if 0' or even '#if 1' as much as possible in
the code.


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[Openvpn-devel] [PATCH applied] Re: cleanup: Remove faulty env processing functions

[Gert Doering]

ACK.

While I'm always cautious about "cleanups", this one is really 
straightforward :-) - unreferenced, not even from #if 0 testing code,
gone with the wind...

Your patch has been applied to the master and release/2.4 branch.

commit a87f119afcfcc1c855a6ea2ba3d765966f1f2591
commit 860d79bc4eadd934390829bee0de37e5ded8907a
Author: David Sommerseth
Date:   Sat Feb 25 03:02:29 2017 +0100

     cleanup: Remove faulty env processing functions

     Signed-off-by: David Sommerseth <davids@...515...>
     Acked-by: Gert Doering <gert@...1296...>
     Message-Id: <20170225020229.17287-1-davids@...515...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14195.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering