Re: ftp: ctype interfaces need unsigned chars

[Todd C. Miller]

On Sat, 10 Oct 2015 16:35:02 -0700, Philip Guenther wrote:

> Some isfoo(char) usages crept back into ftp

OK.

 - todd

Re: ftp: ctype interfaces need unsigned chars

[Joerg Jung]

> Am 11.10.2015 um 04:38 schrieb Philip Guenther :
> 
>> --- 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
>>charbuffer[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@

Re: ftp: ctype interfaces need unsigned chars

[Alexander Bluhm]

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 9.
> >  */
> > for (p = buf; p < end && p < buf + 5; p++) {
> > -   if (!isdigit(*p))
> > +   if (!isdigit((unsigned char)*p))
> > break;
> > }
> > if (buf >= p || p >= end || *p != ' ')

Re: ftp: ctype interfaces need unsigned chars

[Theo de Raadt]

> 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.

Re: ftp: ctype interfaces need unsigned chars

[Philip Guenther]

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

Re: ftp: ctype interfaces need unsigned chars

[Philip Guenther]

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 -  1.39
+++ usr.sbin/httpd/httpd.c  11 Oct 2015 02:29:33 -
@@ -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.c7 Sep 2015 14:46:24 -   1.99
+++ usr.sbin/httpd/server_http.c11 Oct 2015 02:29:33 -
@@ -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 */

ftp: ctype interfaces need unsigned chars

[Philip Guenther]

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 -  1.142
+++ ftp/fetch.c 10 Oct 2015 23:30:52 -
@@ -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([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;

Re: ftp: ctype interfaces need unsigned chars

[Philip Guenther]

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  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 9.
>*/
>   for (p = buf; p < end && p < buf + 5; p++) {
> - if (!isdigit(*p))
> + if (!isdigit((unsigned char)*p))
>   break;
>   }
>   if (buf >= p || p >= end || *p != ' ')

Re: ftp: ctype interfaces need unsigned chars

[Philip Guenther]

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?

Re: ftp: ctype interfaces need unsigned chars

[Bob Beck]

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 -  1.142
> +++ ftp/fetch.c   10 Oct 2015 23:30:52 -
> @@ -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([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?

Re: ftp: ctype interfaces need unsigned chars

[Philip Guenther]

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
>   charbuffer[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.c7 Sep 2015 15:36:53 -   1.4
+++ usr.sbin/smtpd/rfc2822.c11 Oct 2015 02:37:03 -
@@ -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
charbuffer[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 */

Re: ftp: ctype interfaces need unsigned chars

[Philip Guenther]

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 *) _line;

It should be either:
   p = data_line;
or
   p = _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.c4 Dec 2013 02:18:05 -   1.15
+++ usr.sbin/ypserv/ypserv/acl.c11 Oct 2015 02:46:11 -
@@ -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 *) _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 *) _line;
+   p = (unsigned char *)data_line;
 
/* State 1: Initial State */
state = ACLS_INIT;

Re: ftp: ctype interfaces need unsigned chars

[Michael McConville]

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
charbuffer[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 9.
 */
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 

Re: ftp: ctype interfaces need unsigned chars

[Philip Guenther]

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 -  1.14
+++ tcpdump/print-decnet.c  11 Oct 2015 03:25:02 -
@@ -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...gaaahh.  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.

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.c21 Aug 2015 02:07:32 -  1.14
> +++ tcpdump/print-decnet.c11 Oct 2015 03:25:02 -
> @@ -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...gaaahh.  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.