* [PULL 1/3] tests/qtest/migration-test: Fix unlink error and memory leaks
2022-12-04 7:30 [PULL 0/3] Optional fixes for inclusion into QEMU 7.2.0-rc4 Thomas Huth
@ 2022-12-04 7:30 ` Thomas Huth
2022-12-04 7:30 ` [PULL 2/3] target/s390x/tcg: Fix and improve the SACF instruction Thomas Huth
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Thomas Huth @ 2022-12-04 7:30 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
When running the migration test compiled with Clang from Fedora 37
and sanitizers enabled, there is an error complaining about unlink():
../tests/qtest/migration-test.c:1072:12: runtime error: null pointer
passed as argument 1, which is declared to never be null
/usr/include/unistd.h:858:48: note: nonnull attribute specified here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
../tests/qtest/migration-test.c:1072:12 in
(test program exited with status code 1)
TAP parsing error: Too few tests run (expected 33, got 20)
The data->clientcert and data->clientkey pointers can indeed be unset
in some tests, so we have to check them before calling unlink() with
those.
While we're at it, I also noticed that the code is only freeing
some but not all of the allocated strings in this function, and
indeed, valgrind is also complaining about memory leaks here.
So let's call g_free() on all allocated strings to avoid leaking
memory here.
Message-Id: <20221125083054.117504-1-thuth@redhat.com>
Tested-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
tests/qtest/migration-test.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 442998d9eb..dbde726adf 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1066,15 +1066,27 @@ test_migrate_tls_x509_finish(QTestState *from,
TestMigrateTLSX509Data *data = opaque;
test_tls_cleanup(data->keyfile);
+ g_free(data->keyfile);
+
unlink(data->cacert);
+ g_free(data->cacert);
unlink(data->servercert);
+ g_free(data->servercert);
unlink(data->serverkey);
- unlink(data->clientcert);
- unlink(data->clientkey);
- rmdir(data->workdir);
+ g_free(data->serverkey);
+ if (data->clientcert) {
+ unlink(data->clientcert);
+ g_free(data->clientcert);
+ }
+ if (data->clientkey) {
+ unlink(data->clientkey);
+ g_free(data->clientkey);
+ }
+
+ rmdir(data->workdir);
g_free(data->workdir);
- g_free(data->keyfile);
+
g_free(data);
}
#endif /* CONFIG_TASN1 */
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PULL 2/3] target/s390x/tcg: Fix and improve the SACF instruction
2022-12-04 7:30 [PULL 0/3] Optional fixes for inclusion into QEMU 7.2.0-rc4 Thomas Huth
2022-12-04 7:30 ` [PULL 1/3] tests/qtest/migration-test: Fix unlink error and memory leaks Thomas Huth
@ 2022-12-04 7:30 ` Thomas Huth
2022-12-04 7:30 ` [PULL 3/3] hw/display/next-fb: Fix comment typo Thomas Huth
2022-12-04 23:47 ` [PULL 0/3] Optional fixes for inclusion into QEMU 7.2.0-rc4 Stefan Hajnoczi
3 siblings, 0 replies; 5+ messages in thread
From: Thomas Huth @ 2022-12-04 7:30 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
The SET ADDRESS SPACE CONTROL FAST instruction is not privileged, it can be
used from problem space, too. Just the switching to the home address space
is privileged and should still generate a privilege exception. This bug is
e.g. causing programs like Java that use the "getcpu" vdso kernel function
to crash (see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=990417#26 ).
While we're at it, also check if DAT is not enabled. In that case the
instruction is supposed to generate a special operation exception.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/655
Message-Id: <20221201184443.136355-1-thuth@redhat.com>
Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
target/s390x/tcg/insn-data.h.inc | 2 +-
target/s390x/tcg/cc_helper.c | 7 +++++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
index 7e952bdfc8..54d4250c9f 100644
--- a/target/s390x/tcg/insn-data.h.inc
+++ b/target/s390x/tcg/insn-data.h.inc
@@ -1365,7 +1365,7 @@
/* SERVICE CALL LOGICAL PROCESSOR (PV hypercall) */
F(0xb220, SERVC, RRE, Z, r1_o, r2_o, 0, 0, servc, 0, IF_PRIV | IF_IO)
/* SET ADDRESS SPACE CONTROL FAST */
- F(0xb279, SACF, S, Z, 0, a2, 0, 0, sacf, 0, IF_PRIV)
+ C(0xb279, SACF, S, Z, 0, a2, 0, 0, sacf, 0)
/* SET CLOCK */
F(0xb204, SCK, S, Z, 0, m2_64a, 0, 0, sck, 0, IF_PRIV | IF_IO)
/* SET CLOCK COMPARATOR */
diff --git a/target/s390x/tcg/cc_helper.c b/target/s390x/tcg/cc_helper.c
index b2e8d3d9f5..b36f8cdc8b 100644
--- a/target/s390x/tcg/cc_helper.c
+++ b/target/s390x/tcg/cc_helper.c
@@ -487,6 +487,10 @@ void HELPER(sacf)(CPUS390XState *env, uint64_t a1)
{
HELPER_LOG("%s: %16" PRIx64 "\n", __func__, a1);
+ if (!(env->psw.mask & PSW_MASK_DAT)) {
+ tcg_s390_program_interrupt(env, PGM_SPECIAL_OP, GETPC());
+ }
+
switch (a1 & 0xf00) {
case 0x000:
env->psw.mask &= ~PSW_MASK_ASC;
@@ -497,6 +501,9 @@ void HELPER(sacf)(CPUS390XState *env, uint64_t a1)
env->psw.mask |= PSW_ASC_SECONDARY;
break;
case 0x300:
+ if ((env->psw.mask & PSW_MASK_PSTATE) != 0) {
+ tcg_s390_program_interrupt(env, PGM_PRIVILEGED, GETPC());
+ }
env->psw.mask &= ~PSW_MASK_ASC;
env->psw.mask |= PSW_ASC_HOME;
break;
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread