[Openswan dev] [PATCH] silent wrong uninitialized compiler warning

[Gilles Espinasse]

----- Original Message ----- 
From: "D. Hugh Redelmeier" <hugh at mimosa.com>
To: "Gilles Espinasse" <g.esp at free.fr>; <dev at openswan.org>
Sent: Wednesday, June 24, 2009 8:27 AM
Subject: Re: [Openswan dev] [PATCH] silent wrong uninitialized compiler
warning


> | From: Gilles Espinasse <g.esp at free.fr>
>
> | (gcc-4.2.2)
> | /usr/src/openswan-2.6.22rc2/programs/pluto/rnd.c:175: warning: 'rnd_dev'
may be used uninitialized in this function
> | /usr/src/openswan-2.6.22rc2/programs/pluto/nat_traversal.c:673: warning:
'r' may be used uninitialized in this function
>
> I'm glad you are de-linting the code.
>
> It is true that your changes will shut up the GCC warnings.
>
> It is good to
> 1) make sure that the warnings are wrong.
> 2) add a comment to the initializer saying why you are initializing:
>    /* initializing to shut up GCC warning about uninitialized use */
>
> |  int nat_traversal_espinudp_socket (int sk, const char *fam, u_int32_t
type)
> |  {
> | - int r;
> | + int r = 0;
> |  static enum { auto_style, new_style, old_style } style = auto_style;
> |
> |  if (style == auto_style || style == new_style) {
>
> I took a quick look at the code in 2.6.22.  I don't understand its
> complexity.  It seems as if the test in the first if statement has to
> always be true since style is initialized to auto_style.  So lots of
> the code is redundant an r is surely set before it is used.
>
style is declared static.
So first time style==auto_style but on second call style value will be
new_style or old_style, depending of the result of ioctl(sk, ...) or
setsockopt(sk,...)

so there is no dead code there.

> I would take the GCC warning as a suggestion to eliminate dead code in
> that routine.  At that point, GCC should be able to see that r is
> never used uninitialized.
>
> Perhaps the dead code is their for "future expansion".  I don't know.
>
> It looks as if -1 is a better value to initialize r to, if you have to
> do so.
>
I don't understand the reason.

> | diff --git a/programs/pluto/rnd.c b/programs/pluto/rnd.c
> | index dab0a02..a713cd9 100644
> | --- a/programs/pluto/rnd.c
> | +++ b/programs/pluto/rnd.c
> | @@ -172,7 +172,7 @@ init_rnd_pool(void)
> |  #ifndef HAVE_LIBNSS
> |      unsigned int i;
> |      unsigned int max_rnd_devices = elemsof(random_devices)+1;
> | -    const char *rnd_dev;
> | +    const char *rnd_dev = NULL;
> |
> |      if(random_fd != -1) close(random_fd);
> |      random_fd = -1;
>
> The code you are fixing here is overly complex too.  While trying to
> understand it, I found a bug: off-by-one in the for loop.  The
> definition of max_rnd_devices should not have the "+1".
>
I have not written that code!
That's true the +1 is wrong there.
In the case all devices fail to open, openswan_log will access
random_devices[max_rnd_devices].

    Gilles