Message ID | 20210315204004.2025219-1-richard.henderson@linaro.org |
---|---|
State | New |
Headers | show |
Series | linux-user: Use signed lengths in uaccess.c | expand |
Patchew URL: https://patchew.org/QEMU/20210315204004.2025219-1-richard.henderson@linaro.org/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20210315204004.2025219-1-richard.henderson@linaro.org Subject: [PATCH] linux-user: Use signed lengths in uaccess.c === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20210315204004.2025219-1-richard.henderson@linaro.org -> patchew/20210315204004.2025219-1-richard.henderson@linaro.org Switched to a new branch 'test' cabd5c4 linux-user: Use signed lengths in uaccess.c === OUTPUT BEGIN === ERROR: externs should be avoided in .c files #77: FILE: linux-user/uaccess.c:27: +void unlock_user(void *host_ptr, abi_ulong guest_addr, ssize_t len); total: 1 errors, 0 warnings, 81 lines checked Commit cabd5c49328f (linux-user: Use signed lengths in uaccess.c) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20210315204004.2025219-1-richard.henderson@linaro.org/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Le 15/03/2021 à 21:40, Richard Henderson a écrit : > Partially revert 09f679b62dff, but only for the length arguments. > Instead of reverting to long, use ssize_t. Reinstate the > 0 check > in unlock_user. > > Fixes: 09f679b62dff > Reported-by: Coverity (CID 1446711) > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > linux-user/qemu.h | 15 +++++++++------ > linux-user/uaccess.c | 12 ++++++------ > 2 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/linux-user/qemu.h b/linux-user/qemu.h > index 52c981710b..74e06e7121 100644 > --- a/linux-user/qemu.h > +++ b/linux-user/qemu.h > @@ -627,8 +627,8 @@ static inline bool access_ok(CPUState *cpu, int type, > * buffers between the target and host. These internally perform > * locking/unlocking of the memory. > */ > -int copy_from_user(void *hptr, abi_ulong gaddr, size_t len); > -int copy_to_user(abi_ulong gaddr, void *hptr, size_t len); > +int copy_from_user(void *hptr, abi_ulong gaddr, ssize_t len); > +int copy_to_user(abi_ulong gaddr, void *hptr, ssize_t len); > > /* Functions for accessing guest memory. The tget and tput functions > read/write single values, byteswapping as necessary. The lock_user function > @@ -638,16 +638,19 @@ int copy_to_user(abi_ulong gaddr, void *hptr, size_t len); > > /* Lock an area of guest memory into the host. If copy is true then the > host area will have the same contents as the guest. */ > -void *lock_user(int type, abi_ulong guest_addr, size_t len, bool copy); > +void *lock_user(int type, abi_ulong guest_addr, ssize_t len, bool copy); > > /* Unlock an area of guest memory. The first LEN bytes must be > flushed back to guest memory. host_ptr = NULL is explicitly > allowed and does nothing. */ > #ifndef DEBUG_REMAP > -static inline void unlock_user(void *host_ptr, abi_ulong guest_addr, size_t len) > -{ } > +static inline void unlock_user(void *host_ptr, abi_ulong guest_addr, > + ssize_t len) > +{ > + /* no-op */ > +} > #else > -void unlock_user(void *host_ptr, abi_ulong guest_addr, long len); > +void unlock_user(void *host_ptr, abi_ulong guest_addr, ssize_t len); > #endif > > /* Return the length of a string in target memory or -TARGET_EFAULT if > diff --git a/linux-user/uaccess.c b/linux-user/uaccess.c > index c696913016..82b833b8f1 100644 > --- a/linux-user/uaccess.c > +++ b/linux-user/uaccess.c > @@ -4,7 +4,7 @@ > > #include "qemu.h" > > -void *lock_user(int type, abi_ulong guest_addr, size_t len, bool copy) > +void *lock_user(int type, abi_ulong guest_addr, ssize_t len, bool copy) > { > void *host_addr; > > @@ -24,7 +24,7 @@ void *lock_user(int type, abi_ulong guest_addr, size_t len, bool copy) > } > > #ifdef DEBUG_REMAP > -void unlock_user(void *host_ptr, abi_ulong guest_addr, size_t len); > +void unlock_user(void *host_ptr, abi_ulong guest_addr, ssize_t len); The semicolon has been added by 687ca797893c ("linux-user: Move lock_user et al out of line") perhaps it's time to remove it? Reviewed-by: Laurent Vivier <laurent@vivier.eu>
On Mon, 15 Mar 2021 at 21:07, Laurent Vivier <laurent@vivier.eu> wrote: > > Le 15/03/2021 à 21:40, Richard Henderson a écrit : > > Partially revert 09f679b62dff, but only for the length arguments. > > Instead of reverting to long, use ssize_t. Reinstate the > 0 check > > in unlock_user. > > > > Fixes: 09f679b62dff > > Reported-by: Coverity (CID 1446711) > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > Reviewed-by: Laurent Vivier <laurent@vivier.eu> Hi -- did this patch get lost? I think we should put it into 6.0. thanks -- PMM
Le 07/04/2021 à 17:16, Peter Maydell a écrit : > On Mon, 15 Mar 2021 at 21:07, Laurent Vivier <laurent@vivier.eu> wrote: >> >> Le 15/03/2021 à 21:40, Richard Henderson a écrit : >>> Partially revert 09f679b62dff, but only for the length arguments. >>> Instead of reverting to long, use ssize_t. Reinstate the > 0 check >>> in unlock_user. >>> >>> Fixes: 09f679b62dff >>> Reported-by: Coverity (CID 1446711) >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > >> Reviewed-by: Laurent Vivier <laurent@vivier.eu> > > Hi -- did this patch get lost? I think we should put it into 6.0. It seems so... thank you. I prepare a PR with it. Laurent
diff --git a/linux-user/qemu.h b/linux-user/qemu.h index 52c981710b..74e06e7121 100644 --- a/linux-user/qemu.h +++ b/linux-user/qemu.h @@ -627,8 +627,8 @@ static inline bool access_ok(CPUState *cpu, int type, * buffers between the target and host. These internally perform * locking/unlocking of the memory. */ -int copy_from_user(void *hptr, abi_ulong gaddr, size_t len); -int copy_to_user(abi_ulong gaddr, void *hptr, size_t len); +int copy_from_user(void *hptr, abi_ulong gaddr, ssize_t len); +int copy_to_user(abi_ulong gaddr, void *hptr, ssize_t len); /* Functions for accessing guest memory. The tget and tput functions read/write single values, byteswapping as necessary. The lock_user function @@ -638,16 +638,19 @@ int copy_to_user(abi_ulong gaddr, void *hptr, size_t len); /* Lock an area of guest memory into the host. If copy is true then the host area will have the same contents as the guest. */ -void *lock_user(int type, abi_ulong guest_addr, size_t len, bool copy); +void *lock_user(int type, abi_ulong guest_addr, ssize_t len, bool copy); /* Unlock an area of guest memory. The first LEN bytes must be flushed back to guest memory. host_ptr = NULL is explicitly allowed and does nothing. */ #ifndef DEBUG_REMAP -static inline void unlock_user(void *host_ptr, abi_ulong guest_addr, size_t len) -{ } +static inline void unlock_user(void *host_ptr, abi_ulong guest_addr, + ssize_t len) +{ + /* no-op */ +} #else -void unlock_user(void *host_ptr, abi_ulong guest_addr, long len); +void unlock_user(void *host_ptr, abi_ulong guest_addr, ssize_t len); #endif /* Return the length of a string in target memory or -TARGET_EFAULT if diff --git a/linux-user/uaccess.c b/linux-user/uaccess.c index c696913016..82b833b8f1 100644 --- a/linux-user/uaccess.c +++ b/linux-user/uaccess.c @@ -4,7 +4,7 @@ #include "qemu.h" -void *lock_user(int type, abi_ulong guest_addr, size_t len, bool copy) +void *lock_user(int type, abi_ulong guest_addr, ssize_t len, bool copy) { void *host_addr; @@ -24,7 +24,7 @@ void *lock_user(int type, abi_ulong guest_addr, size_t len, bool copy) } #ifdef DEBUG_REMAP -void unlock_user(void *host_ptr, abi_ulong guest_addr, size_t len); +void unlock_user(void *host_ptr, abi_ulong guest_addr, ssize_t len); { void *host_ptr_conv; @@ -35,7 +35,7 @@ void unlock_user(void *host_ptr, abi_ulong guest_addr, size_t len); if (host_ptr == host_ptr_conv) { return; } - if (len != 0) { + if (len > 0) { memcpy(host_ptr_conv, host_ptr, len); } g_free(host_ptr); @@ -48,14 +48,14 @@ void *lock_user_string(abi_ulong guest_addr) if (len < 0) { return NULL; } - return lock_user(VERIFY_READ, guest_addr, (size_t)len + 1, 1); + return lock_user(VERIFY_READ, guest_addr, len + 1, 1); } /* copy_from_user() and copy_to_user() are usually used to copy data * buffers between the target and host. These internally perform * locking/unlocking of the memory. */ -int copy_from_user(void *hptr, abi_ulong gaddr, size_t len) +int copy_from_user(void *hptr, abi_ulong gaddr, ssize_t len) { int ret = 0; void *ghptr = lock_user(VERIFY_READ, gaddr, len, 1); @@ -69,7 +69,7 @@ int copy_from_user(void *hptr, abi_ulong gaddr, size_t len) return ret; } -int copy_to_user(abi_ulong gaddr, void *hptr, size_t len) +int copy_to_user(abi_ulong gaddr, void *hptr, ssize_t len) { int ret = 0; void *ghptr = lock_user(VERIFY_WRITE, gaddr, len, 0);
Partially revert 09f679b62dff, but only for the length arguments. Instead of reverting to long, use ssize_t. Reinstate the > 0 check in unlock_user. Fixes: 09f679b62dff Reported-by: Coverity (CID 1446711) Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- linux-user/qemu.h | 15 +++++++++------ linux-user/uaccess.c | 12 ++++++------ 2 files changed, 15 insertions(+), 12 deletions(-)