From cbfba7a0e954fe0f2c3602594fa609be21a35833 Mon Sep 17 00:00:00 2001
From: Simon Tatham <anakin@pobox.com>
Date: Sun, 26 Jan 2020 14:49:31 +0000
Subject: [PATCH] Greatly improve printf format-string checking.

I've added the gcc-style attribute("printf") to a lot of printf-shaped
functions in this code base that didn't have it. To make that easier,
I moved the wrapping macro into defs.h, and also enabled it if we
detect the __clang__ macro as well as __GNU__ (hence, it will be used
when building for Windows using clang-cl).

The result is that a great many format strings in the code are now
checked by the compiler, where they were previously not. This causes
build failures, which I'll fix in the next commit.
---
 cmdgen.c    |  4 ++--
 defs.h      | 20 ++++++++++++++++++++
 logging.c   |  2 +-
 misc.h      | 26 +++-----------------------
 network.h   |  3 ++-
 pageant.c   |  5 +++--
 pageant.h   |  4 ++--
 pscp.c      |  6 +++---
 putty.h     | 13 +++++++------
 scpserver.c |  9 ++++++---
 ssh.h       | 12 ++++++------
 sshshare.c  |  8 ++++----
 testcrypt.c |  2 +-
 testsc.c    |  2 +-
 tree234.c   |  2 +-
 15 files changed, 62 insertions(+), 56 deletions(-)

diff --git a/cmdgen.c b/cmdgen.c
index 286c12e9..dbac992c 100644
--- a/cmdgen.c
+++ b/cmdgen.c
@@ -1205,7 +1205,7 @@ void test(int retval, ...)
     sfree(argv);
 }
 
