| Message ID | 20170506004106.3291-1-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/05/2017 02:41, Philippe Mathieu-Daudé wrote: > static code analyzer complain: > > linux-user/syscall.c:5575:9: warning: Dereference of undefined pointer value > if (*host_rt_dev_ptr != 0) { > ^~~~~~~~~~~~~~~~ > > Reported-by: Clang Static Analyzer > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > linux-user/syscall.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index cec8428589..c541e22693 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -5526,7 +5526,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp, > int target_size; > void *argptr; > abi_ulong *target_rt_dev_ptr; > - unsigned long *host_rt_dev_ptr; > + unsigned long *host_rt_dev_ptr = NULL; > abi_long ret; > int i; > > @@ -5572,7 +5572,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp, > unlock_user(argptr, arg, 0); > > ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp)); I think we should "assert(host_rt_dev_ptr)" here. It's a bug if host_rt_dev_ptr is not set. The "for" loop scans the structure to find the rt_dev field, and we should always enter in the first "if", so "host_rt_dev_ptr" is always set. > - if (*host_rt_dev_ptr != 0) { > + if (host_rt_dev_ptr != NULL && *host_rt_dev_ptr != 0) { > unlock_user((void *)*host_rt_dev_ptr, > *target_rt_dev_ptr, 0); > } > Laurent
On 05/10/2017 09:32 AM, Laurent Vivier wrote: > On 06/05/2017 02:41, Philippe Mathieu-Daudé wrote: >> static code analyzer complain: >> >> linux-user/syscall.c:5575:9: warning: Dereference of undefined pointer value >> if (*host_rt_dev_ptr != 0) { >> ^~~~~~~~~~~~~~~~ >> >> Reported-by: Clang Static Analyzer >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> linux-user/syscall.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> index cec8428589..c541e22693 100644 >> --- a/linux-user/syscall.c >> +++ b/linux-user/syscall.c >> @@ -5526,7 +5526,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp, >> int target_size; >> void *argptr; >> abi_ulong *target_rt_dev_ptr; >> - unsigned long *host_rt_dev_ptr; >> + unsigned long *host_rt_dev_ptr = NULL; >> abi_long ret; >> int i; >> >> @@ -5572,7 +5572,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp, >> unlock_user(argptr, arg, 0); >> >> ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp)); > > I think we should "assert(host_rt_dev_ptr)" here. It's a bug if > host_rt_dev_ptr is not set. > > The "for" loop scans the structure to find the rt_dev field, and we > should always enter in the first "if", so "host_rt_dev_ptr" is always set. It's better indeed, I'll use g_assert_nonnull() and resend. Thanks! > >> - if (*host_rt_dev_ptr != 0) { >> + if (host_rt_dev_ptr != NULL && *host_rt_dev_ptr != 0) { >> unlock_user((void *)*host_rt_dev_ptr, >> *target_rt_dev_ptr, 0); >> } >> > > Laurent >
>> >> The "for" loop scans the structure to find the rt_dev field, and we >> should always enter in the first "if", so "host_rt_dev_ptr" is always >> set. > > It's better indeed, I'll use g_assert_nonnull() and resend. Careful - we haven't yet resolved whether g_assert_nonnull() is safe to use. https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg05499.html
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index cec8428589..c541e22693 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -5526,7 +5526,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp, int target_size; void *argptr; abi_ulong *target_rt_dev_ptr; - unsigned long *host_rt_dev_ptr; + unsigned long *host_rt_dev_ptr = NULL; abi_long ret; int i; @@ -5572,7 +5572,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp, unlock_user(argptr, arg, 0); ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp)); - if (*host_rt_dev_ptr != 0) { + if (host_rt_dev_ptr != NULL && *host_rt_dev_ptr != 0) { unlock_user((void *)*host_rt_dev_ptr, *target_rt_dev_ptr, 0); }
static code analyzer complain: linux-user/syscall.c:5575:9: warning: Dereference of undefined pointer value if (*host_rt_dev_ptr != 0) { ^~~~~~~~~~~~~~~~ Reported-by: Clang Static Analyzer Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- linux-user/syscall.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)