From: Ian Jackson <ijackson@chiark.greenend.org.uk>
Date: Thu, 25 Jul 2013 17:30:53 +0000 (+0100)
Subject: Use FORMAT everywhere, and fix up the errors it finds
X-Git-Tag: debian/0.3.0_beta2~7
X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?p=secnet.git;a=commitdiff_plain;h=779837e133a4740798015a1d7de6fbf6b4b1720f

Use FORMAT everywhere, and fix up the errors it finds

Many printf-like variadic functions weren't properly decorated with
FORMAT annotations.  Add them everywhere.

It is not possible to annotate a function pointer type, so that
doesn't work for the function pointers in struct log_if.  However, we
have convenience wrapper functions slilog and vslilog, which are
appropriately decorated and therefore safer.  Change all call sites to
use those instead, and leave a comment.  (Rename the function pointer
variable names so that we don't miss any call sites.)

Fix the bugs that this new compiler checking reveals.  These are
nearly all in fatal error reporting, and not very scary.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---

diff --git a/log.c b/log.c
index d940df5..d330113 100644
--- a/log.c
+++ b/log.c
@@ -14,6 +14,8 @@ bool_t secnet_is_daemon=False;
 uint32_t message_level=M_WARNING|M_ERR|M_SECURITY|M_FATAL;
 struct log_if *system_log=NULL;
 
+static void vMessageFallback(uint32_t class, const char *message, va_list args)
+    FORMAT(printf,2,0);
 static void vMessageFallback(uint32_t class, const char *message, va_list args)
 {
     FILE *dest=stdout;
@@ -60,6 +62,8 @@ void Message(uint32_t class, const char *message, ...)
     va_end(ap);
 }
 
+static void MessageFallback(uint32_t class, const char *message, ...)
+    FORMAT(printf,2,3);
 static void MessageFallback(uint32_t class, const char *message, ...)
 {
     va_list ap;
@@ -191,7 +195,7 @@ static void log_vmulti(void *sst, int class, const char *message, va_list args)
 
     if (secnet_is_daemon) {
 	for (i=st; i; i=i->next) {
-	    i->l->vlog(i->l->st,class,message,args);
+	    vslilog(i->l,class,message,args);
 	}
     } else {
 	vMessage(class,message,args);
@@ -199,6 +203,8 @@ static void log_vmulti(void *sst, int class, const char *message, va_list args)
     }
 }
 
+static void log_multi(void *st, int priority, const char *message, ...)
+    FORMAT(printf,3,4);
 static void log_multi(void *st, int priority, const char *message, ...)
 {
     va_list ap;
@@ -242,8 +248,8 @@ struct log_if *init_log(list_t *ll)
     }
     r=safe_malloc(sizeof(*r), "init_log");
     r->st=l;
-    r->log=log_multi;
-    r->vlog=log_vmulti;
+    r->logfn=log_multi;
+    r->vlogfn=log_vmulti;
     return r;
 }
 
@@ -283,6 +289,8 @@ static void logfile_vlog(void *sst, int class, const char *message,
     }
 }
 
+static void logfile_log(void *state, int class, const char *message, ...)
+    FORMAT(printf,3,4);
 static void logfile_log(void *state, int class, const char *message, ...)
 {
     va_list ap;
@@ -355,8 +363,8 @@ static list_t *logfile_apply(closure_t *self, struct cloc loc, dict_t *context,
     st->cl.apply=NULL;
     st->cl.interface=&st->ops;
     st->ops.st=st;
-    st->ops.log=logfile_log;
-    st->ops.vlog=logfile_vlog;
+    st->ops.logfn=logfile_log;
+    st->ops.vlogfn=logfile_vlog;
     st->loc=loc;
     st->f=NULL;
 
@@ -399,6 +407,9 @@ static int msgclass_to_syslogpriority(uint32_t m)
     }
 }
     
+static void syslog_vlog(void *sst, int class, const char *message,
+			 va_list args)
+    FORMAT(printf,3,0);
 static void syslog_vlog(void *sst, int class, const char *message,
 			 va_list args)
 {
@@ -412,6 +423,8 @@ static void syslog_vlog(void *sst, int class, const char *message,
     }
 }
 
