Re: [Qemu-devel] [PATCH] qht: do not segfault when gathering stats from an uninitialized qht

From: Paolo Bonzini <pbonzini@redhat.com>
To: Peter Maydell <peter.maydell@linaro.org>,
	"Emilio G. Cota" <cota@braap.org>
Cc: Changlong Xie <xiecl.fnst@cn.fujitsu.com>,
	QEMU Developers <qemu-devel@nongnu.org>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [Qemu-devel] [PATCH] qht: do not segfault when gathering stats from an uninitialized qht
Date: Sat, 23 Jul 2016 12:54:51 +0200	[thread overview]
Message-ID: <f20f643c-90f4-bd1c-8055-9204e7502e25@redhat.com> (raw)
In-Reply-To: <CAFEAcA_doX3DFP4Ce=nFAGPgQj=N3CA3B05BP1ViiZjcbWun=Q@mail.gmail.com>



On 23/07/2016 12:01, Peter Maydell wrote:
> On 22 July 2016 at 17:36, Emilio G. Cota <cota@braap.org> wrote:
>> So far, QHT functions assume that the passed qht has previously been
>> initialized--otherwise they segfault.
>>
>> This patch makes an exception for qht_statistics_init, with the goal
>> of simplifying calling code. For instance, qht_statistics_init is
>> called from the 'info jit' dump, and given that under KVM the TB qht
>> is never initialized, we get a segfault. Thus, instead of complicating
>> the 'info jit' code with additional checks, let's allow passing an
>> uninitialized qht to qht_statistics_init.
>>
>> While at it, add a test for this to test-qht.
>>
>> Before the patch (for $ qemu -enable-kvm [...]):
>> (qemu) info jit
>> [...]
>> direct jump count   0 (0%) (2 jumps=0 0%)
>> Program received signal SIGSEGV, Segmentation fault.
>>
>> After the patch:
>> (qemu) info jit
>> [...]
>> direct jump count   0 (0%) (2 jumps=0 0%)
>> TB hash buckets     0/0 (-nan% head buckets used)
>> TB hash occupancy   nan% avg chain occ. Histogram: (null)
>> TB hash avg chain   nan buckets. Histogram: (null)
> 
> This looks like we're passing NULL pointers to
> printf %s specifiers. This is undefined behaviour at least
> for POSIX printf, and I can't see anything in the glib
> printf-alike function documentation that gives an extra
> guarantee for this, so it's probably a bad idea.
> 
> Printing 'nan' also looks a bit odd, though it's not UB.

Let's move everything to a new function, so that it's easy to add a
check at the top:

diff --git a/translate-all.c b/translate-all.c
index 0d47c1c..efeba29 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1663,15 +1663,50 @@ void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr)
            TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *));
 }
 
+static void print_qht_statistics(FILE *f, fprintf_function cpu_fprintf,
+                                 struct qht_stats hst)
+{
+    uint32_t hgram_opts;
+    size_t hgram_bins;
+    char *hgram;
+
+    if (!hst.head_buckets) {
+        return;
+    }
+    cpu_fprintf(f, "TB hash buckets     %zu/%zu (%0.2f%% head buckets used)\n",
+                hst.used_head_buckets, hst.head_buckets,
+                (double)hst.used_head_buckets / hst.head_buckets * 100);
+
+    hgram_opts =  QDIST_PR_BORDER | QDIST_PR_LABELS;
+    hgram_opts |= QDIST_PR_100X   | QDIST_PR_PERCENT;
+    if (qdist_xmax(&hst.occupancy) - qdist_xmin(&hst.occupancy) == 1) {
+        hgram_opts |= QDIST_PR_NODECIMAL;
+    }
+    hgram = qdist_pr(&hst.occupancy, 10, hgram_opts);
+    cpu_fprintf(f, "TB hash occupancy   %0.2f%% avg chain occ. Histogram: %s\n",
+                qdist_avg(&hst.occupancy) * 100, hgram);
+    g_free(hgram);
+
+    hgram_opts = QDIST_PR_BORDER | QDIST_PR_LABELS;
+    hgram_bins = qdist_xmax(&hst.chain) - qdist_xmin(&hst.chain);
+    if (hgram_bins > 10) {
+        hgram_bins = 10;
+    } else {
+        hgram_bins = 0;
+        hgram_opts |= QDIST_PR_NODECIMAL | QDIST_PR_NOBINRANGE;
+    }
+    hgram = qdist_pr(&hst.chain, hgram_bins, hgram_opts);
+    cpu_fprintf(f, "TB hash avg chain   %0.3f buckets. Histogram: %s\n",
+                qdist_avg(&hst.chain), hgram);
+    g_free(hgram);
+}
+
 void dump_exec_info(FILE *f, fprintf_function cpu_fprintf)
 {
     int i, target_code_size, max_target_code_size;
     int direct_jmp_count, direct_jmp2_count, cross_page;
     TranslationBlock *tb;
     struct qht_stats hst;
-    uint32_t hgram_opts;
-    size_t hgram_bins;
-    char *hgram;
 
     target_code_size = 0;
     max_target_code_size = 0;
@@ -1724,34 +1759,7 @@ void dump_exec_info(FILE *f, fprintf_function cpu_fprintf)
                         tcg_ctx.tb_ctx.nb_tbs : 0);
 
     qht_statistics_init(&tcg_ctx.tb_ctx.htable, &hst);
-
-    cpu_fprintf(f, "TB hash buckets     %zu/%zu (%0.2f%% head buckets used)\n",
-                hst.used_head_buckets, hst.head_buckets,
-                (double)hst.used_head_buckets / hst.head_buckets * 100);
-
-    hgram_opts =  QDIST_PR_BORDER | QDIST_PR_LABELS;
-    hgram_opts |= QDIST_PR_100X   | QDIST_PR_PERCENT;
-    if (qdist_xmax(&hst.occupancy) - qdist_xmin(&hst.occupancy) == 1) {
-        hgram_opts |= QDIST_PR_NODECIMAL;
-    }
-    hgram = qdist_pr(&hst.occupancy, 10, hgram_opts);
-    cpu_fprintf(f, "TB hash occupancy   %0.2f%% avg chain occ. Histogram: %s\n",
-                qdist_avg(&hst.occupancy) * 100, hgram);
-    g_free(hgram);
-
-    hgram_opts = QDIST_PR_BORDER | QDIST_PR_LABELS;
-    hgram_bins = qdist_xmax(&hst.chain) - qdist_xmin(&hst.chain);
-    if (hgram_bins > 10) {
-        hgram_bins = 10;
-    } else {
-        hgram_bins = 0;
-        hgram_opts |= QDIST_PR_NODECIMAL | QDIST_PR_NOBINRANGE;
-    }
-    hgram = qdist_pr(&hst.chain, hgram_bins, hgram_opts);
-    cpu_fprintf(f, "TB hash avg chain   %0.3f buckets. Histogram: %s\n",
-                qdist_avg(&hst.chain), hgram);
-    g_free(hgram);
-
+    print_qht_statistics(f, cpu_fprintf, hst);
     qht_statistics_destroy(&hst);
 
     cpu_fprintf(f, "\nStatistics:\n");
