[PATCH 4/11] drivers/net/hamradio: Eliminate a NULL pointer dereference

[PATCH 4/11] drivers/net/hamradio: Eliminate a NULL pointer dereference

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

At the point of the print, dev is NULL.

A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@r exists@
expression E,E1;
identifier f;
statement S1,S2,S3;
@@

if ((E == NULL && ...) || ...)
{
  ... when != if (...) S1 else S2
      when != E = E1
* E->f
  ... when any
  return ...;
}
else S3
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/net/hamradio/yam.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/hamradio/yam.c b/drivers/net/hamradio/yam.c
index 694132e..4e7d1d0 100644
--- a/drivers/net/hamradio/yam.c
+++ b/drivers/net/hamradio/yam.c
@@ -1151,8 +1151,7 @@ static int __init yam_init_driver(void)
 		dev = alloc_netdev(sizeof(struct yam_port), name,
 				   yam_setup);
 		if (!dev) {
-			printk(KERN_ERR "yam: cannot allocate net device %s\n",
-			       dev->name);
+			pr_err("yam: cannot allocate net device\n");
 			err = -ENOMEM;
 			goto error;
 		}

Re: [PATCH 4/11] drivers/net/hamradio: Eliminate a NULL pointer dereference

[David Miller]

From: Julia Lawall <julia@diku.dk>
Date: Thu, 27 May 2010 14:33:00 +0200 (CEST)

> At the point of the print, dev is NULL.
> 
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
 ...
> Signed-off-by: Julia Lawall <julia@diku.dk>

Applied.

xfbbd oddness.

[Cathryn Mataga]

well, I'm running xfbbd, and it's forwarding via internet
no problem.  But I have an actual RF connection over AX25 that
I'm trying to make work and I'm getting a fault here.


I haven't really spent any time on this, but I'm just posting
my backtrace here in case anyone else has been this way before.


This is the version R11 off the website with no changes by me.
The older version did the same thing, so this isn't a new issue.

uname -v shows

2.6.32.21-i68.fc12.i686.PAE

This is an SMP kernel, maybe this is still a problem?


It's dying in sig_fct, which doesn't actually do anything with
SIGPIPE.   "SIGPIPE is the signal sent to a process when
it attempts to write to a pipe without a process connected
to the other end."

Could this be an attempt by FBB to write a packet, after
the connection has been closed?   (This is just me thinking
out loud here.)


Time out 200 mn
Timeout voie 73 depasse
Program received signal SIGPIPE, Broken pipe.
0x0016c416 in __kernel_vsyscall ()



(gdb) down
#0  0x0804da80 in sig_fct ()
(gdb) backtrace
#0  0x0804da80 in sig_fct ()
#1  <signal handler called>
#2  0x0016c416 in __kernel_vsyscall ()
#3  0x00a85f73 in __write_nocancel () from /lib/libc.so.6
#4  0x0806cec4 in snd_sck ()
#5  0x0805c18c in snd_drv ()
#6  0x080bc909 in send_buf ()
#7  0x0808eff8 in traite_voie ()
#8  0x080908e8 in user_time_out ()
#9  0x08091899 in kernel ()
#10 0x0804dcf2 in process ()
#11 0x080507d2 in fbb_orb ()
#12 0x0804e05e in main ()
(gdb)

Re: xfbbd oddness.

[Cathryn Mataga]

You know, I'm not an expert on signal programming in Unix.
But, wouldn't another signal coming in during the wait cause
a the system to halt?



         for (i = 1; i < NSIG; i++)
         {
                 if (i == SIGBUS || i == SIGSEGV || i == SIGALRM)
                         continue;
                 signal (i, sig_fct);
         }


static void sig_fct (int sig)
{
         int pid, pstatus;

         sig &= 0xff;
         pid = wait (&pstatus);
         signal (sig, sig_fct);

	switch (sig)
	{
...
stuff
	}
}





This handler does nothing useful anyway except for
SIGHUP, SIGTERM, SIGBUS and SIGSEGV.  I don't see any
reason not to just ignore all signals except for the few
that do have a function here.  I'm going to let it run
like this, and see if it fixes the bug.


         for (i = 1; i < NSIG; i++)
         {
                 if (i == SIGBUS || i == SIGSEGV || i == SIGALRM)
                         continue;
                 if (i == SIGHUP || i == SIGTERM)
                         signal (i, sig_fct);
                 else    signal(i, SIG_IGN);
         }

Re: xfbbd oddness.

[Cathryn Mataga]

