Ensure clean compilation with -Wformat -Wformat=2

Companion to samtools/htslib#1816, which among other things adds
printf format checking attributes to bcf_hdr_printf().  This fixes
a few places that could trigger warnings in bcftools as a result
of that change.  It also adds checking for some functions internal
to bcftools so `clang -Wformat -Wformat=2` works and fixes a couple
of bugs that were found as a result.

Bugs fixed:

 * In open_file() a user-controlled string was passed as the format
   to mkdir_p(), which could lead to information leakage or
   possibly arbitrary writes via a crafted file path name.

 * Use of wrong type for position in vcfsort sort_blocks() error report.

 * Printing the wrong variable in vcfsort do_partial_merge() error report.

The -Wformat=2 option disallows non-literals in printf-like
functions, which can be a little difficult to work around.  Here
most cases were fairly easy to sort out, apart from hdr_append()
in plugins/fill-tags.c.  For that the chosen solution was to
convert the function into a macro. While not particularly nice it
does allow all of the format strings passed in to be validated
properly.

Adds Wformat to clang CI build to catch any future regressions.

Author: daviesrob

.cirrus.yml

@@ -84,6 +84,7 @@ ubuntu_task:
 
   environment:
     CC: clang
+    CFLAGS: -g -O2 -Werror -Wall -Wformat -Wformat=2
     LC_ALL: C
     CIRRUS_CLONE_DEPTH: 1
     HTSDIR: ./htslib

Makefile

@@ -251,15 +251,15 @@ vcfconvert.o: vcfconvert.c $(htslib_faidx_h) $(htslib_vcf_h) $(htslib_bgzf_h) $(
 vcffilter.o: vcffilter.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_vcfutils_h) $(bcftools_h) $(filter_h) rbuf.h regidx.h
 vcfgtcheck.o: vcfgtcheck.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_vcfutils_h) $(htslib_kbitset_h) $(htslib_hts_os_h) $(htslib_bgzf_h) $(bcftools_h) extsort.h filter.h
 vcfindex.o: vcfindex.c $(htslib_vcf_h) $(htslib_tbx_h) $(htslib_kstring_h) $(htslib_bgzf_h) $(bcftools_h)
-vcfisec.o: vcfisec.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_vcfutils_h) $(htslib_hts_os_h) $(bcftools_h) $(filter_h)
+vcfisec.o: vcfisec.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_vcfutils_h) $(htslib_hts_os_h) $(htslib_hts_defs_h) $(bcftools_h) $(filter_h)
 vcfmerge.o: vcfmerge.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_vcfutils_h) $(htslib_faidx_h) $(htslib_kbitset_h) $(htslib_hts_endian_h) $(bcftools_h) regidx.h vcmp.h $(htslib_khash_h) $(htslib_kbitset_h)
 vcfnorm.o: vcfnorm.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_faidx_h) $(htslib_khash_str2int_h) $(bcftools_h) rbuf.h abuf.h gff.h regidx.h
 vcfquery.o: vcfquery.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_khash_str2int_h) $(htslib_vcfutils_h) $(bcftools_h) $(filter_h) $(convert_h) $(smpl_ilist_h)
 vcfroh.o: vcfroh.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_kstring_h) $(htslib_kseq_h) $(htslib_bgzf_h) $(bcftools_h) HMM.h $(smpl_ilist_h) $(filter_h)
-vcfcnv.o: vcfcnv.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_kstring_h) $(htslib_kfunc_h) $(htslib_khash_str2int_h) $(bcftools_h) HMM.h rbuf.h
+vcfcnv.o: vcfcnv.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_kstring_h) $(htslib_kfunc_h) $(htslib_khash_str2int_h) $(htslib_hts_defs_h) $(bcftools_h) HMM.h rbuf.h
 vcfhead.o: vcfhead.c $(htslib_kstring_h) $(htslib_vcf_h) $(bcftools_h)
