[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openbsd-tech
Subject:    Loops depending on undefined behavior
From:       Michael McConville <mmcco () mykolab ! com>
Date:       2016-01-26 3:20:53
Message-ID: 20160126032052.GA25388 () thinkpad ! swarthmore ! edu
[Download RAW message or body]

I've discussed the below with otto@, who thought it was sound but wanted
confirmation before I commit.

While debugging rarpd(8) for unrelated reasons, I noticed a loop of the
following form:

> for (i = 1; i; i <<= 1)

where i is an int.

This relies on shifting into and then out of the sign bit, both of which
are undefined operations. The loop contains no breaks or returns, so
they are always executed.

It was copied around a few times, so there are a few nearly identical
instances included in the below diff.

These instances use i for:

 o boolean tests
 o comparison with small positive constants (RTA_*)
 o &ing with an int

The third case seems questionable. However, (IIUC) integer conversion
rules dictate that the int is implicitly cast to an unsigned int[1] and
that two's complement arithmetic is preserved in this cast.[2]

C integer type rules are such a joy to work with...

Am I interpreting this correctly?

[1] Usual Arithmetic Conversions #3 in:
https://www.securecoding.cert.org/confluence/x/QgE

[2] C99/C11 6.3.1.3
http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1570.pdf


Index: sbin/dhclient/dhclient.c
===================================================================
RCS file: /cvs/src/sbin/dhclient/dhclient.c,v
retrieving revision 1.370
diff -u -p -r1.370 dhclient.c
--- sbin/dhclient/dhclient.c	12 Dec 2015 14:48:17 -0000	1.370
+++ sbin/dhclient/dhclient.c	21 Jan 2016 02:16:17 -0000
@@ -160,7 +160,7 @@ int
 findproto(char *cp, int n)
 {
 	struct sockaddr *sa;
-	int i;
+	unsigned int i;
 
 	if (n == 0)
 		return -1;
Index: usr.sbin/arp/arp.c
===================================================================
RCS file: /cvs/src/usr.sbin/arp/arp.c,v
retrieving revision 1.70
diff -u -p -r1.70 arp.c
--- usr.sbin/arp/arp.c	8 Dec 2015 14:20:24 -0000	1.70
+++ usr.sbin/arp/arp.c	21 Jan 2016 02:16:17 -0000
@@ -695,7 +695,7 @@ rtget(struct sockaddr_inarp **sinp, stru
 	struct sockaddr_dl *sdl = NULL;
 	struct sockaddr *sa;
 	char *cp;
-	int i;
+	unsigned int i;
 
 	if (rtmsg(RTM_GET) < 0)
 		return (1);
Index: usr.sbin/ndp/ndp.c
===================================================================
RCS file: /cvs/src/usr.sbin/ndp/ndp.c,v
retrieving revision 1.68
diff -u -p -r1.68 ndp.c
--- usr.sbin/ndp/ndp.c	12 Jan 2016 09:47:13 -0000	1.68
+++ usr.sbin/ndp/ndp.c	21 Jan 2016 02:16:17 -0000
@@ -879,7 +879,7 @@ rtget(struct sockaddr_in6 **sinp, struct
 	struct sockaddr_dl *sdl = NULL;
 	struct sockaddr *sa;
 	char *cp;
-	int i;
+	unsigned int i;
 
 	if (rtmsg(RTM_GET) < 0)
 		return (1);
Index: usr.sbin/rarpd/arptab.c
===================================================================
RCS file: /cvs/src/usr.sbin/rarpd/arptab.c,v
retrieving revision 1.25
diff -u -p -r1.25 arptab.c
--- usr.sbin/rarpd/arptab.c	19 Nov 2015 19:31:20 -0000	1.25
+++ usr.sbin/rarpd/arptab.c	21 Jan 2016 02:16:17 -0000
@@ -240,7 +240,7 @@ rtget(struct sockaddr_inarp **sinp, stru
 	struct sockaddr_dl *sdl = NULL;
 	struct sockaddr *sa;
 	char *cp;
-	int i;
+	unsigned int i;
 
 	if (rtmsg(RTM_GET) < 0)
 		return (1);

[prev in list] [next in list] [prev in thread] [next in thread]