From 137ae2e9cf0dc3596641e7c8b91d15307a35319e Mon Sep 17 00:00:00 2001

From: Taylor R Campbell <campbell+dnsmasq@mumble.net>

Date: Sat, 25 Feb 2023 15:00:30 +0000

Subject: [PATCH] Avoid undefined behaviour with the ctype(3) functions.

As defined in the C standard:

	In all cases the argument is an int, the value of which shall

	be representable as an unsigned char or shall equal the value

	of the macro EOF.  If the argument has any other value, the

	behavior is undefined.

This is because they're designed to work with the int values returned

by getc or fgetc; they need extra work to handle a char value.

If EOF is -1 (as it almost always is), with 8-bit bytes, the allowed

inputs to the ctype(3) functions are:

	{-1, 0, 1, 2, 3, ..., 255}.

However, on platforms where char is signed, such as x86 with the

usual ABI, code like

	char *arg = ...;

	... isspace(*arg) ...

may pass in values in the range:

	{-128, -127, -126, ..., -2, -1, 0, 1, ..., 127}.

This has two problems:

1. Inputs in the set {-128, -127, -126, ..., -2} are forbidden.

2. The non-EOF byte 0xff is conflated with the value EOF = -1, so

   even though the input is not forbidden, it may give the wrong

   answer.

Casting char to int first before passing the result to ctype(3)

doesn't help: inputs like -128 are unchanged by this cast.  It is

necessary to cast char inputs to unsigned char first; you can then

cast to int if you like but there's no need because the functions

will always convert the argument to int by definition.  So the above

fragment needs to be:

	char *arg = ...;

	... isspace((unsigned char)*arg) ...

This patch inserts unsigned char casts where necessary, and changes

int casts to unsigned char casts where the input is char.

I left alone int casts where the input is unsigned char already --

they're not immediately harmful, although they would have the effect

of suppressing some compiler warnings if the input is ever changed to

be char instead of unsigned char, so it might be better to remove

those casts too.

I also left alone calls where the input is int to begin with because

it came from getc; casting to unsigned char here would be wrong, of

course.

---

 src/dhcp-common.c | 6 +++---

 src/dhcp.c        | 6 +++---

 src/loop.c        | 2 +-

 src/option.c      | 8 ++++----

 src/rfc1035.c     | 2 +-

 src/rfc2131.c     | 2 +-

 src/tftp.c        | 2 +-

 7 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/dhcp-common.c b/src/dhcp-common.c

index 84081ce..b4d255e 100644

--- a/src/dhcp-common.c

+++ b/src/dhcp-common.c