Actually, I think ignoring the SIGPIPE and other signals
does fix a real problem.   I changed this, and I ran
about a day with no SIGPIPE faults -- that is with
frequent connects from an actual RF-based AX25 connection.
No functionality is lost by ignore these signals, so I think
I will send in a patch for this at some point.

With this one patch, I think xfbbd is basically usable for
me on RF connections.


There still is a bug.  After awhile, I start seeing...

select: Bad file descriptor
select: Bad file descriptor
select: Bad file descriptor
select: Bad file descriptor

and then the system bugs out.  The script normally restarts
it, though I'm running from gdb myself so for me it just dies.



Via google, I see someone reported this bug back in 2004.  That
was 6 years ago, so it is obviously a very old bug.
At least it's consistent, I suppose.

Re: xfbbd oddness.

[Cathryn Mataga]

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

Patch versus version r11



1. SOCK_MAXCHAN (drv_sock.c) now set to (MAXVOIES)  This was set to 50, which was less than MAXVOIES, and the code was
accessing data off the end of this table.  This was causing strange problems.  After I made this change the gateway
function started working again.  If anyone was having trouble with xfbbd and RF radio connections, this change may
possibly fix their problem.
2.  Several minor changes designed to eliminate "Bad file descriptor" error messages.  These occurred at the time
of housekeeping due to a minor bug.  The messages actually turned out to be harmless, as far as I can tell.
3.  Only use the signal handler for signals that have functionality in the code.  This is a simplification
to prevent bugs.  Other signals are set to SIG_IGN.




[-- Attachment #2: diff.patch --]
[-- Type: text/plain, Size: 2189 bytes --]

diff -ruN fbbsrc.704r11/src/drv_sock.c fbbsrc.704r11cathryn/src/drv_sock.c
--- fbbsrc.704r11/src/drv_sock.c	2010-06-22 13:03:01.000000000 -0700
+++ fbbsrc.704r11cathryn/src/drv_sock.c	2010-11-13 22:43:56.284120401 -0800
@@ -74,7 +74,8 @@
 #define RETR_EVENT 3
 #define BUSY_EVENT 4
 
-#define SOCK_MAXCAN 50
+#define SOCK_MAXCAN  (MAXVOIES) /* was 50 -- this was causing bad problems. 
+			This table was being accessed beyond the array */
 
 #define	CAN_AX25	0
 #define	CAN_NETROM	1
@@ -104,7 +105,7 @@
 }
 scan_t;
 
-scan_t scan[SOCK_MAXCAN];
+scan_t scan[SOCK_MAXCAN] = {{0}};  /* I think this is necessary (Though not 100% sure.  */
 
 static int last_can = 1;
 static int msocket = -1;
@@ -692,8 +693,11 @@
 
 static int stop_cnx (int port)
 {
-	close(p_port[port].fd);
-	p_port[port].fd = 0;
+	if (p_port[port].fd) /* Prevent closing of 0 */
+	{
+		close(p_port[port].fd);
+		p_port[port].fd = 0;
+	}
 	return 1;
 }
 
diff -ruN fbbsrc.704r11/src/drv_tcp.c fbbsrc.704r11cathryn/src/drv_tcp.c
--- fbbsrc.704r11/src/drv_tcp.c	2009-11-27 08:50:44.000000000 -0800
+++ fbbsrc.704r11cathryn/src/drv_tcp.c	2010-11-13 16:55:53.363119445 -0800
@@ -950,8 +950,11 @@
 
 static int stop_cnx (int port)
 {
-	close(p_port[port].fd);
-	p_port[port].fd = 0;
+	if (p_port[port].fd) /* Prevent closing of 0 */
+	{
+		close(p_port[port].fd);
+		p_port[port].fd = 0;
+	}
 	return 1;
 }
 
@@ -1439,7 +1442,7 @@
 	fd_set tcp_excep;
 	struct timeval to;
 
-	if (can->sock == -1)
+	if (can->sock <= 0) /* Was -1.  Sock=0 during housekeeping.  Cause of select errors */
 		return (0);
 
 	if ((can->timeout) && (can->timeout < time (NULL)))
diff -ruN fbbsrc.704r11/src/xfbbd.c fbbsrc.704r11cathryn/src/xfbbd.c
--- fbbsrc.704r11/src/xfbbd.c	2009-11-27 08:50:44.000000000 -0800
+++ fbbsrc.704r11cathryn/src/xfbbd.c	2010-11-13 17:10:40.879123108 -0800
@@ -195,7 +195,10 @@
 	{
 		if (i == SIGBUS || i == SIGSEGV || i == SIGALRM)
 			continue;
-		signal (i, sig_fct);
+		else if (i == SIGHUP  || i == SIGTERM || i == SIGBUS || i == SIGSEGV)
+			signal (i, sig_fct); /* Use sig_fct only for signals that do something */
+		else 	
+			signal (i, SIG_IGN); /* Otherwise ignore */
 	}
 
 	for (ng = 1; ng < ac; ng++)

