{"thread":{"id":"2085","subject":"Usage of isspace and friends","startedAt":"2005-10-12T01:40:26Z","lastAt":"2005-10-13T16:07:44Z","messageCount":10,"participants":["Morten Welinder","Junio C Hamano","Antti-Juhani Kaijanaho","H. Peter Anvin","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"10052","messageId":"118833cc0510111840k715e1190l54ad65f821c77848@mail.gmail.com","threadId":"2085","inReplyTo":null,"subject":"Usage of isspace and friends","fromName":"Morten Welinder","fromEmail":"mwelinder@gmail.com","sentAt":"2005-10-12T01:40:26Z","receivedAt":"2005-10-12T01:40:26Z","isPatch":false,"sender":{"key":"mwelinder@gmail.com","avatar":null},"body":"Someone needs to audit the usage of isspace, tolower, and friends.  There are\nthings like this in the code:\n\nstatic int is_dev_null(const char *str)\n{\n\treturn !memcmp(\"/dev/null\", str, 9) && isspace(str[9]);\n}\n\nSince str[9] is of type char it should not be used as a argument to\nisspace directly,\nbut rather be cast to unsigned char:\n\n    ... isspace((unsigned char)str[9]);\n\nAdmittedly that is ugly.  Blame K&R.  (Glibc has a partial workaround for this\nkind of coding bug.  On the up side you won't get a crash, but on the down\nside you can get the wrong result.)\n\nMorten\n"},{"id":"10055","messageId":"7vachd6hdx.fsf@assigned-by-dhcp.cox.net","threadId":"2085","inReplyTo":"118833cc0510111840k715e1190l54ad65f821c77848@mail.gmail.com","subject":"Re: Usage of isspace and friends","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-10-13T06:49:30Z","receivedAt":"2005-10-13T06:49:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Morten Welinder <mwelinder@gmail.com> writes:\n\n> Someone needs to audit the usage of isspace, tolower, and\n> friends.  There are things like this in the code:\n>\n> static int is_dev_null(const char *str)\n> {\n> \treturn !memcmp(\"/dev/null\", str, 9) && isspace(str[9]);\n> }\n>\n> Since str[9] is of type char it should not be used as a argument to\n> isspace directly,\n> but rather be cast to unsigned char:\n>\n>     ... isspace((unsigned char)str[9]);\n\nHuh?  isspace is \"int isspace(int)\".  Presumably standard\nintegral promotion rules applies here whether char is signed or\nunsigned, doesn't it?\n\nThe snippet you quoted is from apply.c, and I would say what is\nmore problematic is that we do not force C locale while parsing\nthe diff (see another thread -- we would want to process diffs\nas byte streams).\n"},{"id":"10058","messageId":"434E1AE8.9010509@kaijanaho.info","threadId":"2085","inReplyTo":"7vachd6hdx.fsf@assigned-by-dhcp.cox.net","subject":"Re: Usage of isspace and friends","fromName":"Antti-Juhani Kaijanaho","fromEmail":"antti-juhani@kaijanaho.info","sentAt":"2005-10-13T08:29:28Z","receivedAt":"2005-10-13T08:29:28Z","isPatch":false,"sender":{"key":"antti-juhani@kaijanaho.info","avatar":"https://gravatar.com/avatar/d18220c6252159e5b879101731700abcdc16af1fa9bc34d0ed13e2f35f87e222?d=mp&s=160"},"body":"Junio C Hamano wrote:\n> Morten Welinder <mwelinder@gmail.com> writes:\n>>Since str[9] is of type char it should not be used as a argument to\n>>isspace directly,\n>>but rather be cast to unsigned char:\n>>\n>>    ... isspace((unsigned char)str[9]);\n> \n> \n> Huh?  isspace is \"int isspace(int)\".  Presumably standard\n> integral promotion rules applies here whether char is signed or\n> unsigned, doesn't it?\n\nOf course, but that's not the issue.  isspace treats its parameter as if\nit had been converted from unsigned char to int.  If char is signed,\nïsspace will mistreat those characters that have a negative value.\nThen again, I don't think a space character, one that the C locale\nregards as such, anyway, wiill ever have a negative value, so the issue\nis rather academic.\n\n> The snippet you quoted is from apply.c, and I would say what is\n> more problematic is that we do not force C locale while parsing\n> the diff\n\nQuite true.  One reason I tend to avoid the standard is* functions in my\nown code.\n\n-- \nAntti-Juhani\n"},{"id":"10063","messageId":"434E60AB.8030607@zytor.com","threadId":"2085","inReplyTo":"7vachd6hdx.fsf@assigned-by-dhcp.cox.net","subject":"Re: Usage of isspace and friends","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-10-13T13:27:07Z","receivedAt":"2005-10-13T13:27:07Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Junio C Hamano wrote:\n> \n> Huh?  isspace is \"int isspace(int)\".  Presumably standard\n> integral promotion rules applies here whether char is signed or\n> unsigned, doesn't it?\n> \n> The snippet you quoted is from apply.c, and I would say what is\n> more problematic is that we do not force C locale while parsing\n> the diff (see another thread -- we would want to process diffs\n> as byte streams).\n> \n\nThe problem is that isspace() is defined to operate on an integer which \ncan be an unsigned char value promoted to int or EOF (-1).\n\n\t-hpa\n"},{"id":"10067","messageId":"Pine.LNX.4.64.0510130756550.15297@g5.osdl.org","threadId":"2085","inReplyTo":"7vachd6hdx.fsf@assigned-by-dhcp.cox.net","subject":"Re: Usage of isspace and friends","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-10-13T15:04:17Z","receivedAt":"2005-10-13T15:04:17Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 12 Oct 2005, Junio C Hamano wrote:\n> \n> Huh?  isspace is \"int isspace(int)\".  Presumably standard\n> integral promotion rules applies here whether char is signed or\n> unsigned, doesn't it?\n\nNo.\n\nThe input range for the \"isxxxxx()\" macros is the same as the range for \nthe \"[f]getc[h]()\" family: unsigned char + EOF (the latter usually being \n-1).\n\nSo Morten is right - if you have a \"char *\", it should not be dereferenced \nand used directly, although I think glibc does the right thing (and, in \nfact, I can't understand why the standards haven't been updated to do the \nright thing: it's _not_ that hard. In fact, it should be trivial apart \nfrom the special case of \"255\" that looks undistinguishable from EOF in \nsigned char representation).\n\nI'm almost goign to suggest that we do our own ctype.h, just to get the \nsane semantics: we want locale-independence, _and_ we want the right \nsigned behaviour. Plus we only use a very small subset of ctype.h anyway \n(isspace, isalpha, isdigit and isalnum).\n\n\t\t\tLinus\n"},{"id":"10068","messageId":"7vbr1t4e29.fsf@assigned-by-dhcp.cox.net","threadId":"2085","inReplyTo":"434E60AB.8030607@zytor.com","subject":"Re: Usage of isspace and friends","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-10-13T15:44:14Z","receivedAt":"2005-10-13T15:44:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"H. Peter Anvin\" <hpa@zytor.com> writes:\n\n> The problem is that isspace() is defined to operate on an integer which \n> can be an unsigned char value promoted to int or EOF (-1).\n\nAh, thanks.\n"},{"id":"10069","messageId":"434E8117.3090102@zytor.com","threadId":"2085","inReplyTo":"Pine.LNX.4.64.0510130756550.15297@g5.osdl.org","subject":"Re: Usage of isspace and friends","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-10-13T15:45:27Z","receivedAt":"2005-10-13T15:45:27Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Linus Torvalds wrote:\n> \n> So Morten is right - if you have a \"char *\", it should not be dereferenced \n> and used directly, although I think glibc does the right thing (and, in \n> fact, I can't understand why the standards haven't been updated to do the \n> right thing: it's _not_ that hard. In fact, it should be trivial apart \n> from the special case of \"255\" that looks undistinguishable from EOF in \n> signed char representation).\n> \n\nBecause of the special case of 255 which looks indistinguishable from \nEOF, therefore making it required?\n\nThe original mistake, of course, was allowing EOF to be passed to the \nvarious isxxx() macros.\n\n\t-hpa\n"},{"id":"10070","messageId":"Pine.LNX.4.64.0510130838240.15297@g5.osdl.org","threadId":"2085","inReplyTo":"Pine.LNX.4.64.0510130756550.15297@g5.osdl.org","subject":"Re: Usage of isspace and friends","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-10-13T15:46:47Z","receivedAt":"2005-10-13T15:46:47Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 13 Oct 2005, Linus Torvalds wrote:\n> \n> I'm almost goign to suggest that we do our own ctype.h, just to get the \n> sane semantics: we want locale-independence, _and_ we want the right \n> signed behaviour. Plus we only use a very small subset of ctype.h anyway \n> (isspace, isalpha, isdigit and isalnum).\n\nMaybe something like this.\n\nNo, I'm not 100% sure we need it. But hey, it's probably less complicated \nthan trying to de-localize various different targets.\n\nIs there anything else that is locale-dependent that we use in the C \ntoolchain?\n\n\t\tLinus\n\n---\ndiff-tree f6fea67a590196d81bc4c6a6be1a16dd8bf2815d (from d06b689a933f6d2130f8afdf1ac0ddb83eeb59ab)\nAuthor: Linus Torvalds <torvalds@osdl.org>\nDate:   Thu Oct 13 08:36:35 2005 -0700\n\n    Add locale-independent (and stupid) ctype.\n    \n    It's also safe for signed chars.\n\ndiff --git a/Makefile b/Makefile\nindex 5e7d055..31e62d4 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -158,7 +158,8 @@ LIB_OBJS = \\\n \tobject.o pack-check.o patch-delta.o path.o pkt-line.o \\\n \tquote.o read-cache.o refs.o run-command.o \\\n \tserver-info.o setup.o sha1_file.o sha1_name.o strbuf.o \\\n-\ttag.o tree.o usage.o config.o environment.o $(DIFF_OBJS)\n+\ttag.o tree.o usage.o config.o environment.o ctype.o \\\n+\t$(DIFF_OBJS)\n \n LIBS = $(LIB_FILE)\n LIBS += -lz\ndiff --git a/apply.c b/apply.c\nindex 155fbe8..f4d00f2 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -6,7 +6,6 @@\n  * This applies patches on top of some (arbitrary) version of the SCM.\n  *\n  */\n-#include <ctype.h>\n #include <fnmatch.h>\n #include \"cache.h\"\n \ndiff --git a/cache.h b/cache.h\nindex 1a7e047..a465952 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -386,4 +386,30 @@ extern int git_config_bool(const char *,\n extern char git_default_email[MAX_GITNAME];\n extern char git_default_name[MAX_GITNAME];\n \n+/* Sane ctype - no locale, and works with signed chars */\n+#undef isspace\n+#undef isdigit\n+#undef isalpha\n+#undef isalnum\n+#undef tolower\n+#undef toupper\n+extern unsigned char sane_ctype[256];\n+#define GIT_SPACE 0x01\n+#define GIT_DIGIT 0x02\n+#define GIT_ALPHA 0x04\n+#define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)\n+#define isspace(x) sane_istest(x,GIT_SPACE)\n+#define isdigit(x) sane_istest(x,GIT_DIGIT)\n+#define isalpha(x) sane_istest(x,GIT_ALPHA)\n+#define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)\n+#define tolower(x) sane_case((unsigned char)(x), 0x20)\n+#define toupper(x) sane_case((unsigned char)(x), 0)\n+\n+static inline int sane_case(int x, int high)\n+{\n+\tif (sane_istest(x, GIT_ALPHA))\n+\t\tx = (x & ~0x20) | high;\n+\treturn x;\n+}\n+\n #endif /* CACHE_H */\ndiff --git a/commit-tree.c b/commit-tree.c\nindex 030fb70..ea0fdd4 100644\n--- a/commit-tree.c\n+++ b/commit-tree.c\n@@ -7,7 +7,6 @@\n \n #include <pwd.h>\n #include <time.h>\n-#include <ctype.h>\n \n #define BLOCKING (1ul << 14)\n \ndiff --git a/commit.c b/commit.c\nindex f735f98..8f40318 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1,4 +1,3 @@\n-#include <ctype.h>\n #include \"tag.h\"\n #include \"commit.h\"\n #include \"cache.h\"\ndiff --git a/config.c b/config.c\nindex 9b7c6f2..519fecf 100644\n--- a/config.c\n+++ b/config.c\n@@ -1,4 +1,3 @@\n-#include <ctype.h>\n \n #include \"cache.h\"\n \ndiff --git a/convert-objects.c b/convert-objects.c\nindex 9ad0c77..a892013 100644\n--- a/convert-objects.c\n+++ b/convert-objects.c\n@@ -1,6 +1,5 @@\n #define _XOPEN_SOURCE /* glibc2 needs this */\n #include <time.h>\n-#include <ctype.h>\n #include \"cache.h\"\n \n struct entry {\ndiff --git a/ctype.c b/ctype.c\nnew file mode 100644\nindex 0000000..56bdffa\n--- /dev/null\n+++ b/ctype.c\n@@ -0,0 +1,23 @@\n+/*\n+ * Sane locale-independent, ASCII ctype.\n+ *\n+ * No surprises, and works with signed and unsigned chars.\n+ */\n+#include \"cache.h\"\n+\n+#define SS GIT_SPACE\n+#define AA GIT_ALPHA\n+#define DD GIT_DIGIT\n+\n+unsigned char sane_ctype[256] = {\n+\t 0,  0,  0,  0,  0,  0,  0,  0,  0, SS, SS,  0,  0, SS,  0,  0,\t\t/* 0-15 */\n+\t 0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,\t\t/* 16-15 */\n+\tSS,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,\t\t/* 32-15 */\n+\tDD, DD, DD, DD, DD, DD, DD, DD, DD, DD,  0,  0,  0,  0,  0,  0,\t\t/* 48-15 */\n+\t 0, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA,\t\t/* 64-15 */\n+\tAA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA,  0,  0,  0,  0,  0,\t\t/* 80-15 */\n+\t 0, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA,\t\t/* 96-15 */\n+\tAA, AA, AA, AA, AA, AA, AA, AA, AA, AA, AA,  0,  0,  0,  0,  0,\t\t/* 112-15 */\n+\t/* Nothing in the 128.. range */\n+};\n+\ndiff --git a/date.c b/date.c\nindex b21cadc..63f5a09 100644\n--- a/date.c\n+++ b/date.c\n@@ -4,7 +4,6 @@\n  * Copyright (C) Linus Torvalds, 2005\n  */\n \n-#include <ctype.h>\n #include <time.h>\n \n #include \"cache.h\"\ndiff --git a/diff-tree.c b/diff-tree.c\nindex 2203fa5..8517220 100644\n--- a/diff-tree.c\n+++ b/diff-tree.c\n@@ -1,4 +1,3 @@\n-#include <ctype.h>\n #include \"cache.h\"\n #include \"diff.h\"\n #include \"commit.h\"\ndiff --git a/ident.c b/ident.c\nindex 7a9f567..1bfbc6f 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -9,7 +9,6 @@\n \n #include <pwd.h>\n #include <time.h>\n-#include <ctype.h>\n \n static char git_default_date[50];\n \ndiff --git a/mailsplit.c b/mailsplit.c\nindex 0f8100d..189f4ed 100644\n--- a/mailsplit.c\n+++ b/mailsplit.c\n@@ -11,7 +11,6 @@\n #include <sys/stat.h>\n #include <string.h>\n #include <stdio.h>\n-#include <ctype.h>\n #include <assert.h>\n #include \"cache.h\"\n \ndiff --git a/pack-objects.c b/pack-objects.c\nindex 3d62278..83ac22b 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -1,4 +1,3 @@\n-#include <ctype.h>\n #include \"cache.h\"\n #include \"object.h\"\n #include \"delta.h\"\ndiff --git a/patch-id.c b/patch-id.c\nindex 960e7ce..edbc4aa 100644\n--- a/patch-id.c\n+++ b/patch-id.c\n@@ -1,4 +1,3 @@\n-#include <ctype.h>\n #include \"cache.h\"\n \n static void flush_current_id(int patchlen, unsigned char *id, SHA_CTX *c)\ndiff --git a/refs.c b/refs.c\nindex 5a8cbd4..42240d2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2,7 +2,6 @@\n #include \"cache.h\"\n \n #include <errno.h>\n-#include <ctype.h>\n \n /* We allow \"recursive\" symbolic refs. Only within reason, though */\n #define MAXDEPTH 5\ndiff --git a/update-ref.c b/update-ref.c\nindex 4a1704c..65dc3d6 100644\n--- a/update-ref.c\n+++ b/update-ref.c\n@@ -1,6 +1,5 @@\n #include \"cache.h\"\n #include \"refs.h\"\n-#include <ctype.h>\n \n static const char git_update_ref_usage[] = \"git-update-ref <refname> <value> [<oldval>]\";\n \n"},{"id":"10071","messageId":"Pine.LNX.4.64.0510130847190.15297@g5.osdl.org","threadId":"2085","inReplyTo":"434E8117.3090102@zytor.com","subject":"Re: Usage of isspace and friends","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-10-13T15:56:27Z","receivedAt":"2005-10-13T15:56:27Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 13 Oct 2005, H. Peter Anvin wrote:\n> \n> Because of the special case of 255 which looks indistinguishable from EOF,\n> therefore making it required?\n\nYeah, and I agree, that was a mistake. It could have been fixed by making \nEOF be MIN_INT (or any other value outside the range of either \"unsigned \nchar\" or \"signed char\" - preferably still negative), but there are \nprobably programs that depend on it being -1. \n\nThe stupid thing I just posted doesn't care. It happens to return 0 for \nEOF for all cases, but that's a side effect of (a) not doing locales (so \n255 is never printable or alpha) and (b) not even implementing iscntrl().\n\nIn general, the rule for ctype and EOF _should_ have been that it's part \nof an acceptable input range, but that the return value is undefined ;)\n\n(Which would allow you to test EOF later, and not worry about any faults).\n\n\t\tLinus\n"},{"id":"10073","messageId":"434E8650.7060604@zytor.com","threadId":"2085","inReplyTo":"Pine.LNX.4.64.0510130847190.15297@g5.osdl.org","subject":"Re: Usage of isspace and friends","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2005-10-13T16:07:44Z","receivedAt":"2005-10-13T16:07:44Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Linus Torvalds wrote:\n> \n> Yeah, and I agree, that was a mistake. It could have been fixed by making \n> EOF be MIN_INT (or any other value outside the range of either \"unsigned \n> char\" or \"signed char\" - preferably still negative), but there are \n> probably programs that depend on it being -1. \n> \n> The stupid thing I just posted doesn't care. It happens to return 0 for \n> EOF for all cases, but that's a side effect of (a) not doing locales (so \n> 255 is never printable or alpha) and (b) not even implementing iscntrl().\n> \n\nGiven that for isprint() et al are useless for non-ASCII UTF-8 anyway, \nmight as well (they are not defined to be able to take wide character \nvalues.)\n\n\t-hpa\n"}]}