Richard Hipp wrote: > On 11/13/15, Michael McConville <mmcco at mykolab.com> wrote: > > Hi, everyone. > > > > I've been auditing the OpenBSD codebase for calls to ctype functions > > with potentially signed chars. This is undefined on some platforms. > > I found a number of instances in Sqlite, so I ran my Coccinelle > > script on the repo. > > Thank you. You've already told us this once before.
I'm not sure if the first send ever got approved by a list moderator. It never made it through to marc.info. I should have checked the commit history before resending, though. > All of your findings are either in test programs, programs used as > part of the build process, or obsolete code that we keep around for > historical reference but which is never in fact used. None of your > findings are in the SQLite core. There are no security implications > here. Nevertheless, I went through and fixed all of these cases (even > the ones in code that is *never compiled*) a couple of weeks ago, and > checked the changes into trunk: > > https://www.sqlite.org/src/info/34eb6911afee09e7 I wasn't trying to point fingers or start an argument, and I never implied a significant security risk. Just pointing out undefined behavior where I found it. When the change is this simple, it's easier to just fix 'em all rather than reflect on the significance of each. > I suppose it is too much to ask of Coccinelle to recognize that the > following suggestion is pointless: > > > static int safe_isspace(char c){ > > - return (c&0x80)==0 ? isspace(c) : 0; > > + return (c&0x80)==0 ? isspace((unsigned char)c) : 0; > > } Yup, too much to ask. Coccinelle does "semantic patching" - you script the transformation you want done. It can parse C, so it's syntax-agnostic, but it's restricted to the current transformation you're doing. A completely different tool from general-purpose static analyzers. Here's the simple script I wrote: @@ char x; @@ ( isupper | isalnum | isalpha | isascii | isblank | iscntrl | isdigit | isgraph | islower | isprint | ispunct | isspace | isxdigit | toupper | tolower ) - (x) + ((unsigned char)x) @@ signed char x; @@ ( isupper | isalnum | isalpha | isascii | isblank | iscntrl | isdigit | isgraph | islower | isprint | ispunct | isspace | isxdigit | toupper | tolower ) - (x) + ((unsigned char)x)