+static void syslog_log(void *sst, int priority, const char *message, ...)
+    FORMAT(printf,3,4);
 static void syslog_log(void *sst, int priority, const char *message, ...)
 {
     va_list ap;
@@ -472,8 +485,8 @@ static list_t *syslog_apply(closure_t *self, struct cloc loc, dict_t *context,
     st->cl.apply=NULL;
     st->cl.interface=&st->ops;
     st->ops.st=st;
-    st->ops.log=syslog_log;
-    st->ops.vlog=syslog_vlog;
+    st->ops.logfn=syslog_log;
+    st->ops.vlogfn=syslog_vlog;
 
     item=list_elem(args,0);
     if (!item || item->type!=t_dict)
@@ -528,7 +541,7 @@ static void log_from_fd_afterpoll(void *sst, struct pollfd *fds, int nfds)
 	remain=FDLOG_BUFSIZE-st->i-1;
 	if (remain<=0) {
 	    st->buffer[FDLOG_BUFSIZE-1]=0;
-	    st->log->log(st->log,M_WARNING,"%s: overlong line: %s",
+	    slilog(st->log,M_WARNING,"%s: overlong line: %s",
 			 st->prefix,st->buffer);
 	    st->i=0;
 	    remain=FDLOG_BUFSIZE-1;
@@ -539,7 +552,7 @@ static void log_from_fd_afterpoll(void *sst, struct pollfd *fds, int nfds)
 	    for (i=0; i<st->i; i++) {
 		if (st->buffer[i]=='\n') {
 		    st->buffer[i]=0;
-		    st->log->log(st->log->st,M_INFO,"%s: %s",
+		    slilog(st->log,M_INFO,"%s: %s",
 				 st->prefix,st->buffer);
 		    i++;
 		    memmove(st->buffer,st->buffer+i,st->i-i);
diff --git a/secnet.c b/secnet.c
index 465a93f..c604d44 100644
--- a/secnet.c
+++ b/secnet.c
@@ -332,7 +332,8 @@ static void run(void)
 		fatal("run: beforepoll_fn (%s) returns %d",i->desc,rv);
 	    }
 	    if (timeout<-1) {
-		fatal("run: beforepoll_fn (%s) set timeout to %d",timeout);
+		fatal("run: beforepoll_fn (%s) set timeout to %d",
+		      i->desc,timeout);
 	    }
 	    idx+=nfds;
 	    remain-=nfds;
diff --git a/secnet.h b/secnet.h
index 252eeb6..98a3ae3 100644
--- a/secnet.h
+++ b/secnet.h
@@ -339,8 +339,8 @@ typedef void log_vmsg_fn(void *st, int class, const char *message,
 			 va_list args);
 struct log_if {
     void *st;
-    log_msg_fn *log;
-    log_vmsg_fn *vlog;
+    log_msg_fn *logfn;   /* Do not call these directly - you don't get */
+    log_vmsg_fn *vlogfn; /* printf format checking.  Use [v]slilog instead */
 };
 /* (convenience functions, defined in util.c) */
 extern void slilog(struct log_if *lf, int class, const char *message, ...)
@@ -492,21 +492,25 @@ struct buffer_if {
 #define M_FATAL	       0x100
 
 /* The fatal() family of functions require messages that do not end in '\n' */
-extern NORETURN(fatal(const char *message, ...));
-extern NORETURN(fatal_perror(const char *message, ...));
-extern NORETURN(fatal_status(int status, const char *message, ...));
-extern NORETURN(fatal_perror_status(int status, const char *message, ...));
+extern NORETURN(fatal(const char *message, ...)) FORMAT(printf,1,2);
+extern NORETURN(fatal_perror(const char *message, ...)) FORMAT(printf,1,2);
+extern NORETURN(fatal_status(int status, const char *message, ...))
+       FORMAT(printf,2,3);
+extern NORETURN(fatal_perror_status(int status, const char *message, ...))
+       FORMAT(printf,2,3);
 
 /* The cfgfatal() family of functions require messages that end in '\n' */
 extern NORETURN(cfgfatal(struct cloc loc, cstring_t facility,
-			 const char *message, ...));
+			 const char *message, ...)) FORMAT(printf,3,4);
 extern void cfgfile_postreadcheck(struct cloc loc, FILE *f);
 extern NORETURN(vcfgfatal_maybefile(FILE *maybe_f, struct cloc loc,
 				    cstring_t facility, const char *message,
-				    va_list));
+				    va_list))
+    FORMAT(printf,4,0);
 extern NORETURN(cfgfatal_maybefile(FILE *maybe_f, struct cloc loc,
 				   cstring_t facility,
-				   const char *message, ...));
+				   const char *message, ...))
+    FORMAT(printf,4,5);
 
 extern void Message(uint32_t class, const char *message, ...)
     FORMAT(printf,2,3);
diff --git a/site.c b/site.c
index 6c92193..20f9507 100644
--- a/site.c
+++ b/site.c
@@ -293,6 +293,8 @@ struct site {
     struct transform_inst_if *new_transform; /* For key setup/verify */
 };
 
+static void slog(struct site *st, uint32_t event, cstring_t msg, ...)
+FORMAT(printf,3,4);
 static void slog(struct site *st, uint32_t event, cstring_t msg, ...)
 {
     va_list ap;
@@ -318,7 +320,7 @@ static void slog(struct site *st, uint32_t event, cstring_t msg, ...)
 	}
 
 	vsnprintf(buf,sizeof(buf),msg,ap);
-	st->log->log(st->log->st,class,"%s: %s",st->tunname,buf);
+	slilog(st->log,class,"%s: %s",st->tunname,buf);
     }
     va_end(ap);
 }
