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

[D. Hugh Redelmeier]

| 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.

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.

| 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".

Just for fun, I've rewritten the code to not require the initializer.
I have NOT TESTED it.

Here's a diff based on 2.6.22.
--- rnd.c	2009-06-22 22:53:08.000000000 -0400
+++ rnd.c.better	2009-06-24 02:20:44.000000000 -0400
@@ -171,41 +171,40 @@
 {
 #ifndef HAVE_LIBNSS
     unsigned int i;
-    unsigned int max_rnd_devices = elemsof(random_devices)+1;
-    const char *rnd_dev = NULL;
 
     if(random_fd != -1) close(random_fd);
     random_fd = -1;
 
-    for(i=0; random_fd == -1 && i<max_rnd_devices; i++) {
-	DBG(DBG_CONTROL, DBG_log("opening %s", random_devices[i]));
-	random_fd = open(random_devices[i], O_RDONLY);
-	rnd_dev = random_devices[i];
+    for(i=0; i<elemsof(random_devices); i++) {
+	const char *rnd_dev = random_devices[i];
+
+	DBG(DBG_CONTROL, DBG_log("opening %s", rnd_dev));
+	random_fd = open(rnd_dev, O_RDONLY);
 
 	if (random_fd == -1) {
-	    openswan_log("WARNING: open of %s failed: %s", random_devices[i]
+	    openswan_log("WARNING: open of %s failed: %s", rnd_dev
 			 , strerror(errno));
-	}
-    }
+	} else {
 
-    if(random_fd == -1 || i == max_rnd_devices) {
-	openswan_log("Failed to open any source of random. Unable to start any connections.");
-	return;
-    }
+	    openswan_log("using %s as source of random entropy", rnd_dev);
 
-    openswan_log("using %s as source of random entropy", rnd_dev);
+	    fcntl(random_fd, F_SETFD, FD_CLOEXEC);
 
-    fcntl(random_fd, F_SETFD, FD_CLOEXEC);
+	    get_rnd_bytes(random_pool, RANDOM_POOL_SIZE);
+	    mix_pool();
 
-    get_rnd_bytes(random_pool, RANDOM_POOL_SIZE);
-    mix_pool();
+	    /* start of rand(3) on the right foot */
+	    {
+		unsigned int seed;
 
-    /* start of rand(3) on the right foot */
-    {
-	unsigned int seed;
+		get_rnd_bytes((void *)&seed, sizeof(seed));
+		srand(seed);
+	    }
+	    return;
+	}
+    }
 
-	get_rnd_bytes((void *)&seed, sizeof(seed));
-	srand(seed);
+    openswan_log("Failed to open any source of random. Unable to start any connections.");
     }
 #endif
 }
================ end ================