[ros-diffs] [cgutman] 43095: - Partial rewrite of dispatch() - Header cleanup - Fix several bugs related to null pointer access and out-of-bounds array access - Found by Amine Khaldi - This should fix the issue with dhcp crashing and hogging tons of cpu time

Author: cgutman
Date: Sun Sep 20 17:00:08 2009
New Revision: 43095

URL: http://svn.reactos.org/svn/reactos?rev=43095&view=rev
Log:
 - Partial rewrite of dispatch()
 - Header cleanup
 - Fix several bugs related to null pointer access and out-of-bounds array access
 - Found by Amine Khaldi
 - This should fix the issue with dhcp crashing and hogging tons of cpu time

Modified:
    trunk/reactos/base/services/dhcp/adapter.c
    trunk/reactos/base/services/dhcp/dhclient.c
    trunk/reactos/base/services/dhcp/dispatch.c
    trunk/reactos/base/services/dhcp/include/dhcpd.h
    trunk/reactos/base/services/dhcp/include/rosdhcp.h
    trunk/reactos/base/services/dhcp/options.c

Modified: trunk/reactos/base/services/dhcp/adapter.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/base/services/dhcp/adapter…
==============================================================================
--- trunk/reactos/base/services/dhcp/adapter.c [iso-8859-1] (original)
+++ trunk/reactos/base/services/dhcp/adapter.c [iso-8859-1] Sun Sep 20 17:00:08 2009
@@ -169,10 +169,9 @@
         Adapter->DhclientState.state = S_STATIC;
 
         Netmask = RegReadString( AdapterKey, NULL, "Subnetmask" );
-        if( !Netmask ) Netmask = "255.255.255.0";
 
         Status = AddIPAddress( inet_addr( IPAddress ),
-                               inet_addr( Netmask ),
+                               inet_addr( Netmask ? Netmask : "255.255.255.0"
),
                                Adapter->IfMib.dwIndex,
                                &Adapter->NteContext,
                                &Adapter->NteInstance );
@@ -257,7 +256,7 @@
         DH_DbgPrint(MID_TRACE,("Getting adapter %d attributes\n",
                                Table->table[i].dwIndex));
 
-        if (AdapterFindByHardwareAddress(Table->table[i].bPhysAddr,
Table->table[i].dwPhysAddrLen))
+        if ((Adapter = AdapterFindByHardwareAddress(Table->table[i].bPhysAddr,
Table->table[i].dwPhysAddrLen)))
         {
             /* This is an existing adapter */
             if (InterfaceConnected(Table->table[i])) {

Modified: trunk/reactos/base/services/dhcp/dhclient.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/base/services/dhcp/dhclien…
==============================================================================
--- trunk/reactos/base/services/dhcp/dhclient.c [iso-8859-1] (original)
+++ trunk/reactos/base/services/dhcp/dhclient.c [iso-8859-1] Sun Sep 20 17:00:08 2009
@@ -1576,8 +1576,10 @@
 
 	if (!leaseFile) {	/* XXX */
 		leaseFile = fopen(path_dhclient_db, "w");
-		if (!leaseFile)
+		if (!leaseFile) {
 			error("can't create %s", path_dhclient_db);
+                        return;
+                }
 	}
 
 	fprintf(leaseFile, "lease {\n");
@@ -1600,17 +1602,20 @@
 			    lease->options[i].len, 1, 1));
 
 	t = gmtime(&lease->renewal);
-	fprintf(leaseFile, "  renew %d %d/%d/%d %02d:%02d:%02d;\n",
-	    t->tm_wday, t->tm_year + 1900, t->tm_mon + 1, t->tm_mday,
-	    t->tm_hour, t->tm_min, t->tm_sec);
+        if (t)
+	    fprintf(leaseFile, "  renew %d %d/%d/%d %02d:%02d:%02d;\n",
+	        t->tm_wday, t->tm_year + 1900, t->tm_mon + 1, t->tm_mday,
+	        t->tm_hour, t->tm_min, t->tm_sec);
 	t = gmtime(&lease->rebind);
