cluster: RHEL6 - rgmanager: resrules: make expand_time buffer-less

Ryan McCabe rmccabe at fedoraproject.org
Mon Jun 23 01:14:54 UTC 2014


Gitweb:        http://git.fedorahosted.org/git/?p=cluster.git;a=commitdiff;h=c4aaef4d08c0e886fbba70a0abf4cdc1ce241a27
Commit:        c4aaef4d08c0e886fbba70a0abf4cdc1ce241a27
Parent:        0e5ba8bc5938c8b92d28d1a807616d08bd3ce47e
Author:        Jan Pokorný <jpokorny at redhat.com>
AuthorDate:    Thu Apr 5 20:46:15 2012 +0200
Committer:     Ryan McCabe <rmccabe at redhat.com>
CommitterDate: Sun Jun 22 21:11:10 2014 -0400

rgmanager: resrules: make expand_time buffer-less

This has several advantages:
- safe against unchecked (tainted; restart_expire, etc.) input
- detection of int type oveflow during conversion
- more flexibility when needed (e.g. when introducing more
  strict rules)

Additionally, the int type overflow is checked before
any multiplying operation.  All overflows leads to returning 0.

Test program for interactive tests of the function provided
(use "CFLAGS=-DNO_CCS make test-expand-time disable_dbus=1",
some dependencies needed due to high coupling :/)

Resolves: rhbz#1036652

Signed-off-by: Jan Pokorný <jpokorny at redhat.com>
Signed-off-by: Ryan McCabe <rmccabe at redhat.com>
---
 rgmanager/include/reslist.h              |    2 +-
 rgmanager/src/daemons/Makefile           |   13 +++++-
 rgmanager/src/daemons/resrules.c         |   74 +++++++++++++++++++-----------
 rgmanager/src/daemons/test-expand-time.c |   25 ++++++++++
 4 files changed, 85 insertions(+), 29 deletions(-)

diff --git a/rgmanager/include/reslist.h b/rgmanager/include/reslist.h
index 68ea032..cdfc819 100644
--- a/rgmanager/include/reslist.h
+++ b/rgmanager/include/reslist.h
@@ -136,7 +136,7 @@ int res_condstart(resource_node_t **tree, resource_t *res, void *ret);
 int res_condstop(resource_node_t **tree, resource_t *res, void *ret);
 int res_exec(resource_node_t *node, int op, const char *arg, int depth);
 /*int res_resinfo(resource_node_t **tree, resource_t *res, void *ret);*/
-int expand_time(char *val);
+int expand_time(const char *val);
 int store_action(resource_act_t **actsp, char *name, int depth, int timeout, int interval);
 
 
diff --git a/rgmanager/src/daemons/Makefile b/rgmanager/src/daemons/Makefile
index a178523..43d3377 100644
--- a/rgmanager/src/daemons/Makefile
+++ b/rgmanager/src/daemons/Makefile
@@ -3,6 +3,7 @@ TARGET2= rg_test
 TARGET3= clurgmgrd
 TARGET4= cpglockd
 TARGET5= cpglockdump
+TARGET6= test-expand-time
 
 SBINDIRT=$(TARGET1) $(TARGET2) $(TARGET4) $(TARGET5)
 SBINSYMT=$(TARGET3)
@@ -47,6 +48,11 @@ OBJS4=  cpglockd.o
 
 OBJS5=	cpglockdump.o
 
+OBJS6=	test-expand-time.o \
+	reslist.o \
+	resrules.o \
+	rg_locks.o
+
 CFLAGS += -DSHAREDIR=\"${sharedir}\" -D_GNU_SOURCE
 CFLAGS += -fPIC
 CFLAGS += -I${ccsincdir} -I${cmanincdir} -I${corosyncincdir}
@@ -93,7 +99,7 @@ ${TARGET1}: ${OBJS1} ${LDDEPS}
 #
 # The data in the 'tests' directory is hand-crafted; so running 'gentests.sh'
 # will require that the developer hand-verify the correctness of the
-# resulting output prior to committing back to CVS.
+# resulting output prior to committing back to VCS.
 #
 # This is NOT meant to be an installed binary.  Rather, RPMs and/or other
 # packages should run 'make check' as part of the build process.
@@ -112,6 +118,11 @@ ${TARGET4}: ${OBJS4} ${LDDEPS}
 ${TARGET5}: ${OBJS5} ${LDDEPS}
 	$(CC) -o $@ $^ $(CPG_LDFLAGS)
 