@@ -1906,7 +1908,7 @@ static bool_t transport_compute_setupinit_peers(struct site *st,
     slog(st,LOG_SETUP_INIT,
 	 (!configured_addr ? "using only %d old peer address(es)"
 	  : "using configured address, and/or perhaps %d old peer address(es)"),
-	 st->peers);
+	 st->peers.npeers);
 
     /* Non-mobile peers havve st->peers.npeers==0 or ==1, since they
      * have transport_peers_max==1.  The effect is that this code
diff --git a/slip.c b/slip.c
index d8f1a17..0f62a69 100644
--- a/slip.c
+++ b/slip.c
@@ -230,7 +230,8 @@ static void userv_userv_callback(void *sst, pid_t pid, int status)
 	    fatal("%s: userv exited unexpectedly: uncaught signal %d",
 		  st->slip.nl.name,WTERMSIG(status));
 	} else {
-	    fatal("%s: userv stopped unexpectedly");
+	    fatal("%s: userv stopped unexpectedly",
+		  st->slip.nl.name);
 	}
     }
     Message(M_WARNING,"%s: userv subprocess died with status %d\n",
diff --git a/util.c b/util.c
index cfaefd2..24dd9a3 100644
--- a/util.c
+++ b/util.c
@@ -61,7 +61,7 @@ char *safe_strdup(const char *s, const char *message)
     char *d;
     d=strdup(s);
     if (!d) {
-	fatal_perror(message);
+	fatal_perror("%s",message);
     }
     return d;
 }
@@ -71,7 +71,7 @@ void *safe_malloc(size_t size, const char *message)
     void *r;
     r=malloc(size);
     if (!r) {
-	fatal_perror(message);
+	fatal_perror("%s",message);
     }
     return r;
 }
@@ -208,7 +208,7 @@ bool_t remove_hook(uint32_t phase, hook_fn *fn, void *state)
 
 void vslilog(struct log_if *lf, int priority, const char *message, va_list ap)
 {
-    lf->vlog(lf->st,priority,message,ap);
+    lf->vlogfn(lf->st,priority,message,ap);
 }
 
 void slilog(struct log_if *lf, int priority, const char *message, ...)