-vcfsom.o: vcfsom.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_vcfutils_h) $(htslib_hts_os_h) $(bcftools_h)
-vcfsort.o: vcfsort.c $(htslib_vcf_h) $(htslib_kstring_h) $(htslib_hts_os_h) $(htslib_bgzf_h) kheap.h $(bcftools_h)
+vcfsom.o: vcfsom.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_vcfutils_h) $(htslib_hts_os_h) $(htslib_hts_defs_h) $(bcftools_h)
+vcfsort.o: vcfsort.c $(htslib_vcf_h) $(htslib_kstring_h) $(htslib_hts_os_h) $(htslib_hts_defs_h) $(htslib_bgzf_h) kheap.h $(bcftools_h)
 vcfstats.o: vcfstats.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_vcfutils_h) $(htslib_faidx_h) $(bcftools_h) $(filter_h) bin.h dist.h
 vcfview.o: vcfview.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_vcfutils_h) $(bcftools_h) $(filter_h) $(htslib_khash_str2int_h) $(htslib_kbitset_h)
 reheader.o: reheader.c $(htslib_vcf_h) $(htslib_bgzf_h) $(htslib_tbx_h) $(htslib_kseq_h) $(htslib_thread_pool_h) $(htslib_faidx_h) $(htslib_khash_str2int_h) $(bcftools_h) $(khash_str2str_h)
@@ -276,7 +276,7 @@ mcall.o: mcall.c $(htslib_kfunc_h) $(htslib_khash_str2int_h) $(call_h) $(prob1_h
 prob1.o: prob1.c $(prob1_h)
 vcmp.o: vcmp.c $(htslib_hts_h) $(htslib_vcf_h) vcmp.h
 ploidy.o: ploidy.c $(htslib_khash_str2int_h) $(htslib_kseq_h) $(htslib_hts_h) $(bcftools_h) $(ploidy_h)
-polysomy.o: polysomy.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(bcftools_h) peakfit.h
+polysomy.o: polysomy.c $(htslib_vcf_h) $(htslib_synced_bcf_reader_h) $(htslib_hts_defs_h) $(bcftools_h) peakfit.h
 peakfit.o: peakfit.c peakfit.h $(htslib_hts_h) $(htslib_kstring_h)
 bin.o: bin.c $(bcftools_h) bin.h
 dist.o: dist.c dist.h
@@ -326,7 +326,7 @@ test/test-rbuf.o: test/test-rbuf.c rbuf.h
 test/test-rbuf: test/test-rbuf.o
 	$(CC) $(LDFLAGS) -o $@ $^ $(ALL_LIBS)
 
-test/test-regidx.o: test/test-regidx.c $(htslib_kstring_h) $(htslib_hts_os_h) regidx.h
+test/test-regidx.o: test/test-regidx.c $(htslib_kstring_h) $(htslib_hts_os_h) $(htslib_hts_defs_h) regidx.h
 
 test/test-regidx: test/test-regidx.o regidx.o | $(HTSLIB)
 	$(CC) $(ALL_LDFLAGS) -o $@ $^ $(HTSLIB_LIB) -lpthread $(ALL_LIBS)

plugins/fill-tags.c

@@ -342,12 +342,17 @@ int ftf_filter_expr(args_t *args, bcf1_t *rec, pop_t *pop, ftf_t *ftf)
         error("Error occurred while updating %s at %s:%"PRId64"\n", args->str.s,bcf_seqname(args->in_hdr,rec),(int64_t) rec->pos+1);
     return 0;
 }