-void filecmp(char *file1, char *file2, char *fmt, ...)
+PRINTF_LIKE(3, 4) void filecmp(char *file1, char *file2, char *fmt, ...)
 {
     /*
      * Ideally I should do file comparison myself, to maximise the
@@ -1303,7 +1303,7 @@ char *get_fp(char *filename, unsigned flags)
     return toret;
 }
 
-void check_fp(char *filename, char *fp, char *fmt, ...)
+PRINTF_LIKE(3, 4) void check_fp(char *filename, char *fp, char *fmt, ...)
 {
     char *newfp;
 
diff --git a/defs.h b/defs.h
index afeb5062..0a4f08c8 100644
--- a/defs.h
+++ b/defs.h
@@ -27,6 +27,26 @@ uintmax_t strtoumax(const char *nptr, char **endptr, int base);
 #include <inttypes.h>
 #endif
 
+#if defined __GNUC__ || defined __clang__
+/*
+ * On MinGW, the correct compiler format checking for vsnprintf() etc
+ * can depend on compile-time flags; these control whether you get
+ * ISO C or Microsoft's non-standard format strings.
+ * We sometimes use __attribute__ ((format)) for our own printf-like
+ * functions, which are ultimately interpreted by the toolchain-chosen
+ * printf, so we need to take that into account to get correct warnings.
+ */
+#ifdef __MINGW_PRINTF_FORMAT
+#define PRINTF_LIKE(fmt_index, ellipsis_index) \
+    __attribute__ ((format (__MINGW_PRINTF_FORMAT, fmt_index, ellipsis_index)))
+#else
+#define PRINTF_LIKE(fmt_index, ellipsis_index) \
+    __attribute__ ((format (printf, fmt_index, ellipsis_index)))
+#endif
+#else /* __GNUC__ */
+#define PRINTF_LIKE(fmt_index, ellipsis_index)
+#endif /* __GNUC__ */
+
 typedef struct conf_tag Conf;
 typedef struct terminal_tag Terminal;
 typedef struct term_utf8_decode term_utf8_decode;
diff --git a/logging.c b/logging.c
index 173ed080..e324c77c 100644
--- a/logging.c
+++ b/logging.c
@@ -58,7 +58,7 @@ static void logwrite(LogContext *ctx, ptrlen data)
  * Convenience wrapper on logwrite() which printf-formats the
  * string.
  */
-static void logprintf(LogContext *ctx, const char *fmt, ...)
+static PRINTF_LIKE(2, 3) void logprintf(LogContext *ctx, const char *fmt, ...)
 {
     va_list ap;
     char *data;
diff --git a/misc.h b/misc.h
index 51155a21..eff7e08c 100644
--- a/misc.h
+++ b/misc.h
@@ -24,30 +24,10 @@ char *host_strchr(const char *s, int c);
 char *host_strrchr(const char *s, int c);
 char *host_strduptrim(const char *s);
 
-#ifdef __GNUC__
-/*
- * On MinGW, the correct compiler format checking for vsnprintf() etc
- * can depend on compile-time flags; these control whether you get
- * ISO C or Microsoft's non-standard format strings.
- * We sometimes use __attribute__ ((format)) for our own printf-like
- * functions, which are ultimately interpreted by the toolchain-chosen
- * printf, so we need to take that into account to get correct warnings.
- */
-#ifdef __MINGW_PRINTF_FORMAT
-#define PUTTY_PRINTF_ARCHETYPE __MINGW_PRINTF_FORMAT
-#else
-#define PUTTY_PRINTF_ARCHETYPE printf
-#endif
-#endif /* __GNUC__ */
-
 char *dupstr(const char *s);
 char *dupcat_fn(const char *s1, ...);
 #define dupcat(...) dupcat_fn(__VA_ARGS__, (const char *)NULL)
-char *dupprintf(const char *fmt, ...)
-#ifdef __GNUC__
-    __attribute__ ((format (PUTTY_PRINTF_ARCHETYPE, 1, 2)))
-#endif
-    ;
+char *dupprintf(const char *fmt, ...) PRINTF_LIKE(1, 2);
 char *dupvprintf(const char *fmt, va_list ap);
 void burnstr(char *string);
 
@@ -76,7 +56,7 @@ void *strbuf_append(strbuf *buf, size_t len);
 void strbuf_shrink_to(strbuf *buf, size_t new_len);
 void strbuf_shrink_by(strbuf *buf, size_t amount_to_remove);
 char *strbuf_to_str(strbuf *buf); /* does free buf, but you must free result */
-void strbuf_catf(strbuf *buf, const char *fmt, ...);
+void strbuf_catf(strbuf *buf, const char *fmt, ...) PRINTF_LIKE(2, 3);
 void strbuf_catfv(strbuf *buf, const char *fmt, va_list ap);
 static inline void strbuf_clear(strbuf *buf) { strbuf_shrink_to(buf, 0); }
 bool strbuf_chomp(strbuf *buf, char char_to_remove);
@@ -267,7 +247,7 @@ static inline NORETURN void unreachable_internal(void) { abort(); }
  */
 
 #ifdef DEBUG
-void debug_printf(const char *fmt, ...);
+void debug_printf(const char *fmt, ...) PRINTF_LIKE(1, 2);
 void debug_memdump(const void *buf, int len, bool L);
 #define debug(...) (debug_printf(__VA_ARGS__))
 #define dmemdump(buf,len) (debug_memdump(buf, len, false))
diff --git a/network.h b/network.h
index 2cd91325..9f3c2d9a 100644
--- a/network.h
+++ b/network.h
@@ -268,7 +268,8 @@ char *get_hostname(void);
  * The consume_string variant takes an already-formatted dynamically
  * allocated string, and takes over ownership of that string.
  */
-Socket *new_error_socket_fmt(Plug *plug, const char *fmt, ...);
+Socket *new_error_socket_fmt(Plug *plug, const char *fmt, ...)
+    PRINTF_LIKE(2, 3);
 Socket *new_error_socket_consume_string(Plug *plug, char *errmsg);
 
 /*
diff --git a/pageant.c b/pageant.c
index 766f92ef..b43ef33e 100644
--- a/pageant.c
+++ b/pageant.c
@@ -308,8 +308,9 @@ void pageant_unregister_client(PageantClient *pc)
     sfree(pc->info);
 }
 
-static void failure(PageantClient *pc, PageantClientRequestId *reqid,
-                    strbuf *sb, const char *fmt, ...)
+static PRINTF_LIKE(4, 5) void failure(PageantClient *pc,
+                                      PageantClientRequestId *reqid,
+                                      strbuf *sb, const char *fmt, ...)
 {
     strbuf_clear(sb);
     put_byte(sb, SSH_AGENT_FAILURE);
diff --git a/pageant.h b/pageant.h
index 2bf8c5f6..e80ef44d 100644
--- a/pageant.h
+++ b/pageant.h
@@ -41,7 +41,7 @@ static inline void pageant_client_log_v(
     if (!pc->suppress_logging)
         pc->vt->log(pc, reqid, fmt, ap);
 }
-static inline void pageant_client_log(
+static inline PRINTF_LIKE(3, 4) void pageant_client_log(
     PageantClient *pc, PageantClientRequestId *reqid, const char *fmt, ...)
 {
     if (!pc->suppress_logging) {
@@ -153,7 +153,7 @@ static inline void pageant_listener_client_log_v(
     if (!plc->suppress_logging)
         plc->vt->log(plc, fmt, ap);
 }
-static inline void pageant_listener_client_log(
+static inline PRINTF_LIKE(2, 3) void pageant_listener_client_log(
     PageantListenerClient *plc, const char *fmt, ...)
 {
     if (!plc->suppress_logging) {
diff --git a/pscp.c b/pscp.c
index ef65a591..5f69e4c3 100644
--- a/pscp.c
+++ b/pscp.c
@@ -113,7 +113,7 @@ static void abandon_stats(void)
     }
 }
 
-static void tell_user(FILE *stream, const char *fmt, ...)
+static PRINTF_LIKE(2, 3) void tell_user(FILE *stream, const char *fmt, ...)
 {
     char *str, *str2;
     va_list ap;
@@ -224,7 +224,7 @@ static void ssh_scp_init(void)
 /*
  *  Print an error message and exit after closing the SSH link.
  */
-static NORETURN void bump(const char *fmt, ...)
+static NORETURN PRINTF_LIKE(1, 2) void bump(const char *fmt, ...)
 {
     char *str, *str2;
     va_list ap;
@@ -1541,7 +1541,7 @@ int scp_finish_filerecv(void)
  *  Send an error message to the other side and to the screen.
  *  Increment error counter.
  */
-static void run_err(const char *fmt, ...)
+static PRINTF_LIKE(1, 2) void run_err(const char *fmt, ...)
 {
     char *str, *str2;
     va_list ap;
diff --git a/putty.h b/putty.h
index 0c8337d8..f1769d15 100644
--- a/putty.h
+++ b/putty.h
@@ -1003,7 +1003,7 @@ static inline bool seat_set_trust_status(Seat *seat, bool trusted)
 /* Unlike the seat's actual method, the public entry point
  * seat_connection_fatal is a wrapper function with a printf-like API,
  * defined in misc.c. */
-void seat_connection_fatal(Seat *seat, const char *fmt, ...);
+void seat_connection_fatal(Seat *seat, const char *fmt, ...) PRINTF_LIKE(2, 3);
 
 /* Handy aliases for seat_output which set is_stderr to a fixed value. */
 static inline size_t seat_stdout(Seat *seat, const void *data, size_t len)
@@ -1218,8 +1218,8 @@ static inline bool win_is_utf8(TermWin *win)
 /*
  * Global functions not specific to a connection instance.
  */
-void nonfatal(const char *, ...);
-NORETURN void modalfatalbox(const char *, ...);
+void nonfatal(const char *, ...) PRINTF_LIKE(1, 2);
+NORETURN void modalfatalbox(const char *, ...) PRINTF_LIKE(1, 2);
 NORETURN void cleanup_exit(int);
 
 /*
@@ -1703,7 +1703,7 @@ void logfclose(LogContext *logctx);
 void logtraffic(LogContext *logctx, unsigned char c, int logmode);
 void logflush(LogContext *logctx);
 void logevent(LogContext *logctx, const char *event);
-void logeventf(LogContext *logctx, const char *fmt, ...);
+void logeventf(LogContext *logctx, const char *fmt, ...) PRINTF_LIKE(2, 3);
 void logeventvf(LogContext *logctx, const char *fmt, va_list ap);
 
 /*
@@ -1927,7 +1927,8 @@ bool is_interactive(void);
 void console_print_error_msg(const char *prefix, const char *msg);
 void console_print_error_msg_fmt_v(
     const char *prefix, const char *fmt, va_list ap);
-void console_print_error_msg_fmt(const char *prefix, const char *fmt, ...);
+void console_print_error_msg_fmt(const char *prefix, const char *fmt, ...)
+    PRINTF_LIKE(2, 3);
 
 /*
  * Exports from printing.c.
@@ -1965,7 +1966,7 @@ bool cmdline_host_ok(Conf *);
 #define TOOLTYPE_PORT_ARG 64
 extern int cmdline_tooltype;
 
-void cmdline_error(const char *, ...);
+void cmdline_error(const char *, ...) PRINTF_LIKE(1, 2);
 
 /*
  * Exports from config.c.
diff --git a/scpserver.c b/scpserver.c
index e594f1ab..3a136286 100644
--- a/scpserver.c
+++ b/scpserver.c
@@ -431,7 +431,8 @@ static char *scp_source_err_base(ScpSource *scp, const char *fmt, va_list ap)
     sshfwd_write_ext(scp->sc, true, "\012", 1);
     return msg;
 }
-static void scp_source_err(ScpSource *scp, const char *fmt, ...)
+static PRINTF_LIKE(2, 3) void scp_source_err(
+    ScpSource *scp, const char *fmt, ...)
 {
     va_list ap;
 
@@ -439,7 +440,8 @@ static void scp_source_err(ScpSource *scp, const char *fmt, ...)
     sfree(scp_source_err_base(scp, fmt, ap));
     va_end(ap);
 }
-static void scp_source_abort(ScpSource *scp, const char *fmt, ...)
+static PRINTF_LIKE(2, 3) void scp_source_abort(
+    ScpSource *scp, const char *fmt, ...)
 {
     va_list ap;
     char *msg;
@@ -1312,7 +1314,8 @@ static void scp_error_send_message_cb(void *vscp)
     sshfwd_initiate_close(scp->sc, scp->message);
 }
 
-static ScpError *scp_error_new(SshChannel *sc, const char *fmt, ...)
+static PRINTF_LIKE(2, 3) ScpError *scp_error_new(
+    SshChannel *sc, const char *fmt, ...)
 {
     va_list ap;
     ScpError *scp = snew(ScpError);
diff --git a/ssh.h b/ssh.h
index 61f96267..db189892 100644
--- a/ssh.h
+++ b/ssh.h
@@ -406,12 +406,12 @@ void ssh_conn_processed_data(Ssh *ssh);
 void ssh_check_frozen(Ssh *ssh);
 
 /* Functions to abort the connection, for various reasons. */
-void ssh_remote_error(Ssh *ssh, const char *fmt, ...);
-void ssh_remote_eof(Ssh *ssh, const char *fmt, ...);
-void ssh_proto_error(Ssh *ssh, const char *fmt, ...);
-void ssh_sw_abort(Ssh *ssh, const char *fmt, ...);
-void ssh_sw_abort_deferred(Ssh *ssh, const char *fmt, ...);
-void ssh_user_close(Ssh *ssh, const char *fmt, ...);
+void ssh_remote_error(Ssh *ssh, const char *fmt, ...) PRINTF_LIKE(2, 3);
+void ssh_remote_eof(Ssh *ssh, const char *fmt, ...) PRINTF_LIKE(2, 3);
+void ssh_proto_error(Ssh *ssh, const char *fmt, ...) PRINTF_LIKE(2, 3);
+void ssh_sw_abort(Ssh *ssh, const char *fmt, ...) PRINTF_LIKE(2, 3);
+void ssh_sw_abort_deferred(Ssh *ssh, const char *fmt, ...) PRINTF_LIKE(2, 3);
+void ssh_user_close(Ssh *ssh, const char *fmt, ...) PRINTF_LIKE(2, 3);
 
 /* Bit positions in the SSH-1 cipher protocol word */
 #define SSH1_CIPHER_IDEA        1
diff --git a/sshshare.c b/sshshare.c
index 24f4c125..98071d8e 100644
--- a/sshshare.c
+++ b/sshshare.c
@@ -705,8 +705,8 @@ static void share_remove_forwarding(struct ssh_sharing_connstate *cs,
     sfree(fwd);
 }
 
-static void log_downstream(struct ssh_sharing_connstate *cs,
-                           const char *logfmt, ...)
+static PRINTF_LIKE(2, 3) void log_downstream(struct ssh_sharing_connstate *cs,
+                                             const char *logfmt, ...)
 {
     va_list ap;
     char *buf;
@@ -719,8 +719,8 @@ static void log_downstream(struct ssh_sharing_connstate *cs,
     sfree(buf);
 }
 
-static void log_general(struct ssh_sharing_state *sharestate,
-                        const char *logfmt, ...)
+static PRINTF_LIKE(2, 3) void log_general(struct ssh_sharing_state *sharestate,
+                                          const char *logfmt, ...)
 {
     va_list ap;
     char *buf;
diff --git a/testcrypt.c b/testcrypt.c
index f818e7f1..6b9238b3 100644
--- a/testcrypt.c
+++ b/testcrypt.c
@@ -35,7 +35,7 @@
 #include "mpint.h"
 #include "ecc.h"
 
-static NORETURN void fatal_error(const char *p, ...)
+static NORETURN PRINTF_LIKE(1, 2) void fatal_error(const char *p, ...)
 {
     va_list ap;
     fprintf(stderr, "testcrypt: ");
diff --git a/testsc.c b/testsc.c
index 38edb8aa..0824c5e2 100644
--- a/testsc.c
+++ b/testsc.c
@@ -81,7 +81,7 @@
 #include "mpint.h"
 #include "ecc.h"
 
-static NORETURN void fatal_error(const char *p, ...)
+static NORETURN PRINTF_LIKE(1, 2) void fatal_error(const char *p, ...)
 {
     va_list ap;
     fprintf(stderr, "testsc: ");
diff --git a/tree234.c b/tree234.c
index 5e3146ba..a590382b 100644
--- a/tree234.c
+++ b/tree234.c
@@ -1072,7 +1072,7 @@ int n_errors = 0;
 /*
  * Error reporting function.
  */
-void error(char *fmt, ...)
+PRINTF_LIKE(1, 2) void error(char *fmt, ...)
 {
     va_list ap;
     printf("ERROR: ");