[PATCH 1/3] Prefer mempcpy to doing it by hand
This is less error-prone. Supply a mempcpy if the system lacks it. * Makefile, NEWS: Mention this. * localtime.c (tzloadbody, tzparse): * zic.c (random_dirent, relname, doabbr): * zdump.c (my_snprintf) [!HAVE_SNPRINTF]: (istrftime): Prefer mempcpy to doing it by hand. * private.h (HAVE_MEMPCPY): New macro. (mempcpy) [!HAVE_MEMPCPY]: New function. --- Makefile | 1 + NEWS | 3 +++ localtime.c | 13 +++++++------ private.h | 17 +++++++++++++++++ zdump.c | 11 +++++++---- zic.c | 36 ++++++++++++++++++++---------------- 6 files changed, 55 insertions(+), 26 deletions(-) diff --git a/Makefile b/Makefile index 83122ecc..5b577781 100644 --- a/Makefile +++ b/Makefile @@ -262,6 +262,7 @@ LDLIBS= # -DHAVE_LOCALTIME_RZ=0 if you do not want zdump to use localtime_rz # localtime_rz can make zdump significantly faster, but is nonstandard. # -DHAVE_MALLOC_ERRNO=0 if malloc etc. do not set errno on failure. +# -DHAVE_MEMPCPY=1 if your system has mempcpy, 0 if not (default is guessed) # -DHAVE_POSIX_DECLS=0 if your system's include files do not declare # functions like 'link' or variables like 'tzname' required by POSIX # -DHAVE_SETENV=0 if your system lacks the setenv function diff --git a/NEWS b/NEWS index 5c4786cb..0436c615 100644 --- a/NEWS +++ b/NEWS @@ -51,6 +51,9 @@ Unreleased, experimental changes -DHAVE_GETRESUID=[01], and -DHAVE_GETEUID=[01] to enable or disable these system calls' use. + tzcode now uses mempcpy if available, guessing its availability. + Compile with -DHAVE_MEMPCPY=1 or 0 to override the guess. + tzcode now uses strnlen to improve asymptotic performance a bit. Compile with -DHAVE_STRNLEN=0 if your platform lacks it. diff --git a/localtime.c b/localtime.c index 67500312..316c7504 100644 --- a/localtime.c +++ b/localtime.c @@ -624,6 +624,7 @@ tzloadbody(char const *name, struct state *sp, char tzloadflags, } if (!SUPPRESS_TZDIR && name[0] != '/') { + char *cp; if (sizeof lsp->fullname - sizeof tzdirslash <= strnlen(name, sizeof lsp->fullname - sizeof tzdirslash)) return ENAMETOOLONG; @@ -631,8 +632,9 @@ tzloadbody(char const *name, struct state *sp, char tzloadflags, /* Create a string "TZDIR/NAME". Using sprintf here would pull in stdio (and would fail if the resulting string length exceeded INT_MAX!). */ - memcpy(lsp->fullname, tzdirslash, sizeof tzdirslash); - strcpy(lsp->fullname + sizeof tzdirslash, name); + cp = lsp->fullname; + cp = mempcpy(cp, tzdirslash, sizeof tzdirslash); + strcpy(cp, name); name = lsp->fullname; } @@ -1471,12 +1473,11 @@ tzparse(const char *name, struct state *sp, struct state const *basep) } sp->charcnt = charcnt; cp = sp->chars; - memcpy(cp, stdname, stdlen); - cp += stdlen; + cp = mempcpy(cp, stdname, stdlen); *cp++ = '\0'; if (dstlen != 0) { - memcpy(cp, dstname, dstlen); - *(cp + dstlen) = '\0'; + cp = mempcpy(cp, dstname, dstlen); + *cp = '\0'; } return true; } diff --git a/private.h b/private.h index 6ae0c2fd..867340fa 100644 --- a/private.h +++ b/private.h @@ -837,6 +837,23 @@ extern char *asctime_r(struct tm const *restrict, char *restrict); extern char **environ; #endif +#ifndef HAVE_MEMPCPY +# if (defined mempcpy \ + || defined __FreeBSD__ || defined __NetBSD__ || defined __linux__) +# define HAVE_MEMPCPY 1 +# else +# define HAVE_MEMPCPY 0 +# endif +#endif +#if !HAVE_MEMPCPY +static void * +mempcpy(char *restrict s1, char const *restrict s2, size_t n) +{ + char *p = memcpy(s1, s2, n); + return p + n; +} +#endif + #if 2 <= HAVE_TZNAME + (TZ_TIME_T || !HAVE_POSIX_DECLS) extern char *tzname[]; #endif diff --git a/zdump.c b/zdump.c index 8e836e60..93fe05cb 100644 --- a/zdump.c +++ b/zdump.c @@ -927,6 +927,7 @@ my_snprintf(char *s, size_t size, char const *format, ...) int n; va_list args; char const *arg; + char *cp; size_t arglen, slen; char buf[1024]; va_start(args, format); @@ -943,8 +944,9 @@ my_snprintf(char *s, size_t size, char const *format, ...) arglen = n; } slen = arglen < size ? arglen : size - 1; - memcpy(s, arg, slen); - s[slen] = '\0'; + cp = s; + cp = mempcpy(cp, arg, slen); + *cp = '\0'; n = arglen <= INT_MAX ? arglen : -1; va_end(args); return n; @@ -1073,8 +1075,9 @@ istrftime(char *buf, ptrdiff_t size, char const *time_fmt, char fbuf[100]; bool oversized = sizeof fbuf <= f_prefix_copy_size; char *f_prefix_copy = oversized ? xmalloc(f_prefix_copy_size) : fbuf; - memcpy(f_prefix_copy, f, f_prefix_len); - strcpy(f_prefix_copy + f_prefix_len, "X"); + char *cp = f_prefix_copy; + cp = mempcpy(cp, f, f_prefix_len); + strcpy(cp, "X"); formatted_len = strftime(b, s, f_prefix_copy, tm); if (oversized) free(f_prefix_copy); diff --git a/zic.c b/zic.c index 6bdb039d..e78dfd47 100644 --- a/zic.c +++ b/zic.c @@ -1327,10 +1327,10 @@ random_dirent(char const **name, char **namealloc) uint_fast64_t unfair_min = - ((UINTMAX_MAX % base__6 + 1) % base__6); if (!dst) { - dst = xmalloc(size_sum(dirlen, prefixlen + suffixlen + 1)); - memcpy(dst, src, dirlen); - memcpy(dst + dirlen, prefix, prefixlen); - dst[dirlen + prefixlen + suffixlen] = '\0'; + char *cp = dst = xmalloc(size_sum(dirlen, prefixlen + suffixlen + 1)); + cp = mempcpy(cp, src, dirlen); + cp = mempcpy(cp, prefix, prefixlen); + cp[suffixlen] = '\0'; *name = *namealloc = dst; } @@ -1430,15 +1430,17 @@ relname(char const *target, char const *linkname) if (*linkname == '/') { /* Make F absolute too. */ size_t len = strlen(directory); - size_t lenslash = len + (len && directory[len - 1] != '/'); + bool needs_slash = len && directory[len - 1] != '/'; + size_t lenslash = len + needs_slash; size_t targetsize = strlen(target) + 1; + char *cp; if (*directory != '/') return NULL; linksize = size_sum(lenslash, targetsize); - f = result = xmalloc(linksize); - memcpy(result, directory, len); - result[len] = '/'; - memcpy(result + lenslash, target, targetsize); + f = cp = result = xmalloc(linksize); + cp = mempcpy(cp, directory, len); + *cp = '/'; + memcpy(cp + needs_slash, target, targetsize); } for (i = 0; f[i] && f[i] == linkname[i]; i++) if (f[i] == '/') @@ -1448,11 +1450,13 @@ relname(char const *target, char const *linkname) taillen = strlen(f + dir_len); dotdotetcsize = size_sum(size_product(dotdots, 3), taillen + 1); if (dotdotetcsize <= linksize) { + char *cp; if (!result) result = xmalloc(dotdotetcsize); + cp = result; for (i = 0; i < dotdots; i++) - memcpy(result + 3 * i, "../", 3); - memmove(result + 3 * dotdots, f + dir_len, taillen + 1); + cp = mempcpy(cp, "../", 3); + memmove(cp, f + dir_len, taillen + 1); } return result; } @@ -2875,11 +2879,11 @@ doabbr(char *abbr, struct zone const *zp, char const *letters, else if (letters == disable_percent_s) return 0; sprintf(abbr, format, letters); - } else if (isdst) { - strcpy(abbr, slashp + 1); - } else { - memcpy(abbr, format, slashp - format); - abbr[slashp - format] = '\0'; + } else if (isdst) + strcpy(abbr, slashp + 1); + else { + char *abbrend = mempcpy(abbr, format, slashp - format); + *abbrend = '\0'; } len = strlen(abbr); if (!doquotes) -- 2.48.1
* localtime.c (tzloadbody, tzset_unlocked): When the source string is not known to be constant, prefer mempcpy (and terminating the destination by hand instead of trusting the source) to strcpy. This avoids some undefined behavior if another thread modifies the data while we access it. With typical optimization the resulting machine code will not call strcpy because all uses of strcpy are optimized away. --- localtime.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/localtime.c b/localtime.c index 316c7504..a7a55f48 100644 --- a/localtime.c +++ b/localtime.c @@ -625,8 +625,9 @@ tzloadbody(char const *name, struct state *sp, char tzloadflags, if (!SUPPRESS_TZDIR && name[0] != '/') { char *cp; - if (sizeof lsp->fullname - sizeof tzdirslash - <= strnlen(name, sizeof lsp->fullname - sizeof tzdirslash)) + size_t namesizemax = sizeof lsp->fullname - sizeof tzdirslash; + size_t namelen = strnlen (name, namesizemax); + if (namesizemax <= namelen) return ENAMETOOLONG; /* Create a string "TZDIR/NAME". Using sprintf here @@ -634,7 +635,8 @@ tzloadbody(char const *name, struct state *sp, char tzloadflags, resulting string length exceeded INT_MAX!). */ cp = lsp->fullname; cp = mempcpy(cp, tzdirslash, sizeof tzdirslash); - strcpy(cp, name); + cp = mempcpy(cp, name, namelen); + *cp = '\0'; name = lsp->fullname; } @@ -846,7 +848,9 @@ tzloadbody(char const *name, struct state *sp, char tzloadflags, if (! (j < charcnt)) { int tsabbrlen = strnlen(tsabbr, TZ_MAX_CHARS - j); if (j + tsabbrlen < TZ_MAX_CHARS) { - strcpy(sp->chars + j, tsabbr); + char *cp = sp->chars + j; + cp = mempcpy(cp, tsabbr, tsabbrlen); + *cp = '\0'; charcnt = j + tsabbrlen + 1; ts->ttis[i].tt_desigidx = j; gotabbr++; @@ -1563,8 +1567,11 @@ tzset_unlocked(void) if (name || err != ENOENT) strcpy(sp->chars, UNSPEC); } - if (namelen < sizeof lcl_TZname) - memcpy(lcl_TZname, name, namelen + 1); + if (namelen < sizeof lcl_TZname) { + char *cp = lcl_TZname; + cp = mempcpy(cp, name, namelen); + *cp = '\0'; + } } settzname(); lcl_is_set = (sizeof lcl_TZname > namelen) - (sizeof lcl_TZname < namelen); -- 2.48.1
It likely hurt performance for typical cases and didn’t add enough clarity to justify its use. * localtime.c (TZ_ABBR_CHAR_SET, TZ_ABBR_ERR_CHAR): Remove. All uses removed. (scrub_abbrs, tzloadbody): Open-code rather than using strchr. --- localtime.c | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/localtime.c b/localtime.c index a7a55f48..c5bc4e93 100644 --- a/localtime.c +++ b/localtime.c @@ -108,15 +108,6 @@ typedef intmax_t timex_t; # endif #endif -#ifndef TZ_ABBR_CHAR_SET -# define TZ_ABBR_CHAR_SET \ - "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789 :+-._" -#endif /* !defined TZ_ABBR_CHAR_SET */ - -#ifndef TZ_ABBR_ERR_CHAR -# define TZ_ABBR_ERR_CHAR '_' -#endif /* !defined TZ_ABBR_ERR_CHAR */ - /* Port to platforms that lack some O_* flags. Unless otherwise specified, the flags are standardized by POSIX. */ @@ -511,8 +502,27 @@ scrub_abbrs(struct state *sp) /* Replace bogus characters. */ for (i = 0; i < sp->charcnt; ++i) - if (strchr(TZ_ABBR_CHAR_SET, sp->chars[i]) == NULL) - sp->chars[i] = TZ_ABBR_ERR_CHAR; + switch (sp->chars[i]) { + case '\0': + case ' ': + case '+': case '-': case '.': + case '0': case '1': case '2': case '3': case '4': + case '5': case '6': case '7': case '8': case '9': + case ':': + case 'A': case 'B': case 'C': case 'D': case 'E': case 'F': case 'G': + case 'H': case 'I': case 'J': case 'K': case 'L': case 'M': case 'N': + case 'O': case 'P': case 'Q': case 'R': case 'S': case 'T': case 'U': + case 'V': case 'W': case 'X': case 'Y': case 'Z': + case 'a': case 'b': case 'c': case 'd': case 'e': case 'f': case 'g': + case 'h': case 'i': case 'j': case 'k': case 'l': case 'm': case 'n': + case 'o': case 'p': case 'q': case 'r': case 's': case 't': case 'u': + case 'v': case 'w': case 'x': case 'y': case 'z': + break; + + default: + sp->chars[i] = '_'; + break; + } return 0; } @@ -613,14 +623,11 @@ tzloadbody(char const *name, struct state *sp, char tzloadflags, as such a name could read a file outside TZDIR. */ if (relname[0] != '/') { char const *component; - for (component = relname; ; component++) { + for (component = relname; component[0]; component++) if (component[0] == '.' && component[1] == '.' - && ((component[2] == '/') | !component[2])) + && ((component[2] == '/') | !component[2]) + && (component == relname || component[-1] == '/')) return ENOTCAPABLE; - component = strchr(component, '/'); - if (!component) - break; - } } if (!SUPPRESS_TZDIR && name[0] != '/') { -- 2.48.1
participants (1)
-
Paul Eggert