-void hdr_append(args_t *args, char *fmt)
-{
-    int i;
-    for (i=0; i<args->npop; i++)
-        bcf_hdr_printf(args->out_hdr, fmt, args->pop[i].suffix,*args->pop[i].name ? " in " : "",args->pop[i].name);
-}
+
+// This is implemented as a macro so the compiler can properly validate the
+// printf format string.
+#define hdr_append(args, fmt) \
+do { \
+    int i; \
+    for (i=0; i<args->npop; i++) \
+        bcf_hdr_printf(args->out_hdr, fmt, args->pop[i].suffix,*args->pop[i].name ? " in " : "",args->pop[i].name); \
+} while (0)
+
+
 int parse_func_pop(args_t *args, pop_t *pop, char *tag_expr, char *expr)
 {
     pop->nftf++;

plugins/scatter.c

@@ -28,6 +28,7 @@
 #include <unistd.h>
 #include <ctype.h>
 #include <htslib/synced_bcf_reader.h>
+#include <htslib/hts_defs.h>
 #include "bcftools.h"
 #include "htslib/khash_str2int.h"
 #include "regidx.h"
@@ -108,7 +109,7 @@ static const char *usage_text(void)
         "\n";
 }
 
-void mkdir_p(const char *fmt, ...);
+void mkdir_p(const char *fmt, ...) HTS_FORMAT(HTS_PRINTF_FMT, 1, 2);
 
 // most of this code was inspired by Petr Danecek's code in regidx.c
 #define MAX_COOR_0 REGIDX_MAX   // CSI and hts_itr_query limit, 0-based

plugins/split-vep.c

@@ -711,15 +711,13 @@ static void parse_column_str(args_t *args)
         ann->idx = j = column[i];
         ann->field = strdup(args->field[j]);
         ann->tag = strdup(args->field[j]);
-        args->kstr.l = 0;
         const char *type = "String";
         if ( ann->type==BCF_HT_REAL ) type = "Float";
         else if ( ann->type==BCF_HT_INT ) type = "Integer";
         else if ( ann->type==BCF_HT_FLAG ) type = "Flag";
         else if ( ann->type==BCF_HT_STR ) type = "String";
         else if ( ann->type==-1 ) type = get_column_type(args, args->field[j], &ann->type);
-        ksprintf(&args->kstr,"##INFO=<ID=%%s,Number=.,Type=%s,Description=\"The %%s field from INFO/%%s\">",type);
-        bcf_hdr_printf(args->hdr_out, args->kstr.s, ann->tag,ann->field,args->vep_tag);
+        bcf_hdr_printf(args->hdr_out, "##INFO=<ID=%s,Number=.,Type=%s,Description=\"The %s field from INFO/%s\">", ann->tag,type,ann->field,args->vep_tag);
         if ( str.l ) kputc(',',&str);
         kputs(ann->tag,&str);
     }

plugins/split.c

@@ -143,7 +143,7 @@ static const char *usage_text(void)
         "\n";
 }
 
-void mkdir_p(const char *fmt, ...);
+void mkdir_p(const char *fmt, ...) HTS_FORMAT(HTS_PRINTF_FMT, 1, 2);
 
 static char *create_unique_file_name(args_t *args, const char *template)
 {

polysomy.c

@@ -34,6 +34,7 @@
 #include <gsl/gsl_multifit_nlin.h>
 #include <htslib/vcf.h>
 #include <htslib/synced_bcf_reader.h>
+#include <htslib/hts_defs.h>
 #include "bcftools.h"
 #include "peakfit.h"
 
@@ -62,7 +63,7 @@ typedef struct
 }
 args_t;
 