-	fprintf(leaseFile, "  rebind %d %d/%d/%d %02d:%02d:%02d;\n",
-	    t->tm_wday, t->tm_year + 1900, t->tm_mon + 1, t->tm_mday,
-	    t->tm_hour, t->tm_min, t->tm_sec);
+        if (t)
+	    fprintf(leaseFile, "  rebind %d %d/%d/%d %02d:%02d:%02d;\n",
+	         t->tm_wday, t->tm_year + 1900, t->tm_mon + 1, t->tm_mday,
+	         t->tm_hour, t->tm_min, t->tm_sec);
 	t = gmtime(&lease->expiry);
-	fprintf(leaseFile, "  expire %d %d/%d/%d %02d:%02d:%02d;\n",
-	    t->tm_wday, t->tm_year + 1900, t->tm_mon + 1, t->tm_mday,
-	    t->tm_hour, t->tm_min, t->tm_sec);
+        if (t)
+	    fprintf(leaseFile, "  expire %d %d/%d/%d %02d:%02d:%02d;\n",
+	        t->tm_wday, t->tm_year + 1900, t->tm_mon + 1, t->tm_mday,
+	        t->tm_hour, t->tm_min, t->tm_sec);
 	fprintf(leaseFile, "}\n");
 	fflush(leaseFile);
 }
@@ -1765,7 +1770,7 @@
 						config->defaults[i].data,
 						ip->client->
 						config->defaults[i].len);
-					dp[len] = '\0';
+					dp[len-1] = '\0';
 				}
 			} else {
 				dp = ip->client->

Modified: trunk/reactos/base/services/dhcp/dispatch.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/base/services/dhcp/dispatc…
==============================================================================
--- trunk/reactos/base/services/dhcp/dispatch.c [iso-8859-1] (original)
+++ trunk/reactos/base/services/dhcp/dispatch.c [iso-8859-1] Sun Sep 20 17:00:08 2009
@@ -63,11 +63,16 @@
 void
 dispatch(void)
 {
-    int count, i, to_msec, nfds, err;
+    int count, to_msec, err;
     struct protocol *l;
     fd_set fds;
     time_t howlong, cur_time;
     struct timeval timeval;
+
+    if (!AdapterDiscover()) {
+         AdapterStop();
+         return;
+    }
 
     ApiLock();
 
@@ -76,17 +81,6 @@
          * Call any expired timeouts, and then if there's still
          * a timeout registered, time out the select call then.
          */
-    another:
-        if (!AdapterDiscover()) {
-            AdapterStop();
-            break;
-        }
-
-        for (l = protocols, nfds = 0; l; l = l->next)
-            nfds++;
-
-        FD_ZERO(&fds);
-
         time(&cur_time);
 
         if (timeouts) {
@@ -98,7 +92,7 @@
                 (*(t->func))(t->what);
                 t->next = free_timeouts;
                 free_timeouts = t;
-                goto another;
+                continue;
             }
 
             /*
@@ -112,49 +106,30 @@
                 howlong = INT_MAX / 1000;
             to_msec = howlong * 1000;
         } else
-            to_msec = -1;
+            to_msec = 5000;
 
         /* Set up the descriptors to be polled. */
-        for (i = 0, l = protocols; l; l = l->next) {
-            struct interface_info *ip = l->local;
-
-            if (ip && (l->handler != got_one || !ip->dead)) {
-                FD_SET(l->fd, &fds);
-                i++;
-            }
-        }
-
-        if (i == 0) {
-            /* Wait for 5 seconds before looking for more interfaces */
-            Sleep(5000);
-            continue;
-        } else {
-            /* Wait for a packet or a timeout... XXX */
-            timeval.tv_sec = to_msec / 1000;
-            timeval.tv_usec = (to_msec % 1000) * 1000;
-        }
+        FD_ZERO(&fds);
+
+        for (l = protocols; l; l = l->next)
+             FD_SET(l->fd, &fds);
+
+        /* Wait for a packet or a timeout... XXX */
+        timeval.tv_sec = to_msec / 1000;
+        timeval.tv_usec = to_msec % 1000;
 
         ApiUnlock();
 
-        count = select(nfds, &fds, NULL, NULL, &timeval);
+        if (protocols)
+            count = select(0, &fds, NULL, NULL, &timeval);
+        else {
+            Sleep(to_msec);
+            count = 0;
+        }
+
+        ApiLock();
 
         DH_DbgPrint(MID_TRACE,("Select: %d\n", count));
-
-        /* Review poll output */
-        for (i = 0, l = protocols; l; l = l->next) {
-            struct interface_info *ip = l->local;
-
-            if (ip && (l->handler != got_one || !ip->dead)) {
-                DH_DbgPrint
-                    (MID_TRACE,
-                     ("set(%d) -> %s\n",
-                      l->fd, FD_ISSET(l->fd, &fds) ? "true" :
"false"));
-                i++;
-            }
-        }
-
-
-        ApiLock();
 
         /* Not likely to be transitory... */
         if (count == SOCKET_ERROR) {
@@ -163,7 +138,6 @@
             break;
         }
 
-        i = 0;
         for (l = protocols; l; l = l->next) {
             struct interface_info *ip;
             ip = l->local;
@@ -173,7 +147,6 @@
                     DH_DbgPrint(MID_TRACE,("Handling %x\n", l));
                     (*(l->handler))(l);
                 }
-                i++;
             }
         }
     } while (1);
