ftp: ctype interfaces need unsigned chars

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

ftp: ctype interfaces need unsigned chars

Philip Guenther-2

Some isfoo(char) usages crept back into ftp

ok?

Philip Guenther


Index: ftp/fetch.c
===================================================================
RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.142
diff -u -p -r1.142 fetch.c
--- ftp/fetch.c 10 Sep 2015 13:43:35 -0000 1.142
+++ ftp/fetch.c 10 Oct 2015 23:30:52 -0000
@@ -1374,7 +1374,8 @@ urldecode(const char *str)
  /* Cannot use strtol here because next char
  * after %xx may be a digit.
  */
- if (c == '%' && isxdigit(str[i+1]) && isxdigit(str[i+2])) {
+ if (c == '%' && isxdigit((unsigned char)str[i+1]) &&
+    isxdigit((unsigned char)str[i+2])) {
  *ret = hextochar(&str[i+1]);
  i+=2;
  continue;
@@ -1409,7 +1410,7 @@ recode_credentials(const char *userinfo)
 char
 hextochar(const char *str)
 {
- char c, ret;
+ unsigned char c, ret;
 
  c = str[0];
  ret = c;

Reply | Threaded
Open this post in threaded view
|

Re: ftp: ctype interfaces need unsigned chars

Bob Beck-2
On Sat, Oct 10, 2015 at 04:35:02PM -0700, Philip Guenther wrote:

>
> Some isfoo(char) usages crept back into ftp
>
> ok?
>
> Philip Guenther
>
>
> Index: ftp/fetch.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
> retrieving revision 1.142
> diff -u -p -r1.142 fetch.c
> --- ftp/fetch.c 10 Sep 2015 13:43:35 -0000 1.142
> +++ ftp/fetch.c 10 Oct 2015 23:30:52 -0000
> @@ -1374,7 +1374,8 @@ urldecode(const char *str)
>   /* Cannot use strtol here because next char
>   * after %xx may be a digit.
>   */
> - if (c == '%' && isxdigit(str[i+1]) && isxdigit(str[i+2])) {
> + if (c == '%' && isxdigit((unsigned char)str[i+1]) &&
> +    isxdigit((unsigned char)str[i+2])) {
>   *ret = hextochar(&str[i+1]);
>   i+=2;
>   continue;

Yes.

> @@ -1409,7 +1410,7 @@ recode_credentials(const char *userinfo)
>  char
>  hextochar(const char *str)
>  {
> - char c, ret;
> + unsigned char c, ret;
>  
>   c = str[0];
>   ret = c;
>

Looking at the rest of this and the additions going on, doesn't ret also need to be?

Reply | Threaded
Open this post in threaded view
|

Re: ftp: ctype interfaces need unsigned chars

Theo de Raadt
In reply to this post by Philip Guenther-2
> Some isfoo(char) usages crept back into ftp

Hmm.  I wonder how we can keep these errors out of base.
Having to re-audit all the time is painful.

Reply | Threaded
Open this post in threaded view
|

Re: ftp: ctype interfaces need unsigned chars

Philip Guenther-2
On Sat, 10 Oct 2015, Theo de Raadt wrote:
> > Some isfoo(char) usages crept back into ftp
>
> Hmm.  I wonder how we can keep these errors out of base.
> Having to re-audit all the time is painful.

Right now, _ctype_ is a generic const char * pointer.  Maybe there's way
to make it a pointer to char[257] that would convince the compiler to
check this?

Reply | Threaded
Open this post in threaded view
|

Re: ftp: ctype interfaces need unsigned chars

Philip Guenther-2
In reply to this post by Bob Beck-2
On Sat, 10 Oct 2015, Bob Beck wrote:
> On Sat, Oct 10, 2015 at 04:35:02PM -0700, Philip Guenther wrote:
...

> > @@ -1409,7 +1410,7 @@ recode_credentials(const char *userinfo)
> >  char
> >  hextochar(const char *str)
> >  {
> > - char c, ret;
> > + unsigned char c, ret;
> >  
> >   c = str[0];
> >   ret = c;
> >
>
> Looking at the rest of this and the additions going on, doesn't ret also
> need to be?

You mean the return type of hextochar()?  In the end, either here or in
the caller, we need to stuff the resulting character back into a char*
string.  Doing the (unsigned char) --> (char) conversion in the
        return ret;

statement is the simplest, IMO.  Your preference?


Philip Guenther

Reply | Threaded
Open this post in threaded view
|

Re: ftp: ctype interfaces need unsigned chars

Michael McConville-2
In reply to this post by Theo de Raadt
Theo de Raadt wrote:
> > Some isfoo(char) usages crept back into ftp
>
> Hmm.  I wonder how we can keep these errors out of base.
> Having to re-audit all the time is painful.

FWIW, this is a perfect use case for Coccinelle. Below is what I dredged
up in src/usr.sbin (diff not yet carefully audited, but apparently
sane).

I've been considering cronjobbing Coccinelle scripts (having the diffs
emailed to me) to prevent these sorts of entropic regressions.


--- httpd/httpd.c
+++ /tmp/cocci-output-27324-3d6efb-httpd.c
@@ -602,7 +602,7 @@ url_decode(char *url)
  switch (*p) {
  case '%':
  /* Encoding character is followed by two hex chars */
- if (!(isxdigit(p[1]) && isxdigit(p[2])))
+ if (!(isxdigit((unsigned char)p[1]) && isxdigit((unsigned char)p[2])))
  return (NULL);
 
  hex[0] = p[1];
--- httpd/server_http.c
+++ /tmp/cocci-output-27324-ad673b-server_http.c
@@ -918,7 +918,7 @@ server_expand_http(struct client *clt, c
  /* Find previously matched substrings by index */
  for (p = val; clt->clt_srv_match.sm_nmatch &&
     (p = strstr(p, "%")) != NULL; p++) {
- if (!isdigit(*(p + 1)))
+ if (!isdigit((unsigned char)*(p + 1)))
  continue;
 
  /* Copy number, leading '%' char and add trailing \0 */
--- ldapd/syntax.c
+++ /tmp/cocci-output-29625-30ee70-syntax.c
@@ -142,7 +142,7 @@ syntax_is_printable_string(struct schema
  char *p;
 
  for (p = value; len > 0 && *p != '\0'; p++, len--) {
- if (!isalnum(*p) && strchr(special, *p) == NULL)
+ if (!isalnum((unsigned char)*p) && strchr(special, *p) == NULL)
  return 0;
  }
 
--- smtpd/rfc2822.c
+++ /tmp/cocci-output-29655-69b554-rfc2822.c
@@ -93,13 +93,13 @@ parser_feed_header(struct rfc2822_parser
  char *pos;
 
  /* new header */
- if (! isspace(*line) && *line != '\0') {
+ if (! isspace((unsigned char)*line) && *line != '\0') {
  rp->in_hdr = 1;
  if ((pos = strchr(line, ':')) == NULL)
  return 0;
  memset(rp->header.name, 0, sizeof rp->header.name);
  (void)memcpy(rp->header.name, line, pos - line);
- if (isspace(*(pos + 1)))
+ if (isspace((unsigned char)*(pos + 1)))
  return parser_feed_header(rp, pos + 1);
  else {
  *pos = ' ';
@@ -169,7 +169,7 @@ rfc2822_parser_feed(struct rfc2822_parse
  char buffer[RFC2822_MAX_LINE_SIZE+1];
 
  /* in header and line is not a continuation, execute callback */
- if (rp->in_hdr && (*line == '\0' || !isspace(*line)))
+ if (rp->in_hdr && (*line == '\0' || !isspace((unsigned char)*line)))
  header_callback(rp);
 
  /* no longer in headers */
--- syslogd/syslogd.c
+++ /tmp/cocci-output-17550-6b6404-syslogd.c
@@ -1126,7 +1126,7 @@ octet_counting(struct evbuffer *evbuf, c
  * It can be assumed that octet-counting framing is used if a syslog
  * frame starts with a digit.
  */
- if (buf >= end || !isdigit(*buf))
+ if (buf >= end || !isdigit((unsigned char)*buf))
  return (-1);
  /*
  * SYSLOG-FRAME = MSG-LEN SP SYSLOG-MSG
@@ -1134,7 +1134,7 @@ octet_counting(struct evbuffer *evbuf, c
  * We support up to 5 digits in MSG-LEN, so the maximum is 99999.
  */
  for (p = buf; p < end && p < buf + 5; p++) {
- if (!isdigit(*p))
+ if (!isdigit((unsigned char)*p))
  break;
  }
  if (buf >= p || p >= end || *p != ' ')
--- tcpdump/print-decnet.c
+++ /tmp/cocci-output-17550-bc5c0e-print-decnet.c
@@ -761,7 +761,7 @@ pdata(u_char *dp, u_int maxlen)
 
  while (x-- > 0) {
     c = *dp++;
-    if (isprint(c))
+    if (isprint((unsigned char)c))
  putchar(c);
     else
  printf("\\%o", c & 0xFF);
--- tcpdump/print-ipsec.c
+++ /tmp/cocci-output-17550-499a71-print-ipsec.c
@@ -101,7 +101,7 @@ esp_init (char *espspec)
  s[0] = espkey[2*i];
  s[1] = espkey[2*i + 1];
  s[2] = 0;
- if (!isxdigit(s[0]) || !isxdigit(s[1])) {
+ if (!isxdigit((unsigned char)s[0]) || !isxdigit((unsigned char)s[1])) {
  free(key);
  error("espkey must be specified in hex");
  }
--- tcpdump/smbutil.c
+++ /tmp/cocci-output-10974-0cf207-smbutil.c
@@ -148,7 +148,7 @@ static int name_interpret(const uchar *i
     out--;
   *out = '\0';
   for (; *ob; ob++)
-    if (!isprint(*ob))
+    if (!isprint((unsigned char)*ob))
       *ob = 'X';
 
   return(ret);
@@ -335,7 +335,7 @@ static const uchar *fdata1(const uchar *
  int l = atoi(fmt+1);
  buf += l;
  fmt++;
- while (isdigit(*fmt)) fmt++;
+ while (isdigit((unsigned char)*fmt)) fmt++;
  break;
       }
     case 'r':
@@ -425,14 +425,14 @@ static const uchar *fdata1(const uchar *
  int l = atoi(fmt+1);
  printf("%-*.*s",l,l,buf);
  buf += l;
- fmt++; while (isdigit(*fmt)) fmt++;
+ fmt++; while (isdigit((unsigned char)*fmt)) fmt++;
  break;
       }
     case 'h':
       {
  int l = atoi(fmt+1);
  while (l--) printf("%02x",*buf++);
- fmt++; while (isdigit(*fmt)) fmt++;
+ fmt++; while (isdigit((unsigned char)*fmt)) fmt++;
  break;
       }
     case 'n':
@@ -461,7 +461,7 @@ static const uchar *fdata1(const uchar *
   buf += 16;
   break;
  }
- fmt++; while (isdigit(*fmt)) fmt++;
+ fmt++; while (isdigit((unsigned char)*fmt)) fmt++;
  break;
       }
     case 'T':
@@ -491,7 +491,7 @@ static const uchar *fdata1(const uchar *
   error("fdata1: invalid fmt: %s", fmt);
  }
  printf("%s",t?asctime(localtime(&t)):"NULL ");
- fmt++; while (isdigit(*fmt)) fmt++;
+ fmt++; while (isdigit((unsigned char)*fmt)) fmt++;
  break;
       }
     default:
--- ypserv/stdethers/stdethers.c
+++ /tmp/cocci-output-2270-d04742-stdethers.c
@@ -148,9 +148,9 @@ main(int argc, char *argv[])
  p = (char *) &data_line;
 
  k = p; /* save start of key */
- while (!isspace(*p)) /* find first "space" */
+ while (!isspace((unsigned char)*p)) /* find first "space" */
  p++;
- while (isspace(*p)) /* move over "space" */
+ while (isspace((unsigned char)*p)) /* move over "space" */
  p++;
 
  v = p; /* save start of value */
--- ypserv/stdhosts/stdhosts.c
+++ /tmp/cocci-output-2270-72dd65-stdhosts.c
@@ -112,9 +112,9 @@ main(int argc, char *argv[])
 
  p = (char *) &data_line;
  k = p; /* save start of key */
- while (!isspace(*p)) /* find first "space" */
+ while (!isspace((unsigned char)*p)) /* find first "space" */
  p++;
- while (isspace(*p)) /* replace space with <NUL> */
+ while (isspace((unsigned char)*p)) /* replace space with <NUL> */
  *p++ = '\0';
 
  v = p; /* save start of value */
--- ypserv/ypserv/acl.c
+++ /tmp/cocci-output-2270-df68c1-acl.c
@@ -161,7 +161,7 @@ acl_init(char *file)
  k = p; /* save start of verb */
  i = 0;
  while (*p != '\0' &&
-    !isspace((*p = tolower(*p)))) {
+    !isspace((unsigned char)(*p = tolower(*p)))) {
  p++;
  i++;
  }
@@ -190,7 +190,7 @@ acl_init(char *file)
  k = p; /* save start of verb */
  i = 0;
  while (*p != '\0' &&
-    !isspace((*p = tolower(*p)))) {
+    !isspace((unsigned char)(*p = tolower(*p)))) {
  p++;
  i++;
  }
@@ -231,7 +231,7 @@ acl_init(char *file)
  k = p; /* save start of verb */
  i = 0;
  while (*p != '\0' &&
-    !isspace((*p = tolower(*p)))) {
+    !isspace((unsigned char)(*p = tolower(*p)))) {
  p++;
  i++;
  }
@@ -302,7 +302,7 @@ acl_init(char *file)
  k = p; /* save start of verb */
  i = 0;
  while (*p != '\0' &&
-    !isspace((*p = tolower(*p)))) {
+    !isspace((unsigned char)(*p = tolower(*p)))) {
  p++;
  i++;
  }
@@ -325,7 +325,7 @@ acl_init(char *file)
  k = p; /* save start of verb */
  i = 0;
  while (*p != '\0' &&
-    !isspace((*p = tolower(*p)))) {
+    !isspace((unsigned char)(*p = tolower(*p)))) {
  p++;
  i++;
  }
@@ -451,7 +451,7 @@ acl_securenet(char *file)
  k = p; /* save start of verb */
  i = 0;
  while (*p != '\0' &&
-    !isspace((*p = tolower(*p)))) {
+    !isspace((unsigned char)(*p = tolower(*p)))) {
  p++;
  i++;
  }
@@ -481,7 +481,7 @@ acl_securenet(char *file)
  k = p; /* save start of verb */
  i = 0;
  while (*p != '\0' &&
-    !isspace((*p = tolower(*p)))) {
+    !isspace((unsigned char)(*p = tolower(*p)))) {
  p++;
  i++;
  }

Reply | Threaded
Open this post in threaded view
|

Re: ftp: ctype interfaces need unsigned chars

Philip Guenther-2
On Sat, 10 Oct 2015, Michael McConville wrote:
...
> FWIW, this is a perfect use case for Coccinelle. Below is what I dredged
> up in src/usr.sbin (diff not yet carefully audited, but apparently
> sane).

I'm replying to this multiple times, cc'ing in the particular maintainers
as appropriate.


> --- httpd/httpd.c
> +++ /tmp/cocci-output-27324-3d6efb-httpd.c
> @@ -602,7 +602,7 @@ url_decode(char *url)
>   switch (*p) {
>   case '%':
>   /* Encoding character is followed by two hex chars */
> - if (!(isxdigit(p[1]) && isxdigit(p[2])))
> + if (!(isxdigit((unsigned char)p[1]) && isxdigit((unsigned char)p[2])))

Needs line wrapping, and I think it reads better if De Morgan's law is
applied to make it:
                        if (!isxdigit((unsigned char)p[1]) ||
                            !isxdigit((unsigned char)p[2]))

(also indents nicer that way...)


> --- httpd/server_http.c
> +++ /tmp/cocci-output-27324-ad673b-server_http.c
> @@ -918,7 +918,7 @@ server_expand_http(struct client *clt, c
>   /* Find previously matched substrings by index */
>   for (p = val; clt->clt_srv_match.sm_nmatch &&
>      (p = strstr(p, "%")) != NULL; p++) {
> - if (!isdigit(*(p + 1)))
> + if (!isdigit((unsigned char)*(p + 1)))

I think that should be changed to use array indexing:
                if (!isdigit((unsigned char)p[1]))

oks?


Philip


Index: usr.sbin/httpd/httpd.c
===================================================================
RCS file: /cvs/src/usr.sbin/httpd/httpd.c,v
retrieving revision 1.39
diff -u -p -r1.39 httpd.c
--- usr.sbin/httpd/httpd.c 20 Aug 2015 13:00:23 -0000 1.39
+++ usr.sbin/httpd/httpd.c 11 Oct 2015 02:29:33 -0000
@@ -602,7 +602,8 @@ url_decode(char *url)
  switch (*p) {
  case '%':
  /* Encoding character is followed by two hex chars */
- if (!(isxdigit(p[1]) && isxdigit(p[2])))
+ if (!isxdigit((unsigned char)p[1]) ||
+    !isxdigit((unsigned char)p[2]))
  return (NULL);
 
  hex[0] = p[1];
Index: usr.sbin/httpd/server_http.c
===================================================================
RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
retrieving revision 1.99
diff -u -p -r1.99 server_http.c
--- usr.sbin/httpd/server_http.c 7 Sep 2015 14:46:24 -0000 1.99
+++ usr.sbin/httpd/server_http.c 11 Oct 2015 02:29:33 -0000
@@ -918,7 +918,7 @@ server_expand_http(struct client *clt, c
  /* Find previously matched substrings by index */
  for (p = val; clt->clt_srv_match.sm_nmatch &&
     (p = strstr(p, "%")) != NULL; p++) {
- if (!isdigit(*(p + 1)))
+ if (!isdigit((unsigned char)p[1]))
  continue;
 
  /* Copy number, leading '%' char and add trailing \0 */

Reply | Threaded
Open this post in threaded view
|

Re: ftp: ctype interfaces need unsigned chars

Philip Guenther-2
In reply to this post by Michael McConville-2
On Sat, 10 Oct 2015, Michael McConville wrote:
> FWIW, this is a perfect use case for Coccinelle. Below is what I dredged
> up in src/usr.sbin (diff not yet carefully audited, but apparently
> sane).

I'm replying to this multiple times, cc'ing in the particular maintainers
as appropriate.


> --- smtpd/rfc2822.c
> +++ /tmp/cocci-output-29655-69b554-rfc2822.c
> @@ -93,13 +93,13 @@ parser_feed_header(struct rfc2822_parser
>   char *pos;
>  
>   /* new header */
> - if (! isspace(*line) && *line != '\0') {
> + if (! isspace((unsigned char)*line) && *line != '\0') {

Yep


> - if (isspace(*(pos + 1)))
        > + if (isspace((unsigned char)*(pos + 1)))

Again, array indexing reads better here, IMO:
                if (isspace((unsigned char)pos[1])


> @@ -169,7 +169,7 @@ rfc2822_parser_feed(struct rfc2822_parse
>   char buffer[RFC2822_MAX_LINE_SIZE+1];
>  
>   /* in header and line is not a continuation, execute callback */
> - if (rp->in_hdr && (*line == '\0' || !isspace(*line)))
> + if (rp->in_hdr && (*line == '\0' || !isspace((unsigned char)*line)))

Yep.

ok?

Philip Guenther


Index: usr.sbin/smtpd/rfc2822.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/rfc2822.c,v
retrieving revision 1.4
diff -u -p -r1.4 rfc2822.c
--- usr.sbin/smtpd/rfc2822.c 7 Sep 2015 15:36:53 -0000 1.4
+++ usr.sbin/smtpd/rfc2822.c 11 Oct 2015 02:37:03 -0000
@@ -93,13 +93,13 @@ parser_feed_header(struct rfc2822_parser
  char *pos;
 
  /* new header */
- if (! isspace(*line) && *line != '\0') {
+ if (! isspace((unsigned char)*line) && *line != '\0') {
  rp->in_hdr = 1;
  if ((pos = strchr(line, ':')) == NULL)
  return 0;
  memset(rp->header.name, 0, sizeof rp->header.name);
  (void)memcpy(rp->header.name, line, pos - line);
- if (isspace(*(pos + 1)))
+ if (isspace((unsigned char)pos[1]))
  return parser_feed_header(rp, pos + 1);
  else {
  *pos = ' ';
@@ -169,7 +169,7 @@ rfc2822_parser_feed(struct rfc2822_parse
  char buffer[RFC2822_MAX_LINE_SIZE+1];
 
  /* in header and line is not a continuation, execute callback */
- if (rp->in_hdr && (*line == '\0' || !isspace(*line)))
+ if (rp->in_hdr && (*line == '\0' || !isspace((unsigned char)*line)))
  header_callback(rp);
 
  /* no longer in headers */

Reply | Threaded
Open this post in threaded view
|

Re: ftp: ctype interfaces need unsigned chars

Philip Guenther-2
In reply to this post by Michael McConville-2
On Sat, 10 Oct 2015, Michael McConville wrote:
> FWIW, this is a perfect use case for Coccinelle. Below is what I dredged
> up in src/usr.sbin (diff not yet carefully audited, but apparently
> sane).

The ypserv chunks show your Coccinelle script could use an enhancement...

> --- ypserv/ypserv/acl.c
> +++ /tmp/cocci-output-2270-df68c1-acl.c
> @@ -161,7 +161,7 @@ acl_init(char *file)
>   k = p; /* save start of verb */
>   i = 0;
>   while (*p != '\0' &&
> -    !isspace((*p = tolower(*p)))) {
> +    !isspace((unsigned char)(*p = tolower(*p)))) {

tolower() and toupper() follow the same rules as is*(), so the above is
still wrong: if a cast is needed for isspace() then it's also needed for
tolower().  Can you enhance your Coccinelle script to apply the same rules
to them?

This is also a case where changing the type of the variable is the cleaner
solution, IMO.

Finally, this code also has an incorrect/unnecessary use of '& of array'.  
Given these declarations:
       char data_line[1024], *p;

this line is bogus:
               p = (char *) &data_line;

It should be either:
               p = data_line;
or
               p = &data_line[0];

(With the type change to 'p', however, the cast will still be needed in
the revised diff below)

Does the diff below pass your Coccinelle script?

oks?


Philip Guenther


Index: usr.sbin/ypserv/ypserv/acl.c
===================================================================
RCS file: /cvs/src/usr.sbin/ypserv/ypserv/acl.c,v
retrieving revision 1.15
diff -u -p -r1.15 acl.c
--- usr.sbin/ypserv/ypserv/acl.c 4 Dec 2013 02:18:05 -0000 1.15
+++ usr.sbin/ypserv/ypserv/acl.c 11 Oct 2015 02:46:11 -0000
@@ -133,7 +133,8 @@ acl_add_host(int allow, struct in_addr *
 int
 acl_init(char *file)
 {
- char data_line[1024], *p, *k;
+ char data_line[1024], *k;
+ unsigned char *p;
  int line_no = 0, len, i, state;
  int allow = TRUE, error_cnt = 0;
  struct in_addr addr, mask, *host_addr;
@@ -151,7 +152,7 @@ acl_init(char *file)
  len = strlen(data_line);
  if (len == 0)
  continue;
- p = (char *) &data_line;
+ p = (unsigned char *)data_line;
 
  /* State 1: Initial State */
 
@@ -420,7 +421,8 @@ acl_init(char *file)
 int
 acl_securenet(char *file)
 {
- char data_line[1024], *p, *k;
+ char data_line[1024], *k;
+ unsigned char *p;
  int line_no = 0, len, i, allow = TRUE, state;
  int error_cnt = 0;
  struct in_addr addr, mask;
@@ -442,7 +444,7 @@ acl_securenet(char *file)
  len = strlen(data_line);
  if (len == 0)
  continue;
- p = (char *) &data_line;
+ p = (unsigned char *)data_line;
 
  /* State 1: Initial State */
  state = ACLS_INIT;

Reply | Threaded
Open this post in threaded view
|

Re: ftp: ctype interfaces need unsigned chars

Philip Guenther-2
In reply to this post by Michael McConville-2
On Sat, 10 Oct 2015, Michael McConville wrote:
> FWIW, this is a perfect use case for Coccinelle. Below is what I dredged
> up in src/usr.sbin (diff not yet carefully audited, but apparently
> sane).

These look good to me.  bluhm?

Side note: bluhm, please rename the dprintf() macro to something that
doesn't conflict with the <stdio.h> dprintf() function.  I *strongly*
recommend that, as a macro, you use something in all caps so that readers
have fair warning that evaluation may not follow the normal rules.


Philip


> --- syslogd/syslogd.c
> +++ /tmp/cocci-output-17550-6b6404-syslogd.c
> @@ -1126,7 +1126,7 @@ octet_counting(struct evbuffer *evbuf, c
>   * It can be assumed that octet-counting framing is used if a syslog
>   * frame starts with a digit.
>   */
> - if (buf >= end || !isdigit(*buf))
> + if (buf >= end || !isdigit((unsigned char)*buf))
>   return (-1);
>   /*
>   * SYSLOG-FRAME = MSG-LEN SP SYSLOG-MSG
> @@ -1134,7 +1134,7 @@ octet_counting(struct evbuffer *evbuf, c
>   * We support up to 5 digits in MSG-LEN, so the maximum is 99999.
>   */
>   for (p = buf; p < end && p < buf + 5; p++) {
> - if (!isdigit(*p))
> + if (!isdigit((unsigned char)*p))
>   break;
>   }
>   if (buf >= p || p >= end || *p != ' ')

Reply | Threaded
Open this post in threaded view
|

Re: ftp: ctype interfaces need unsigned chars

Philip Guenther-2
In reply to this post by Michael McConville-2
On Sat, 10 Oct 2015, Michael McConville wrote:
> Theo de Raadt wrote:
> > > Some isfoo(char) usages crept back into ftp
> >
> > Hmm.  I wonder how we can keep these errors out of base.
> > Having to re-audit all the time is painful.
>
> FWIW, this is a perfect use case for Coccinelle. Below is what I dredged
> up in src/usr.sbin (diff not yet carefully audited, but apparently
> sane).

I've committed this (with line wrap):

> --- ldapd/syntax.c
> +++ /tmp/cocci-output-29625-30ee70-syntax.c
> @@ -142,7 +142,7 @@ syntax_is_printable_string(struct schema
>   char *p;
>  
>   for (p = value; len > 0 && *p != '\0'; p++, len--) {
> - if (!isalnum(*p) && strchr(special, *p) == NULL)
> + if (!isalnum((unsigned char)*p) && strchr(special, *p) == NULL)


as well as this:

> --- tcpdump/print-ipsec.c
> +++ /tmp/cocci-output-17550-499a71-print-ipsec.c
> @@ -101,7 +101,7 @@ esp_init (char *espspec)
>   s[0] = espkey[2*i];
>   s[1] = espkey[2*i + 1];
>   s[2] = 0;
> - if (!isxdigit(s[0]) || !isxdigit(s[1])) {
> + if (!isxdigit((unsigned char)s[0]) || !isxdigit((unsigned char)s[1])) {


For tcpdump/print-decnet.c, I think it's best to change the variable type,
as putchar() expects an int ("EOF or unsigned char") like isprint():


--- tcpdump/print-decnet.c 21 Aug 2015 02:07:32 -0000 1.14
+++ tcpdump/print-decnet.c 11 Oct 2015 03:25:02 -0000
@@ -756,11 +756,11 @@ dnname_string(u_short dnaddr)
 static void
 pdata(u_char *dp, u_int maxlen)
 {
- char c;
+ int c;
  u_int x = maxlen;
 
  while (x-- > 0) {
-    c = *dp++;
+    c = (unsigned char)*dp++;
     if (isprint(c))
  putchar(c);
     else


For tcpdump/smbutil.c...gaaaaaaahhhhhh.  Add the return of atoi() to a
pointer and then skip all digits?  That has *FUN* results with negative
numbers and numbers greater than the length of the buffer!  fdata1() needs
to be hit repeatedly with a big stick until it stops assuming that no one
makes errors.

Reply | Threaded
Open this post in threaded view
|

Re: ftp: ctype interfaces need unsigned chars

Theo de Raadt
> as well as this:
>
> > --- tcpdump/print-ipsec.c
> > +++ /tmp/cocci-output-17550-499a71-print-ipsec.c
> > @@ -101,7 +101,7 @@ esp_init (char *espspec)
> >   s[0] = espkey[2*i];
> >   s[1] = espkey[2*i + 1];
> >   s[2] = 0;
> > - if (!isxdigit(s[0]) || !isxdigit(s[1])) {
> > + if (!isxdigit((unsigned char)s[0]) || !isxdigit((unsigned char)s[1])) {
>
>
> For tcpdump/print-decnet.c, I think it's best to change the variable type,
> as putchar() expects an int ("EOF or unsigned char") like isprint():
>
>
> --- tcpdump/print-decnet.c 21 Aug 2015 02:07:32 -0000 1.14
> +++ tcpdump/print-decnet.c 11 Oct 2015 03:25:02 -0000
> @@ -756,11 +756,11 @@ dnname_string(u_short dnaddr)
>  static void
>  pdata(u_char *dp, u_int maxlen)
>  {
> - char c;
> + int c;
>   u_int x = maxlen;
>  
>   while (x-- > 0) {
> -    c = *dp++;
> +    c = (unsigned char)*dp++;
>      if (isprint(c))
>   putchar(c);
>      else
>
>
> For tcpdump/smbutil.c...gaaaaaaahhhhhh.  Add the return of atoi() to a
> pointer and then skip all digits?  That has *FUN* results with negative
> numbers and numbers greater than the length of the buffer!  fdata1() needs
> to be hit repeatedly with a big stick until it stops assuming that no one
> makes errors.

Luckily, our tcpdump is privsep.

One day something like this is going to hurt very badly.  Poor other people.

Reply | Threaded
Open this post in threaded view
|

Re: ftp: ctype interfaces need unsigned chars

Joerg Jung
In reply to this post by Philip Guenther-2

> Am 11.10.2015 um 04:38 schrieb Philip Guenther <[hidden email]>:
>
>> --- smtpd/rfc2822.c
>> +++ /tmp/cocci-output-29655-69b554-rfc2822.c
>> @@ -93,13 +93,13 @@ parser_feed_header(struct rfc2822_parser
>>    char            *pos;
>>
>>    /* new header */
>> -    if (! isspace(*line) && *line != '\0') {
>> +    if (! isspace((unsigned char)*line) && *line != '\0') {
>
> Yep

I would remove the space between '!' and 'isspace()'.

>> -        if (isspace(*(pos + 1)))
>    > +        if (isspace((unsigned char)*(pos + 1)))
>
> Again, array indexing reads better here, IMO:
>        if (isspace((unsigned char)pos[1])

Yes.

>> @@ -169,7 +169,7 @@ rfc2822_parser_feed(struct rfc2822_parse
>>    char            buffer[RFC2822_MAX_LINE_SIZE+1];
>>
>>    /* in header and line is not a continuation, execute callback */
>> -    if (rp->in_hdr && (*line == '\0' || !isspace(*line)))
>> +    if (rp->in_hdr && (*line == '\0' || !isspace((unsigned char)*line)))
>
> Yep.
>
> ok?

ok jung@

Reply | Threaded
Open this post in threaded view
|

Re: ftp: ctype interfaces need unsigned chars

Alexander Bluhm
In reply to this post by Philip Guenther-2
On Sat, Oct 10, 2015 at 08:03:28PM -0700, Philip Guenther wrote:
> On Sat, 10 Oct 2015, Michael McConville wrote:
> > FWIW, this is a perfect use case for Coccinelle. Below is what I dredged
> > up in src/usr.sbin (diff not yet carefully audited, but apparently
> > sane).
>
> These look good to me.  bluhm?

Commited, thanks.

> > --- syslogd/syslogd.c
> > +++ /tmp/cocci-output-17550-6b6404-syslogd.c
> > @@ -1126,7 +1126,7 @@ octet_counting(struct evbuffer *evbuf, c
> >   * It can be assumed that octet-counting framing is used if a syslog
> >   * frame starts with a digit.
> >   */
> > - if (buf >= end || !isdigit(*buf))
> > + if (buf >= end || !isdigit((unsigned char)*buf))
> >   return (-1);
> >   /*
> >   * SYSLOG-FRAME = MSG-LEN SP SYSLOG-MSG
> > @@ -1134,7 +1134,7 @@ octet_counting(struct evbuffer *evbuf, c
> >   * We support up to 5 digits in MSG-LEN, so the maximum is 99999.
> >   */
> >   for (p = buf; p < end && p < buf + 5; p++) {
> > - if (!isdigit(*p))
> > + if (!isdigit((unsigned char)*p))
> >   break;
> >   }
> >   if (buf >= p || p >= end || *p != ' ')

Reply | Threaded
Open this post in threaded view
|

Re: ftp: ctype interfaces need unsigned chars

Todd C. Miller
In reply to this post by Philip Guenther-2
On Sat, 10 Oct 2015 16:35:02 -0700, Philip Guenther wrote:

> Some isfoo(char) usages crept back into ftp

OK.

 - todd