| Message ID | 20210201225404.3941395-3-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | i386: Ensure feature names are always defined | expand |
On 2/1/21 11:54 PM, Eduardo Habkost wrote: > Additional sanity checks will be added to the code, so move the > existing asserts to a separate function. > > Wrap the whole function in `#ifndef NDEBUG` because the checks > will become more complex than trivial assert() calls. How can you build with NDEBUG? See commit 262a69f4282: osdep.h: Prohibit disabling assert() in supported builds We already have several files that knowingly require assert() to work, sometimes because refactoring the code for proper error handling has not been tackled yet; there are probably other files that have a similar situation but with no comments documenting the same. In fact, we have places in migration that handle untrusted input with assertions, where disabling the assertions risks a worse security hole than the current behavior of losing the guest to SIGABRT when migration fails because of the assertion. Promote our current per-file safety-valve to instead be project-wide, and expand it to also cover glib's g_assert(). and "qemu/osdep.h": /* * We have a lot of unaudited code that may fail in strange ways, or * even be a security risk during migration, if you disable assertions * at compile-time. You may comment out these safety checks if you * absolutely want to disable assertion overhead, but it is not * supported upstream so the risk is all yours. Meanwhile, please * submit patches to remove any side-effects inside an assertion, or * fixing error handling that should use Error instead of assert. */ #ifdef NDEBUG #error building with NDEBUG is not supported #endif IOW no need for the #idef. > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target/i386/cpu.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 2bf3ab78056..6285fb00eb8 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -5431,17 +5431,25 @@ static void x86_register_cpu_model_type(const char *name, X86CPUModel *model) > type_register(&ti); > } > > -static void x86_register_cpudef_types(X86CPUDefinition *def) > +/* Sanity check CPU model definition before registering it */ > +static void x86_cpudef_validate(X86CPUDefinition *def) > { > - X86CPUModel *m; > - const X86CPUVersionDefinition *vdef; > - > +#ifndef NDEBUG > /* AMD aliases are handled at runtime based on CPUID vendor, so > * they shouldn't be set on the CPU model table. > */ > assert(!(def->features[FEAT_8000_0001_EDX] & CPUID_EXT2_AMD_ALIASES)); > /* catch mistakes instead of silently truncating model_id when too long */ > assert(def->model_id && strlen(def->model_id) <= 48); > +#endif > +} > + > +static void x86_register_cpudef_types(X86CPUDefinition *def) > +{ > + X86CPUModel *m; > + const X86CPUVersionDefinition *vdef; > + > + x86_cpudef_validate(def); > > /* Unversioned model: */ > m = g_new0(X86CPUModel, 1); >
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 2bf3ab78056..6285fb00eb8 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -5431,17 +5431,25 @@ static void x86_register_cpu_model_type(const char *name, X86CPUModel *model) type_register(&ti); } -static void x86_register_cpudef_types(X86CPUDefinition *def) +/* Sanity check CPU model definition before registering it */ +static void x86_cpudef_validate(X86CPUDefinition *def) { - X86CPUModel *m; - const X86CPUVersionDefinition *vdef; - +#ifndef NDEBUG /* AMD aliases are handled at runtime based on CPUID vendor, so * they shouldn't be set on the CPU model table. */ assert(!(def->features[FEAT_8000_0001_EDX] & CPUID_EXT2_AMD_ALIASES)); /* catch mistakes instead of silently truncating model_id when too long */ assert(def->model_id && strlen(def->model_id) <= 48); +#endif +} + +static void x86_register_cpudef_types(X86CPUDefinition *def) +{ + X86CPUModel *m; + const X86CPUVersionDefinition *vdef; + + x86_cpudef_validate(def); /* Unversioned model: */ m = g_new0(X86CPUModel, 1);
Additional sanity checks will be added to the code, so move the existing asserts to a separate function. Wrap the whole function in `#ifndef NDEBUG` because the checks will become more complex than trivial assert() calls. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target/i386/cpu.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)