[PATCH] xfbbd: Fix issue with xfbbd and AX25 radio connections

[Cathryn Mataga]

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

Patch versus version xfbbd version r11

Same patch file, Except I changed the email subject.



1. SOCK_MAXCHAN (drv_sock.c) now set to (MAXVOIES)  This was set to 50, which was less than MAXVOIES, and the code was
accessing data off the end of this table.  This was causing strange problems.  After I made this change the gateway
function started working again.  If anyone was having trouble with xfbbd and RF radio connections, this change may
possibly fix their problem.
2.  Several minor changes designed to eliminate "Bad file descriptor" error messages.  These occurred at the time
of housekeeping due to a minor bug.  The messages actually turned out to be harmless, as far as I can tell.
3.  Only use the signal handler for signals that have functionality in the code.  This is a simplification
to prevent bugs.  Other signals are set to SIG_IGN.



[-- Attachment #2: diff.patch --]
[-- Type: text/plain, Size: 2189 bytes --]

diff -ruN fbbsrc.704r11/src/drv_sock.c fbbsrc.704r11cathryn/src/drv_sock.c
--- fbbsrc.704r11/src/drv_sock.c	2010-06-22 13:03:01.000000000 -0700
+++ fbbsrc.704r11cathryn/src/drv_sock.c	2010-11-13 22:43:56.284120401 -0800
@@ -74,7 +74,8 @@
 #define RETR_EVENT 3
 #define BUSY_EVENT 4
 
-#define SOCK_MAXCAN 50
+#define SOCK_MAXCAN  (MAXVOIES) /* was 50 -- this was causing bad problems. 
+			This table was being accessed beyond the array */
 
 #define	CAN_AX25	0
 #define	CAN_NETROM	1
@@ -104,7 +105,7 @@
 }
 scan_t;
 
-scan_t scan[SOCK_MAXCAN];
+scan_t scan[SOCK_MAXCAN] = {{0}};  /* I think this is necessary (Though not 100% sure.  */
 
 static int last_can = 1;
 static int msocket = -1;
@@ -692,8 +693,11 @@
 
 static int stop_cnx (int port)
 {
-	close(p_port[port].fd);
-	p_port[port].fd = 0;
+	if (p_port[port].fd) /* Prevent closing of 0 */
+	{
+		close(p_port[port].fd);
+		p_port[port].fd = 0;
+	}
 	return 1;
 }
 
diff -ruN fbbsrc.704r11/src/drv_tcp.c fbbsrc.704r11cathryn/src/drv_tcp.c
--- fbbsrc.704r11/src/drv_tcp.c	2009-11-27 08:50:44.000000000 -0800
+++ fbbsrc.704r11cathryn/src/drv_tcp.c	2010-11-13 16:55:53.363119445 -0800
@@ -950,8 +950,11 @@
 
 static int stop_cnx (int port)
 {
-	close(p_port[port].fd);
-	p_port[port].fd = 0;
+	if (p_port[port].fd) /* Prevent closing of 0 */
+	{
+		close(p_port[port].fd);
+		p_port[port].fd = 0;
+	}
 	return 1;
 }
 
@@ -1439,7 +1442,7 @@
 	fd_set tcp_excep;
 	struct timeval to;
 
-	if (can->sock == -1)
+	if (can->sock <= 0) /* Was -1.  Sock=0 during housekeeping.  Cause of select errors */
 		return (0);
 
 	if ((can->timeout) && (can->timeout < time (NULL)))
diff -ruN fbbsrc.704r11/src/xfbbd.c fbbsrc.704r11cathryn/src/xfbbd.c
--- fbbsrc.704r11/src/xfbbd.c	2009-11-27 08:50:44.000000000 -0800
+++ fbbsrc.704r11cathryn/src/xfbbd.c	2010-11-13 17:10:40.879123108 -0800
@@ -195,7 +195,10 @@
 	{
 		if (i == SIGBUS || i == SIGSEGV || i == SIGALRM)
 			continue;
-		signal (i, sig_fct);
+		else if (i == SIGHUP  || i == SIGTERM || i == SIGBUS || i == SIGSEGV)
+			signal (i, sig_fct); /* Use sig_fct only for signals that do something */
+		else 	
+			signal (i, SIG_IGN); /* Otherwise ignore */
 	}
 
 	for (ng = 1; ng < ac; ng++)