Message ID | 20190919124115.11510-1-cohuck@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thu, 19 Sep 2019 at 13:41, Cornelia Huck <cohuck@redhat.com> wrote: > > The following changes since commit f8c3db33a5e863291182f8862ddf81618a7c6194: > > target/sparc: Switch to do_transaction_failed() hook (2019-09-17 12:01:00 +0100) > > are available in the Git repository at: > > https://github.com/cohuck/qemu tags/s390x-20190919 > > for you to fetch changes up to 37105adebeb28e60da3cb1ef82231d7ed8d23589: > > Merge tag 'tags/s390-ccw-bios-2019-09-18' into s390-next-staging (2019-09-19 12:04:01 +0200) > > ---------------------------------------------------------------- > - bugfixes in tcg and the ccw bios > - gen15a is called z15 > - officially require a 3.15 kernel or later for kvm > > ---------------------------------------------------------------- Hi -- I'm afraid this pullreq results in new warnings from the runtime-sanitizer build when 'make check' is run: MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} QTEST_QEMU_BINARY=s390x-softmmu/qemu-system-s390x QTEST_QEMU_IMG=qemu-img tests /boot-serial-test -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="boot-serial-test" /home/petmay01/linaro/qemu-for-merges/target/s390x/mem_helper.c:293:17: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/string.h:47:14: note: nonnull attribute specified here /home/petmay01/linaro/qemu-for-merges/target/s390x/mem_helper.c:293:32: runtime error: null pointer passed as argument 2, which is declared to never be null (and the same warnings for a few other tests). Looks like you sometimes can pass NULL pointers to the source and destination of memmove(). This isn't permitted by the standard even in the case where the size argument is zero. thanks -- PMM
On Fri, 20 Sep 2019 11:45:18 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On Thu, 19 Sep 2019 at 13:41, Cornelia Huck <cohuck@redhat.com> wrote: > > > > The following changes since commit f8c3db33a5e863291182f8862ddf81618a7c6194: > > > > target/sparc: Switch to do_transaction_failed() hook (2019-09-17 12:01:00 +0100) > > > > are available in the Git repository at: > > > > https://github.com/cohuck/qemu tags/s390x-20190919 > > > > for you to fetch changes up to 37105adebeb28e60da3cb1ef82231d7ed8d23589: > > > > Merge tag 'tags/s390-ccw-bios-2019-09-18' into s390-next-staging (2019-09-19 12:04:01 +0200) > > > > ---------------------------------------------------------------- > > - bugfixes in tcg and the ccw bios > > - gen15a is called z15 > > - officially require a 3.15 kernel or later for kvm > > > > ---------------------------------------------------------------- > > Hi -- I'm afraid this pullreq results in new warnings from > the runtime-sanitizer build when 'make check' is run: > MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} > QTEST_QEMU_BINARY=s390x-softmmu/qemu-system-s390x > QTEST_QEMU_IMG=qemu-img tests > /boot-serial-test -m=quick -k --tap < /dev/null | > ./scripts/tap-driver.pl --test-name="boot-serial-test" > /home/petmay01/linaro/qemu-for-merges/target/s390x/mem_helper.c:293:17: > runtime error: null pointer passed as argument 1, which is declared to > never be null > /usr/include/string.h:47:14: note: nonnull attribute specified here > /home/petmay01/linaro/qemu-for-merges/target/s390x/mem_helper.c:293:32: > runtime error: null pointer passed as argument 2, which is declared to > never be null > > (and the same warnings for a few other tests). > > Looks like you sometimes can pass NULL pointers to the source > and destination of memmove(). This isn't permitted by the > standard even in the case where the size argument is zero. > > thanks > -- PMM David, can you take a look?
On 20.09.19 13:00, Cornelia Huck wrote: > On Fri, 20 Sep 2019 11:45:18 +0100 > Peter Maydell <peter.maydell@linaro.org> wrote: > >> On Thu, 19 Sep 2019 at 13:41, Cornelia Huck <cohuck@redhat.com> wrote: >>> >>> The following changes since commit f8c3db33a5e863291182f8862ddf81618a7c6194: >>> >>> target/sparc: Switch to do_transaction_failed() hook (2019-09-17 12:01:00 +0100) >>> >>> are available in the Git repository at: >>> >>> https://github.com/cohuck/qemu tags/s390x-20190919 >>> >>> for you to fetch changes up to 37105adebeb28e60da3cb1ef82231d7ed8d23589: >>> >>> Merge tag 'tags/s390-ccw-bios-2019-09-18' into s390-next-staging (2019-09-19 12:04:01 +0200) >>> >>> ---------------------------------------------------------------- >>> - bugfixes in tcg and the ccw bios >>> - gen15a is called z15 >>> - officially require a 3.15 kernel or later for kvm >>> >>> ---------------------------------------------------------------- >> >> Hi -- I'm afraid this pullreq results in new warnings from >> the runtime-sanitizer build when 'make check' is run: >> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} >> QTEST_QEMU_BINARY=s390x-softmmu/qemu-system-s390x >> QTEST_QEMU_IMG=qemu-img tests >> /boot-serial-test -m=quick -k --tap < /dev/null | >> ./scripts/tap-driver.pl --test-name="boot-serial-test" >> /home/petmay01/linaro/qemu-for-merges/target/s390x/mem_helper.c:293:17: >> runtime error: null pointer passed as argument 1, which is declared to >> never be null >> /usr/include/string.h:47:14: note: nonnull attribute specified here >> /home/petmay01/linaro/qemu-for-merges/target/s390x/mem_helper.c:293:32: >> runtime error: null pointer passed as argument 2, which is declared to >> never be null >> >> (and the same warnings for a few other tests). >> >> Looks like you sometimes can pass NULL pointers to the source >> and destination of memmove(). This isn't permitted by the >> standard even in the case where the size argument is zero. >> >> thanks >> -- PMM > > David, can you take a look? > I would have assumed these pointers are ignored in case the length is zero, too (the only way this can happen). Easy to fix.
On 20.09.19 13:51, David Hildenbrand wrote: > On 20.09.19 13:00, Cornelia Huck wrote: >> On Fri, 20 Sep 2019 11:45:18 +0100 >> Peter Maydell <peter.maydell@linaro.org> wrote: >> >>> On Thu, 19 Sep 2019 at 13:41, Cornelia Huck <cohuck@redhat.com> wrote: >>>> >>>> The following changes since commit f8c3db33a5e863291182f8862ddf81618a7c6194: >>>> >>>> target/sparc: Switch to do_transaction_failed() hook (2019-09-17 12:01:00 +0100) >>>> >>>> are available in the Git repository at: >>>> >>>> https://github.com/cohuck/qemu tags/s390x-20190919 >>>> >>>> for you to fetch changes up to 37105adebeb28e60da3cb1ef82231d7ed8d23589: >>>> >>>> Merge tag 'tags/s390-ccw-bios-2019-09-18' into s390-next-staging (2019-09-19 12:04:01 +0200) >>>> >>>> ---------------------------------------------------------------- >>>> - bugfixes in tcg and the ccw bios >>>> - gen15a is called z15 >>>> - officially require a 3.15 kernel or later for kvm >>>> >>>> ---------------------------------------------------------------- >>> >>> Hi -- I'm afraid this pullreq results in new warnings from >>> the runtime-sanitizer build when 'make check' is run: >>> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} >>> QTEST_QEMU_BINARY=s390x-softmmu/qemu-system-s390x >>> QTEST_QEMU_IMG=qemu-img tests >>> /boot-serial-test -m=quick -k --tap < /dev/null | >>> ./scripts/tap-driver.pl --test-name="boot-serial-test" >>> /home/petmay01/linaro/qemu-for-merges/target/s390x/mem_helper.c:293:17: >>> runtime error: null pointer passed as argument 1, which is declared to >>> never be null >>> /usr/include/string.h:47:14: note: nonnull attribute specified here >>> /home/petmay01/linaro/qemu-for-merges/target/s390x/mem_helper.c:293:32: >>> runtime error: null pointer passed as argument 2, which is declared to >>> never be null >>> >>> (and the same warnings for a few other tests). >>> >>> Looks like you sometimes can pass NULL pointers to the source >>> and destination of memmove(). This isn't permitted by the >>> standard even in the case where the size argument is zero. >>> >>> thanks >>> -- PMM >> >> David, can you take a look? >> > > I would have assumed these pointers are ignored in case the length is > zero, too (the only way this can happen). Easy to fix. > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index e50cec9263..ef8e0c20a7 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -290,17 +290,23 @@ static void access_memmove(CPUS390XState *env, S390Access *desta, if (srca->size1 == desta->size1) { memmove(desta->haddr1, srca->haddr1, srca->size1); - memmove(desta->haddr2, srca->haddr2, srca->size2); + if (likely(srca->size2)) { + memmove(desta->haddr2, srca->haddr2, srca->size2); + } } else if (srca->size1 < desta->size1) { diff = desta->size1 - srca->size1; memmove(desta->haddr1, srca->haddr1, srca->size1); memmove(desta->haddr1 + srca->size1, srca->haddr2, diff); - memmove(desta->haddr2, srca->haddr2 + diff, desta->size2); + if (likely(desta->size2)) { + memmove(desta->haddr2, srca->haddr2 + diff, desta->size2); + } } else { diff = srca->size1 - desta->size1; memmove(desta->haddr1, srca->haddr1, desta->size1); memmove(desta->haddr2, srca->haddr1 + desta->size1, diff); - memmove(desta->haddr2 + diff, srca->haddr2, srca->size2); + if (likely(srca->size2)) { + memmove(desta->haddr2 + diff, srca->haddr2, srca->size2); + } } } For "s390x/tcg: Fault-safe memmove" should do the trick. Will test.
On Fri, 20 Sep 2019 13:59:12 +0200 David Hildenbrand <david@redhat.com> wrote: > On 20.09.19 13:51, David Hildenbrand wrote: > > On 20.09.19 13:00, Cornelia Huck wrote: > >> On Fri, 20 Sep 2019 11:45:18 +0100 > >> Peter Maydell <peter.maydell@linaro.org> wrote: > >> > >>> On Thu, 19 Sep 2019 at 13:41, Cornelia Huck <cohuck@redhat.com> wrote: > >>>> > >>>> The following changes since commit f8c3db33a5e863291182f8862ddf81618a7c6194: > >>>> > >>>> target/sparc: Switch to do_transaction_failed() hook (2019-09-17 12:01:00 +0100) > >>>> > >>>> are available in the Git repository at: > >>>> > >>>> https://github.com/cohuck/qemu tags/s390x-20190919 > >>>> > >>>> for you to fetch changes up to 37105adebeb28e60da3cb1ef82231d7ed8d23589: > >>>> > >>>> Merge tag 'tags/s390-ccw-bios-2019-09-18' into s390-next-staging (2019-09-19 12:04:01 +0200) > >>>> > >>>> ---------------------------------------------------------------- > >>>> - bugfixes in tcg and the ccw bios > >>>> - gen15a is called z15 > >>>> - officially require a 3.15 kernel or later for kvm > >>>> > >>>> ---------------------------------------------------------------- > >>> > >>> Hi -- I'm afraid this pullreq results in new warnings from > >>> the runtime-sanitizer build when 'make check' is run: > >>> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} > >>> QTEST_QEMU_BINARY=s390x-softmmu/qemu-system-s390x > >>> QTEST_QEMU_IMG=qemu-img tests > >>> /boot-serial-test -m=quick -k --tap < /dev/null | > >>> ./scripts/tap-driver.pl --test-name="boot-serial-test" > >>> /home/petmay01/linaro/qemu-for-merges/target/s390x/mem_helper.c:293:17: > >>> runtime error: null pointer passed as argument 1, which is declared to > >>> never be null > >>> /usr/include/string.h:47:14: note: nonnull attribute specified here > >>> /home/petmay01/linaro/qemu-for-merges/target/s390x/mem_helper.c:293:32: > >>> runtime error: null pointer passed as argument 2, which is declared to > >>> never be null > >>> > >>> (and the same warnings for a few other tests). > >>> > >>> Looks like you sometimes can pass NULL pointers to the source > >>> and destination of memmove(). This isn't permitted by the > >>> standard even in the case where the size argument is zero. > >>> > >>> thanks > >>> -- PMM > >> > >> David, can you take a look? > >> > > > > I would have assumed these pointers are ignored in case the length is > > zero, too (the only way this can happen). Easy to fix. > > > > > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c > index e50cec9263..ef8e0c20a7 100644 > --- a/target/s390x/mem_helper.c > +++ b/target/s390x/mem_helper.c > @@ -290,17 +290,23 @@ static void access_memmove(CPUS390XState *env, S390Access *desta, > > if (srca->size1 == desta->size1) { > memmove(desta->haddr1, srca->haddr1, srca->size1); > - memmove(desta->haddr2, srca->haddr2, srca->size2); > + if (likely(srca->size2)) { > + memmove(desta->haddr2, srca->haddr2, srca->size2); > + } > } else if (srca->size1 < desta->size1) { > diff = desta->size1 - srca->size1; > memmove(desta->haddr1, srca->haddr1, srca->size1); > memmove(desta->haddr1 + srca->size1, srca->haddr2, diff); > - memmove(desta->haddr2, srca->haddr2 + diff, desta->size2); > + if (likely(desta->size2)) { > + memmove(desta->haddr2, srca->haddr2 + diff, desta->size2); > + } > } else { > diff = srca->size1 - desta->size1; > memmove(desta->haddr1, srca->haddr1, desta->size1); > memmove(desta->haddr2, srca->haddr1 + desta->size1, diff); > - memmove(desta->haddr2 + diff, srca->haddr2, srca->size2); > + if (likely(srca->size2)) { > + memmove(desta->haddr2 + diff, srca->haddr2, srca->size2); > + } > } > } > > For "s390x/tcg: Fault-safe memmove" should do the trick. Will test. Ok, great. Peter, FYI: I'll be on vacation for two weeks (starting later today), so David/Thomas/Christian will probably handle any s390x-related things including pull requests for that time (I don't think I want to put a v2 together in a hurry...)
On Fri, 20 Sep 2019 at 14:33, Cornelia Huck <cohuck@redhat.com> wrote: > Peter, FYI: I'll be on vacation for two weeks (starting later today), > so David/Thomas/Christian will probably handle any s390x-related things > including pull requests for that time (I don't think I want to put a v2 > together in a hurry...) No worries; have a good holiday! -- PMM
On 20.09.19 15:41, Peter Maydell wrote: > On Fri, 20 Sep 2019 at 14:33, Cornelia Huck <cohuck@redhat.com> wrote: >> Peter, FYI: I'll be on vacation for two weeks (starting later today), >> so David/Thomas/Christian will probably handle any s390x-related things >> including pull requests for that time (I don't think I want to put a v2 >> together in a hurry...) > > No worries; have a good holiday! > > -- PMM > Peter, I'll send the s390x/tcg bits as a separate pull request directly to you this time. Cheers!