[PATCH] zic: don't use zone FORMAT field as a printf format string
In doabbr(), the zone line's FORMAT field (zp->z_format) was passed directly as the format argument of sprintf(): sprintf(abbr, format, letters); FORMAT comes from zone data, which is not always trusted: zic is also used to compile third-party or user-supplied zone description files. Passing such data as a printf format string is the classic uncontrolled-format-string anti-pattern (CWE-134 / CERT FIO30-C), and it is what makes this line the sole -Wformat-nonliteral warning in doabbr under -Wformat=2. inzsub() already restricts a slash-less FORMAT to at most one '%s' conversion (a '%z' is rewritten to '%s' earlier), so expand that lone conversion explicitly with mempcpy()/strcpy() instead of handing the data to sprintf(). This removes the format-string sink while keeping the output byte-for-byte identical for every valid FORMAT, so the generated TZif files are unchanged. * zic.c (doabbr): Expand FORMAT's single '%s' by hand rather than using FORMAT as a printf format string. --- diff --git a/zic.c b/zic.c index af2bb80..7cc48ef 100644 --- a/zic.c +++ b/zic.c @@ -3135,7 +3135,15 @@ doabbr(char *abbr, struct zone const *zp, char const *letters, letters = "%s"; else if (letters == disable_percent_s) return 0; - sprintf(abbr, format, letters); + /* Expand FORMAT's lone "%s" (see inzsub) without treating + FORMAT, which comes from zone data, as a printf format string. */ + cp = strchr(format, '%'); + if (cp) { + char *dp = mempcpy(abbr, format, cp - format); + dp = mempcpy(dp, letters, strlen(letters)); + strcpy(dp, cp + 2); + } else + strcpy(abbr, format); } else if (isdst) strcpy(abbr, slashp + 1); else { -- 2.50.1 (Apple Git-155)
On 2026-07-02 01:25, Syed Abdul Khaliq via tz wrote:
- sprintf(abbr, format, letters); + /* Expand FORMAT's lone "%s" (see inzsub) without treating + FORMAT, which comes from zone data, as a printf format string. */ + cp = strchr(format, '%'); + if (cp) { + char *dp = mempcpy(abbr, format, cp - format); + dp = mempcpy(dp, letters, strlen(letters)); + strcpy(dp, cp + 2); + } else + strcpy(abbr, format);
This doesn't fix any bugs, right? It merely pacifies 'gcc -Wformat=2 -Wformat-nonliteral'. That is, as I understand it, no matter what input the user supplies, the format contains at most one '%' byte and it's immediately followed by an 's' byte. Even if we have a multi-byte encoding in which a trailing byte of a character contains '%' (and although allowed by ISO C, I don't know of any real-world encodings that do that), as far as I can see no format can cause sprintf to overflow the output buffer. If so, I'm puzzled as to why the patch is useful. By my count there are eight other false positives in tzcode, i.e., places where 'gcc -Wformat=2 -Wformat-nonliteral' complains. This suggests that it's better to not use -Wformat-nonliteral. Complicating the code (as in the above) seems like a net loss by comparison.
This doesn't fix any bugs, right? It merely pacifies 'gcc -Wformat=2 -Wformat-nonliteral'.
Correct, it fixes no bug. I confirmed your reasoning against the code: inzsub() rejects any FORMAT whose '%' isn't followed by 's' or 'z', with no second '%', and the '%z' case is rewritten to '%s' before doabbr() runs. So format always holds at most one '%s' and letters is a plain string; sprintf can't overflow abbr. It's a false positive, as you say.
By my count there are eight other false positives in tzcode [...] This suggests that it's better to not use -Wformat-nonliteral.
Agreed — given that ratio, dropping -Wformat-nonliteral is the better response than open-coding the expansion here, and it keeps this line readable. Please drop the patch; sorry for the noise.
participants (2)
-
Paul Eggert -
Syed Abdul Khaliq