+# This is a test of a specific parsing function;  after top-level "configure",
+# compile it locally as "CFLAGS=-DNO_CCS make test-expand-time disable_dbus=1".
+${TARGET6}: ${OBJS6}
+	$(CC) -o $@ $^ $(XML2_LDFLAGS)
+
 check: rg_test
 	cd tests && ./runtests.sh
 
diff --git a/rgmanager/src/daemons/resrules.c b/rgmanager/src/daemons/resrules.c
index 142cfcd..3cfc90f 100644
--- a/rgmanager/src/daemons/resrules.c
+++ b/rgmanager/src/daemons/resrules.c
@@ -5,6 +5,7 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
+#include <limits.h>
 #include <resgroup.h>
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -213,65 +214,84 @@ _get_version(xmlDocPtr doc, xmlXPathContextPtr ctx, char *base,
 }
 
 
+/* Relying on compiler to cope with constants efficiently */
+#define TIMEF_M  (  60 * 1       )
+#define TIMEF_H  (  60 * TIMEF_M )
+#define TIMEF_D  (  24 * TIMEF_H )
+#define TIMEF_W  (   7 * TIMEF_D )
+#define TIMEF_Y  ( 365 * TIMEF_D )
+
+/* NOTE: Only int type oveflows are checked (no targeted against time_t)  */
 int
-expand_time (char *val)
+expand_time (const char *val)
 {
-	int curval, len;
-	int ret = 0;
-	char *start = val, ival[16];
-
-	if (!val)
-		return (time_t)0;
+	int curval, tmp, ret = 0;
 
-	while (start[0]) {
+	if (!val || *val == '\0')
+		return 0;
 
-		len = 0;
+	do {
 		curval = 0;
-		memset(ival, 0, sizeof(ival));
 
-		while (isdigit(start[len])) {
-			ival[len] = start[len];
-			len++;
-		}
+		while (isdigit(*val)) {
+			tmp = *val - '0';
 
-		if (len) {
-			curval = atoi(ival);
-		} else {
-			len = 1;
+			if (curval > INT_MAX/10
+			    || (curval == INT_MAX/10 && tmp > INT_MAX%10))
+				/* Overflow */
+				break;
+
+			curval *= 10;
+			curval += tmp;
+			++val;
 		}
 
-		switch(start[len]) {
+		if (isdigit(*val))
+			/* Overflow detected */
+			return 0;
+
+		/* Watch for overflow also here */
+		switch(*val) {
 		case 0:
 		case 'S':
 		case 's':
 			break;
 		case 'M':
         	case 'm':
-			curval *= 60;
+			if (curval > INT_MAX/TIMEF_M)
+				return 0;
+			curval *= TIMEF_M;
 			break;
 		case 'h':
 		case 'H':
-			curval *= 3600;
+			if (curval > INT_MAX/TIMEF_H)
+				return 0;
+			curval *= TIMEF_H;
 			break;
 		case 'd':
 		case 'D':
-			curval *= 86400;
+			if (curval > INT_MAX/TIMEF_D)
+				return 0;
+			curval *= TIMEF_D;
 			break;
 		case 'w':
 		case 'W':
-			curval *= 604800;
+			if (curval > INT_MAX/TIMEF_W)
+				return 0;
+			curval *= TIMEF_W;
 			break;
 		case 'y':
 		case 'Y':
-			curval *= 31536000;
+			if (curval > INT_MAX/TIMEF_Y)
+				return 0;
+			curval *= TIMEF_Y;
 			break;
 		default:
 			curval = 0;
 		}
+		ret += curval;
 
-		ret += (time_t)curval;
-		start += len;
-	}
+	} while (*++val != '\0');
 
 	return ret;
 }
diff --git a/rgmanager/src/daemons/test-expand-time.c b/rgmanager/src/daemons/test-expand-time.c
new file mode 100644
index 0000000..0a15452
--- /dev/null
+++ b/rgmanager/src/daemons/test-expand-time.c
@@ -0,0 +1,25 @@
+#include <stdio.h>
+#include <time.h>
+
+/* XXX "list_head()", reslist.h should include on its own? */
+#include "list.h"
+/* XXX "restart_counter_t", dtto */
+#include "restart_counter.h"
+
+#include "reslist.h"
+
+int main(int argc, char *argv[])
+{
+	char buffer[255];
+	char *aux;
+	for (;;) {
+		printf("Time string: ");
+		if (!fgets(buffer, sizeof(buffer), stdin) && feof(stdin))
+			break;
+		aux = strchr(buffer,'\n');
+		if (aux)
+			*aux = '\0';
+		printf("Expanded   : %d\n", expand_time(buffer));
+	}
+	return EXIT_SUCCESS;
+}


More information about the cluster-commits mailing list