@@ -838,7 +838,7 @@ char *option_string(int prot, unsigned int opt, unsigned char *val, int opt_len,

 		for (i = 0, j = 0; i < opt_len && j < buf_len ; i++)

 		  {

 		    char c = val[i];

-		    if (isprint((int)c))

+		    if (isprint((unsigned char)c))

 		      buf[j++] = c;

 		  }

 #ifdef HAVE_DHCP6

@@ -852,7 +852,7 @@ char *option_string(int prot, unsigned int opt, unsigned char *val, int opt_len,

 		    for (k = i + 1; k < opt_len && k < l && j < buf_len ; k++)

 		     {

 		       char c = val[k];

-		       if (isprint((int)c))

+		       if (isprint((unsigned char)c))

 			 buf[j++] = c;

 		     }

 		    i = l;

@@ -873,7 +873,7 @@ char *option_string(int prot, unsigned int opt, unsigned char *val, int opt_len,

 		    for (k = 0; k < len && j < buf_len; k++)

 		      {

 		       char c = *p++;

-		       if (isprint((int)c))

+		       if (isprint((unsigned char)c))

 			 buf[j++] = c;

 		     }

 		    i += len +2;

diff --git a/src/dhcp.c b/src/dhcp.c

index 42d819f..e578391 100644

--- a/src/dhcp.c

+++ b/src/dhcp.c

@@ -916,14 +916,14 @@ void dhcp_read_ethers(void)

       lineno++;

-      while (strlen(buff) > 0 && isspace((int)buff[strlen(buff)-1]))

+      while (strlen(buff) > 0 && isspace((unsigned char)buff[strlen(buff)-1]))

 	buff[strlen(buff)-1] = 0;

       if ((*buff == '#') || (*buff == '+') || (*buff == 0))

 	continue;

-      for (ip = buff; *ip && !isspace((int)*ip); ip++);

-      for(; *ip && isspace((int)*ip); ip++)

+      for (ip = buff; *ip && !isspace((unsigned char)*ip); ip++);

+      for(; *ip && isspace((unsigned char)*ip); ip++)

 	*ip = 0;

       if (!*ip || parse_hex(buff, hwaddr, ETHER_ADDR_LEN, NULL, NULL) != ETHER_ADDR_LEN)

 	{

diff --git a/src/loop.c b/src/loop.c

index cd4855e..19bfae0 100644

--- a/src/loop.c

+++ b/src/loop.c

@@ -92,7 +92,7 @@ int detect_loop(char *query, int type)

     return 0;

   for (i = 0; i < 8; i++)

-    if (!isxdigit(query[i]))

+    if (!isxdigit((unsigned char)query[i]))

       return 0;

   uid = strtol(query, NULL, 16);

diff --git a/src/option.c b/src/option.c

index e4810fd..1090bca 100644

--- a/src/option.c

+++ b/src/option.c

@@ -2751,7 +2751,7 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma

 		ret_err(gen_err);

 	      for (p = arg; *p; p++)

-		if (!isxdigit((int)*p))

+		if (!isxdigit((unsigned char)*p))

 		  ret_err(gen_err);

 	      set_option_bool(OPT_UMBRELLA_DEVID);

@@ -4836,7 +4836,7 @@ err:

 	    new->target = target;

 	    new->ttl = ttl;

-	    for (arg += arglen+1; *arg && isspace(*arg); arg++);

+	    for (arg += arglen+1; *arg && isspace((unsigned char)*arg); arg++);

 	  }

 	break;

@@ -5227,7 +5227,7 @@ err:

 	unhide_metas(keyhex);

 	/* 4034: "Whitespace is allowed within digits" */

 	for (cp = keyhex; *cp; )

-	  if (isspace(*cp))

+	  if (isspace((unsigned char)*cp))

 	    for (cp1 = cp; *cp1; cp1++)

 	      *cp1 = *(cp1+1);

 	  else

@@ -5315,7 +5315,7 @@ static void read_file(char *file, FILE *f, int hard_opt, int from_script)

 	      memmove(p, p+1, strlen(p+1)+1);

 	    }

-	  if (isspace(*p))

+	  if (isspace((unsigned char)*p))

 	    {

 	      *p = ' ';

 	      white = 1;

diff --git a/src/rfc1035.c b/src/rfc1035.c

index 5c0df56..1693253 100644

--- a/src/rfc1035.c

+++ b/src/rfc1035.c

@@ -519,7 +519,7 @@ static int print_txt(struct dns_header *header, const size_t qlen, char *name,

       /* make counted string zero-term and sanitise */

       for (i = 0; i < len; i++)

 	{

-	  if (!isprint((int)*(p3+1)))

+	  if (!isprint((unsigned char)*(p3+1)))

 	    break;

 	  *p3 = *(p3+1);

 	  p3++;

diff --git a/src/rfc2131.c b/src/rfc2131.c

index 17e97b5..5190982 100644

--- a/src/rfc2131.c

+++ b/src/rfc2131.c

@@ -1678,7 +1678,7 @@ static int sanitise(unsigned char *opt, char *buf)

   for (i = option_len(opt); i > 0; i--)

     {

       char c = *p++;

-      if (isprint((int)c))

+      if (isprint((unsigned char)c))

 	*buf++ = c;

     }

   *buf = 0; /* add terminator */

diff --git a/src/tftp.c b/src/tftp.c

index 0861f37..8e1dc4a 100644

--- a/src/tftp.c

+++ b/src/tftp.c

@@ -405,7 +405,7 @@ void tftp_request(struct listener *listen, time_t now)

 	if (*p == '\\')

 	  *p = '/';

 	else if (option_bool(OPT_TFTP_LC))

-	  *p = tolower(*p);

+	  *p = tolower((unsigned char)*p);

       strcpy(daemon->namebuff, "/");

       if (prefix)

-- 

2.33.7