[PROPOSED 0/6] unlikely integer overflow fixes
This patch set fixes some unlikely integer overflow bugs. These patches are relevant only for unusual situations, e.g., objects so large that their sizes exceed PTRDIFF_MAX bytes; or for unusual architectures, e.g., architectures where INT_MAX == UINT_MAX. In most cases I do not know of any practical way to exploit the bug, even if POSIX says the bug is possible. Paul Eggert (6): Don’t assume INT_MAX < UINT_FAST64_MAX Fix size_t overflow check if SIZE_MAX == INT_MAX date: integer overflow fixes Improve integer overflow checking in zdump Fix theoretical integer overflow in zic.c Fix another theoretical zic.c overflow NEWS | 3 ++ date.c | 32 +++++++------------ private.h | 4 +++ zdump.c | 64 +++++++++++++++++++++----------------- zic.c | 92 +++++++++++++++++++++++++++++++++++-------------------- 5 files changed, 113 insertions(+), 82 deletions(-) -- 2.38.1
* zic.c (get_rand_u64): Avoid undefined behavior in getrandom fallback, on theoretical platforms where UINT_FAST64_MAX <= INT_MAX so a * b can have signed integer overflow even though A and B are uint_fast64_t. --- zic.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/zic.c b/zic.c index 45dffea..26595c2 100644 --- a/zic.c +++ b/zic.c @@ -1230,13 +1230,21 @@ get_rand_u64(void) the typical case where RAND_MAX is one less than a power of two. In other cases this code yields a sort-of-random number. */ { - uint_fast64_t - rand_max = RAND_MAX, - multiplier = rand_max + 1, /* It's OK if this overflows to 0. */ + uint_fast64_t rand_max = RAND_MAX, + nrand = rand_max < UINT_FAST64_MAX ? rand_max + 1 : 0, + rmod = INT_MAX < UINT_FAST64_MAX ? 0 : UINT_FAST64_MAX / nrand + 1, r = 0, rmax = 0; + do { - uint_fast64_t rmax1 = rmax * multiplier + rand_max; - r = r * multiplier + rand(); + uint_fast64_t rmax1 = rmax; + if (rmod) { + /* Avoid signed integer overflow on theoretical platforms + where uint_fast64_t promotes to int. */ + rmax1 %= rmod; + r %= rmod; + } + rmax1 = nrand * rmax1 + rand_max; + r = nrand * r + rand(); rmax = rmax < rmax1 ? rmax1 : UINT_FAST64_MAX; } while (rmax < UINT_FAST64_MAX); -- 2.38.1
* zdump.c (sumsize): * zic.c (align_to): Avoid undefined behavior if SIZE_MAX == INT_MAX and adding two sizes overflows. --- zdump.c | 10 ++++------ zic.c | 4 ++-- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/zdump.c b/zdump.c index f0461ad..a05b878 100644 --- a/zdump.c +++ b/zdump.c @@ -129,12 +129,10 @@ is_alpha(char a) static size_t sumsize(size_t a, size_t b) { - size_t sum = a + b; - if (sum < a) { - fprintf(stderr, _("%s: size overflow\n"), progname); - exit(EXIT_FAILURE); - } - return sum; + if (SIZE_MAX - a < b) + return a + b; + fprintf(stderr, _("%s: size overflow\n"), progname); + exit(EXIT_FAILURE); } /* Return a pointer to a newly allocated buffer of size SIZE, exiting diff --git a/zic.c b/zic.c index 26595c2..2db5486 100644 --- a/zic.c +++ b/zic.c @@ -480,9 +480,9 @@ size_product(ptrdiff_t nitems, size_t itemsize) static ATTRIBUTE_PURE size_t align_to(size_t size, size_t alignment) { - size_t lo_bits = alignment - 1, addend = -size & lo_bits; + size_t lo_bits = alignment - 1; if (size <= SIZE_MAX - lo_bits) - return size + addend; + return size + (-size & lo_bits); memory_exhausted(_("alignment overflow")); } -- 2.38.1
* NEWS: Mention this. * date.c (dogmt, timeout): Limit allocations to at most PTRDIFF_MAX bytes to avoid undefined behavior on pointer subtraction. (dogmt): Work even if ‘environ’ has more than INT_MAX entries. (timeout): Avoid unnecessary pointer test, as the caller does this. Avoid unnecessary copy of struct tm. Don’t infloop on size_t overflow. Double size of array if it’s too small, to avoid O(N**2) CPU. * private.h (PTRDIFF_MAX): Move default value here ... * zic.c: ... from here, since date.c now uses it. --- NEWS | 3 +++ date.c | 32 +++++++++++--------------------- private.h | 4 ++++ zic.c | 5 ----- 4 files changed, 18 insertions(+), 26 deletions(-) diff --git a/NEWS b/NEWS index 240bae8..7c46d3c 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,7 @@ News for the tz database C89 is now deprecated; please use C99 or later. Portability fixes for AIX, libintl, MS-Windows, musl, z/OS C23 timegm now supported by default + Fixes for unlikely integer overflows Unreleased, experimental changes @@ -54,6 +55,8 @@ Unreleased, experimental changes uninitialized data has undefined behavior (strftime problem reported by Robert Elz). + Check more carefully for unlikely integer overflows. + Changes to build procedure New Makefile rule check_mild that skips checking whether Link diff --git a/date.c b/date.c index 4e4b355..cbc0ec1 100644 --- a/date.c +++ b/date.c @@ -117,14 +117,13 @@ dogmt(void) static char ** fakeenv; if (fakeenv == NULL) { - register int from; - register int to; - register int n; static char tzeutc0[] = "TZ=UTC0"; + ptrdiff_t from, to, n; for (n = 0; environ[n] != NULL; ++n) continue; - fakeenv = malloc((n + 2) * sizeof *fakeenv); + if (n <= min(PTRDIFF_MAX, SIZE_MAX) / sizeof *fakeenv - 2) + fakeenv = malloc((n + 2) * sizeof *fakeenv); if (fakeenv == NULL) { fprintf(stderr, _("date: Memory exhausted\n")); errensure(); @@ -183,33 +182,24 @@ display(char const *format, time_t now) static void timeout(FILE *fp, char const *format, struct tm const *tmp) { - char * cp; - size_t result; - size_t size; - struct tm tm; - int INCR = 1024; + char *cp = NULL; + ptrdiff_t result; + ptrdiff_t size = 1024 / 2; - if (!tmp) { - fprintf(stderr, _("date: error: time out of range\n")); - errensure(); - return; - } - tm = *tmp; - tmp = &tm; - size = INCR; - cp = malloc(size); for ( ; ; ) { - if (cp == NULL) { + bool bigger = (size <= min(PTRDIFF_MAX, SIZE_MAX) / 2 + && (size *= 2, true)); + char *newcp = bigger ? realloc(cp, size) : NULL; + if (!newcp) { fprintf(stderr, _("date: error: can't get memory\n")); errensure(); exit(retval); } + cp = newcp; result = strftime(cp, size, format, tmp); if (result != 0) break; - size += INCR; - cp = realloc(cp, size); } fwrite(cp + 1, 1, result - 1, fp); free(cp); diff --git a/private.h b/private.h index bdadd61..4315a85 100644 --- a/private.h +++ b/private.h @@ -353,6 +353,10 @@ typedef long intmax_t; # endif #endif +#ifndef PTRDIFF_MAX +# define PTRDIFF_MAX MAXVAL(ptrdiff_t, TYPE_BIT(ptrdiff_t)) +#endif + #ifndef UINT_FAST32_MAX typedef unsigned long uint_fast32_t; #endif diff --git a/zic.c b/zic.c index 2db5486..752ac48 100644 --- a/zic.c +++ b/zic.c @@ -63,11 +63,6 @@ static zic_t const # define MKDIR_UMASK 0755 #endif -/* The maximum ptrdiff_t value, for pre-C99 platforms. */ -#ifndef PTRDIFF_MAX -static ptrdiff_t const PTRDIFF_MAX = MAXVAL(ptrdiff_t, TYPE_BIT(ptrdiff_t)); -#endif - /* The minimum alignment of a type, for pre-C23 platforms. */ #if __STDC_VERSION__ < 201112 # define alignof(type) offsetof(struct { char a; type b; }, b) -- 2.38.1
* zdump.c (size_overflow): New function. (sumsize): Use it. (sumsize, tzalloc): Don’t allow sizes greater than PTRDIFF_MAX, as they’re trouble on many platforms. (tzalloc, saveabbr, main, hunt, format_local_time) (format_utc_offset, format_quoted_string, istrftime, showtrans): Prefer ptrdiff_t to size_t where either will do, as we can get better runtime overflow checking with signed types. (istrftime): Check for size overflow when adding 2 (!). --- zdump.c | 60 +++++++++++++++++++++++++++++++++------------------------ 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/zdump.c b/zdump.c index a05b878..669d8fd 100644 --- a/zdump.c +++ b/zdump.c @@ -125,16 +125,24 @@ is_alpha(char a) } } -/* Return A + B, exiting if the result would overflow. */ -static size_t -sumsize(size_t a, size_t b) +static _Noreturn void +size_overflow(void) { - if (SIZE_MAX - a < b) - return a + b; fprintf(stderr, _("%s: size overflow\n"), progname); exit(EXIT_FAILURE); } +/* Return A + B, exiting if the result would overflow either ptrdiff_t + or size_t. */ +static ATTRIBUTE_PURE ptrdiff_t +sumsize(size_t a, size_t b) +{ + ptrdiff_t sum_max = min(PTRDIFF_MAX, SIZE_MAX); + if (a <= sum_max && b <= sum_max - a) + return a + b; + size_overflow(); +} + /* Return a pointer to a newly allocated buffer of size SIZE, exiting on failure. SIZE should be nonzero. */ static void * ATTRIBUTE_MALLOC @@ -237,17 +245,19 @@ tzalloc(char const *val) enum { TZeqlen = 3 }; static char const TZeq[TZeqlen] = "TZ="; static char **fakeenv; - static size_t fakeenv0size; + static ptrdiff_t fakeenv0size; void *freeable = NULL; char **env = fakeenv, **initial_environ; size_t valsize = strlen(val) + 1; if (fakeenv0size < valsize) { char **e = environ, **to; - ptrdiff_t initial_nenvptrs; /* Counting the trailing NULL pointer. */ + ptrdiff_t initial_nenvptrs = 1; /* Counting the trailing NULL pointer. */ - while (*e++) - continue; - initial_nenvptrs = e - environ; + while (*e++) { + if (initial_nenvptrs == min(PTRDIFF_MAX, SIZE_MAX) / sizeof *environ) + size_overflow(); + initial_nenvptrs++; + } fakeenv0size = sumsize(valsize, valsize); fakeenv0size = max(fakeenv0size, 64); freeable = env; @@ -383,7 +393,7 @@ abbrok(const char *const abbrp, const char *const zone) return the abbreviation. Get the abbreviation from TMP. Exit on memory allocation failure. */ static char const * -saveabbr(char **buf, size_t *bufalloc, struct tm const *tmp) +saveabbr(char **buf, ptrdiff_t *bufalloc, struct tm const *tmp) { char const *ab = abbr(tmp); if (HAVE_LOCALTIME_RZ) @@ -440,7 +450,7 @@ main(int argc, char *argv[]) { /* These are static so that they're initially zero. */ static char * abbrev; - static size_t abbrevsize; + static ptrdiff_t abbrevsize; register int i; register bool vflag; @@ -696,7 +706,7 @@ static time_t hunt(timezone_t tz, char *name, time_t lot, time_t hit, bool only_ok) { static char * loab; - static size_t loabsize; + static ptrdiff_t loabsize; struct tm lotm; struct tm tm; @@ -935,7 +945,7 @@ my_snprintf(char *s, size_t size, char const *format, ...) fit, return the length that the string would have been if it had fit; do not overrun the output buffer. */ static int -format_local_time(char *buf, size_t size, struct tm const *tm) +format_local_time(char *buf, ptrdiff_t size, struct tm const *tm) { int ss = tm->tm_sec, mm = tm->tm_min, hh = tm->tm_hour; return (ss @@ -958,7 +968,7 @@ format_local_time(char *buf, size_t size, struct tm const *tm) the length that the string would have been if it had fit; do not overrun the output buffer. */ static int -format_utc_offset(char *buf, size_t size, struct tm const *tm, time_t t) +format_utc_offset(char *buf, ptrdiff_t size, struct tm const *tm, time_t t) { long off = gmtoff(tm, &t, NULL); char sign = ((off < 0 @@ -987,11 +997,11 @@ format_utc_offset(char *buf, size_t size, struct tm const *tm, time_t t) If the representation's length is less than SIZE, return the length; the representation is not null terminated. Otherwise return SIZE, to indicate that BUF is too small. */ -static size_t -format_quoted_string(char *buf, size_t size, char const *p) +static ptrdiff_t +format_quoted_string(char *buf, ptrdiff_t size, char const *p) { char *b = buf; - size_t s = size; + ptrdiff_t s = size; if (!s) return size; *b++ = '"', s--; @@ -1029,11 +1039,11 @@ format_quoted_string(char *buf, size_t size, char const *p) and omit any trailing tabs. */ static bool -istrftime(char *buf, size_t size, char const *time_fmt, +istrftime(char *buf, ptrdiff_t size, char const *time_fmt, struct tm const *tm, time_t t, char const *ab, char const *zone_name) { char *b = buf; - size_t s = size; + ptrdiff_t s = size; char const *f = time_fmt, *p; for (p = f; ; p++) @@ -1042,9 +1052,9 @@ istrftime(char *buf, size_t size, char const *time_fmt, else if (!*p || (*p == '%' && (p[1] == 'f' || p[1] == 'L' || p[1] == 'Q'))) { - size_t formatted_len; - size_t f_prefix_len = p - f; - size_t f_prefix_copy_size = p - f + 2; + ptrdiff_t formatted_len; + ptrdiff_t f_prefix_len = p - f; + ptrdiff_t f_prefix_copy_size = sumsize(f_prefix_len, 2); char fbuf[100]; bool oversized = sizeof fbuf <= f_prefix_copy_size; char *f_prefix_copy = oversized ? xmalloc(f_prefix_copy_size) : fbuf; @@ -1076,7 +1086,7 @@ istrftime(char *buf, size_t size, char const *time_fmt, b += offlen, s -= offlen; if (show_abbr) { char const *abp; - size_t len; + ptrdiff_t len; if (s <= 1) return false; *b++ = '\t', s--; @@ -1115,7 +1125,7 @@ showtrans(char const *time_fmt, struct tm const *tm, time_t t, char const *ab, putchar('\n'); } else { char stackbuf[1000]; - size_t size = sizeof stackbuf; + ptrdiff_t size = sizeof stackbuf; char *buf = stackbuf; char *bufalloc = NULL; while (! istrftime(buf, size, time_fmt, tm, t, ab, zone_name)) { -- 2.38.1
* zic.c (FORMAT_LEN_GROWTH_BOUND): New constant. (infile): Don’t allocate a local buffer so large that later size calculations can overflow. (getsave, inzsub, doabbr, stringzone): Prefer ptrdiff_t to size_t where either will do, as signed overflow is easier for a debugging implementation to check. --- zic.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/zic.c b/zic.c index 752ac48..53351d0 100644 --- a/zic.c +++ b/zic.c @@ -34,6 +34,9 @@ static zic_t const # define ZIC_MAX_ABBR_LEN_WO_WARN 6 #endif /* !defined ZIC_MAX_ABBR_LEN_WO_WARN */ +/* An upper bound on how much a format might grow due to concatenation. */ +enum { FORMAT_LEN_GROWTH_BOUND = 5 }; + #ifdef HAVE_DIRECT_H # include <direct.h> # include <io.h> @@ -1634,7 +1637,10 @@ infile(int fnum, char const *name) } wantcont = false; for (num = 1; ; ++num) { - char buf[_POSIX2_LINE_MAX]; + enum { bufsize_bound + = (min(INT_MAX, min(PTRDIFF_MAX, SIZE_MAX)) + / FORMAT_LEN_GROWTH_BOUND) }; + char buf[min(_POSIX2_LINE_MAX, bufsize_bound)]; int nfields; char *fields[MAX_FIELDS]; eat(fnum, num); @@ -1748,7 +1754,7 @@ getsave(char *field, bool *isdst) { int dst = -1; zic_t save; - size_t fieldlen = strlen(field); + ptrdiff_t fieldlen = strlen(field); if (fieldlen != 0) { char *ep = field + fieldlen - 1; switch (*ep) { @@ -1844,7 +1850,7 @@ inzsub(char **fields, int nfields, bool iscont) register char * cp; char * cp1; struct zone z; - size_t format_len; + int format_len; register int i_stdoff, i_rule, i_format; register int i_untilyear, i_untilmonth; register int i_untilday, i_untiltime; @@ -2747,13 +2753,13 @@ abbroffset(char *buf, zic_t offset) static char const disable_percent_s[] = ""; -static size_t +static ptrdiff_t doabbr(char *abbr, struct zone const *zp, char const *letters, bool isdst, zic_t save, bool doquotes) { register char * cp; register char * slashp; - register size_t len; + ptrdiff_t len; char const *format = zp->z_format; slashp = strchr(format, '/'); @@ -2919,9 +2925,9 @@ stringzone(char *result, struct zone const *zpfirst, ptrdiff_t zonecount) register ptrdiff_t i; register int compat = 0; register int c; - size_t len; int offsetlen; struct rule stdr, dstr; + ptrdiff_t len; int dstcmp; struct rule *lastrp[2] = { NULL, NULL }; struct zone zstr[2]; @@ -3054,8 +3060,10 @@ outzone(const struct zone *zpfirst, ptrdiff_t zonecount) check_for_signal(); + /* This cannot overflow; see FORMAT_LEN_GROWTH_BOUND. */ max_abbr_len = 2 + max_format_len + max_abbrvar_len; max_envvar_len = 2 * max_abbr_len + 5 * 9; + startbuf = emalloc(max_abbr_len + 1); ab = emalloc(max_abbr_len + 1); envvar = emalloc(max_envvar_len + 1); -- 2.38.1
* zic.c (size_overflow, size_sum): New functions. (size_product, align_to, growalloc, relname): Prefer ptrdiff_t to size_t where either will do. (random_dirent, relname): Check for unlikely integer overflow. --- zic.c | 49 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/zic.c b/zic.c index 53351d0..a69d4e0 100644 --- a/zic.c +++ b/zic.c @@ -466,22 +466,35 @@ memory_exhausted(const char *msg) exit(EXIT_FAILURE); } +static _Noreturn void +size_overflow(void) +{ + memory_exhausted(_("size overflow")); +} + static ATTRIBUTE_PURE ptrdiff_t -size_product(ptrdiff_t nitems, size_t itemsize) +size_sum(size_t a, size_t b) +{ + ptrdiff_t sum_max = min(PTRDIFF_MAX, SIZE_MAX); + if (a <= sum_max && b <= sum_max - a) + return a + b; + size_overflow(); +} + +static ATTRIBUTE_PURE ptrdiff_t +size_product(ptrdiff_t nitems, ptrdiff_t itemsize) { ptrdiff_t nitems_max = min(PTRDIFF_MAX, SIZE_MAX) / itemsize; if (nitems <= nitems_max) return nitems * itemsize; - memory_exhausted(_("size overflow")); + size_overflow(); } -static ATTRIBUTE_PURE size_t -align_to(size_t size, size_t alignment) +static ATTRIBUTE_PURE ptrdiff_t +align_to(ptrdiff_t size, ptrdiff_t alignment) { - size_t lo_bits = alignment - 1; - if (size <= SIZE_MAX - lo_bits) - return size + (-size & lo_bits); - memory_exhausted(_("alignment overflow")); + ptrdiff_t lo_bits = alignment - 1, sum = size_sum(size, lo_bits); + return sum & ~lo_bits; } #if !HAVE_STRDUP @@ -532,7 +545,8 @@ grow_nitems_alloc(ptrdiff_t *nitems_alloc, ptrdiff_t itemsize) } static void * -growalloc(void *ptr, size_t itemsize, ptrdiff_t nitems, ptrdiff_t *nitems_alloc) +growalloc(void *ptr, ptrdiff_t itemsize, ptrdiff_t nitems, + ptrdiff_t *nitems_alloc) { return (nitems < *nitems_alloc ? ptr @@ -1284,7 +1298,7 @@ random_dirent(char const **name, char **namealloc) uint_fast64_t unfair_min = - ((UINTMAX_MAX % base__6 + 1) % base__6); if (!dst) { - dst = emalloc(dirlen + prefixlen + suffixlen + 1); + dst = emalloc(size_sum(dirlen, prefixlen + suffixlen + 1)); memcpy(dst, src, dirlen); memcpy(dst + dirlen, prefix, prefixlen); dst[dirlen + prefixlen + suffixlen] = '\0'; @@ -1363,19 +1377,20 @@ rename_dest(char *tempname, char const *name) static char * relname(char const *target, char const *linkname) { - size_t i, taillen, dotdotetcsize; - size_t dir_len = 0, dotdots = 0, linksize = SIZE_MAX; + size_t i, taillen, dir_len = 0, dotdots = 0; + ptrdiff_t dotdotetcsize, linksize = min(PTRDIFF_MAX, SIZE_MAX); char const *f = target; char *result = NULL; if (*linkname == '/') { /* Make F absolute too. */ size_t len = strlen(directory); - bool needslash = len && directory[len - 1] != '/'; - linksize = len + needslash + strlen(target) + 1; + size_t lenslash = len + (len && directory[len - 1] != '/'); + size_t targetsize = strlen(target) + 1; + linksize = size_sum(lenslash, targetsize); f = result = emalloc(linksize); - strcpy(result, directory); + memcpy(result, directory, len); result[len] = '/'; - strcpy(result + len + needslash, target); + memcpy(result + lenslash, target, targetsize); } for (i = 0; f[i] && f[i] == linkname[i]; i++) if (f[i] == '/') @@ -1383,7 +1398,7 @@ relname(char const *target, char const *linkname) for (; linkname[i]; i++) dotdots += linkname[i] == '/' && linkname[i - 1] != '/'; taillen = strlen(f + dir_len); - dotdotetcsize = 3 * dotdots + taillen + 1; + dotdotetcsize = size_sum(size_product(dotdots, 3), taillen + 1); if (dotdotetcsize <= linksize) { if (!result) result = emalloc(dotdotetcsize); -- 2.38.1
participants (1)
-
Paul Eggert