-FILE *open_file(char **fname, const char *mode, const char *fmt, ...);
+FILE *open_file(char **fname, const char *mode, const char *fmt, ...) HTS_FORMAT(HTS_PRINTF_FMT, 3, 4);
 
 static void init_dist(args_t *args, dist_t *dist, int verbose)
 {

test/test-regidx.c

@@ -33,28 +33,32 @@
 #include <getopt.h>
 #include <htslib/kstring.h>
 #include <htslib/hts_os.h>
+#include <htslib/hts_defs.h>
 #include <time.h>
 #include "regidx.h"
 
 static int verbose = 0;
 
-void debug(const char *format, ...)
+void HTS_FORMAT(HTS_PRINTF_FMT, 1, 2)
+debug(const char *format, ...)
 {
     if ( verbose<2 ) return;
     va_list ap;
     va_start(ap, format);
     vfprintf(stderr, format, ap);
     va_end(ap);
 }
-void info(const char *format, ...)
+void HTS_FORMAT(HTS_PRINTF_FMT, 1, 2)
+info(const char *format, ...)
 {
     if ( verbose<1 ) return;
     va_list ap;
     va_start(ap, format);
     vfprintf(stderr, format, ap);
     va_end(ap);
 }
-void error(const char *format, ...)
+void HTS_FORMAT(HTS_PRINTF_FMT, 1, 2) HTS_NORETURN
+error(const char *format, ...)
 {
     va_list ap;
     va_start(ap, format);

vcfcnv.c

@@ -41,6 +41,7 @@
 #include <htslib/kstring.h>
 #include <htslib/kfunc.h>
 #include <htslib/khash_str2int.h>
+#include <htslib/hts_defs.h>
 #include "bcftools.h"
 #include "HMM.h"
 #include "rbuf.h"
@@ -105,7 +106,7 @@ typedef struct _args_t
 }
 args_t;
 
-FILE *open_file(char **fname, const char *mode, const char *fmt, ...);
+FILE *open_file(char **fname, const char *mode, const char *fmt, ...) HTS_FORMAT(HTS_PRINTF_FMT, 3, 4);
 
 static inline void hmm2cn_state(int nstates, int i, int *a, int *b)
 {

vcfisec.c

@@ -34,6 +34,7 @@ THE SOFTWARE.  */
 #include <htslib/synced_bcf_reader.h>
 #include <htslib/vcfutils.h>
 #include <htslib/hts_os.h>
+#include <htslib/hts_defs.h>
 #include "bcftools.h"
 #include "filter.h"
 
@@ -69,32 +70,33 @@ args_t;
  *  mkdir_p() - create new directory for a file $fname
  *  @fname:   the file name to create the directory for, the part after last "/" is ignored
  */
-void mkdir_p(const char *fmt, ...)
+void HTS_FORMAT(HTS_PRINTF_FMT, 1, 2)
+mkdir_p(const char *fmt, ...)
 {
     va_list ap;
     va_start(ap, fmt);
     int n = vsnprintf(NULL, 0, fmt, ap) + 2;
     va_end(ap);
 
-    char *path = (char*)malloc(n);
+    char *tmp = (char*)malloc(n);
+    if (!tmp) error("Couldn't allocate space for path: %s\n", strerror(errno));
     va_start(ap, fmt);
-    vsnprintf(path, n, fmt, ap);
+    vsnprintf(tmp, n, fmt, ap);
     va_end(ap);
 
-    char *tmp = strdup(path), *p = tmp+1;
+    char *p = tmp+1;
     while (*p)
     {
         while (*p && *p!='/') p++;
         if ( !*p ) break;
         char ctmp = *p;
         *p = 0;
         int ret = mkdir(tmp,S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH);
-        if ( ret!=0 && errno!=EEXIST ) error("Error creating directory %s: %s\n", path,strerror(errno));
+        if ( ret!=0 && errno!=EEXIST ) error("Error creating directory %s: %s\n", tmp,strerror(errno));
         *p = ctmp;
         while ( *p && *p=='/' ) p++;
     }
     free(tmp);
-    free(path);
 }
 
 /**
@@ -105,7 +107,8 @@ void mkdir_p(const char *fmt, ...)
  *
  *  Returns open file descriptor or NULL if mode is NULL.
  */
-FILE *open_file(char **fname, const char *mode, const char *fmt, ...)
+FILE * HTS_FORMAT(HTS_PRINTF_FMT, 3, 4)
+open_file(char **fname, const char *mode, const char *fmt, ...)
 {
     va_list ap;
     va_start(ap, fmt);
@@ -117,7 +120,7 @@ FILE *open_file(char **fname, const char *mode, const char *fmt, ...)
     vsnprintf(str, n, fmt, ap);
     va_end(ap);
 
-    mkdir_p(str);
+    mkdir_p("%s", str);
     if ( !mode )
     {
         if ( !fname ) error("Uh: expected fname or mode\n");