@@ -269,8 +242,10 @@
             q->what = what;
         } else {
             q = malloc(sizeof(struct timeout));
-            if (!q)
+            if (!q) {
                 error("Can't allocate timeout structure!");
+                return;
+            }
             q->func = where;
             q->what = what;
         }

Modified: trunk/reactos/base/services/dhcp/include/dhcpd.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/base/services/dhcp/include…
==============================================================================
--- trunk/reactos/base/services/dhcp/include/dhcpd.h [iso-8859-1] (original)
+++ trunk/reactos/base/services/dhcp/include/dhcpd.h [iso-8859-1] Sun Sep 20 17:00:08
2009
@@ -101,7 +101,6 @@
 #include <fcntl.h>
 #include <limits.h>
 //#include <unistd.h>
-#include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>

Modified: trunk/reactos/base/services/dhcp/include/rosdhcp.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/base/services/dhcp/include…
==============================================================================
--- trunk/reactos/base/services/dhcp/include/rosdhcp.h [iso-8859-1] (original)
+++ trunk/reactos/base/services/dhcp/include/rosdhcp.h [iso-8859-1] Sun Sep 20 17:00:08
2009
@@ -10,7 +10,6 @@
 #include <dhcpcsdk.h>
 #include <stdio.h>
 #include <io.h>
-#include <setjmp.h>
 #include "stdint.h"
 #include "predec.h"
 #include <dhcp/rosdhcp_public.h>

Modified: trunk/reactos/base/services/dhcp/options.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/base/services/dhcp/options…
==============================================================================
--- trunk/reactos/base/services/dhcp/options.c [iso-8859-1] (original)
+++ trunk/reactos/base/services/dhcp/options.c [iso-8859-1] Sun Sep 20 17:00:08 2009
@@ -175,9 +175,11 @@
 			 * for clients, but what the heck...
 			 */
 			t = calloc(1, len + packet->options[code].len + 1);
-			if (!t)
+			if (!t) {
 				error("Can't expand storage for option %s.",
 				    dhcp_options[code].name);
+                                return;
+                        }
 			memcpy(t, packet->options[code].data,
 				packet->options[code].len);
 			memcpy(t + packet->options[code].len,