| Message ID | 20170724182751.18261-28-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit : > Clang's scan-build-5.0 reports: > > linux-user/syscall.c:5581:9: warning: Dereference of undefined pointer value > if (*host_rt_dev_ptr != 0) { > ^~~~~~~~~~~~~~~~ > > Reported-by: Clang Static Analyzer > Suggested-by: Laurent Vivier <lvivier@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > linux-user/syscall.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index fcd20fa276..e79b5baec4 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -5524,7 +5524,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; > > @@ -5570,6 +5570,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)); > + assert(host_rt_dev_ptr); > if (*host_rt_dev_ptr != 0) { > unlock_user((void *)*host_rt_dev_ptr, > *target_rt_dev_ptr, 0); > I think if you add the assert you should not have to add the "= NULL". Could you check? Thanks, Laurent
On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Clang's scan-build-5.0 reports: > > linux-user/syscall.c:5581:9: warning: Dereference of undefined pointer value > if (*host_rt_dev_ptr != 0) { > ^~~~~~~~~~~~~~~~ > > Reported-by: Clang Static Analyzer > Suggested-by: Laurent Vivier <lvivier@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > linux-user/syscall.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index fcd20fa276..e79b5baec4 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -5524,7 +5524,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; > > @@ -5570,6 +5570,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)); > + assert(host_rt_dev_ptr); There's not much point in this assert because it doesn't tell us anything we're not about to find out immediately by dereferencing the pointer... > if (*host_rt_dev_ptr != 0) { > unlock_user((void *)*host_rt_dev_ptr, > *target_rt_dev_ptr, 0); > -- > 2.13.3 thanks -- PMM
On 07/24/2017 04:33 PM, Laurent Vivier wrote: > Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit : >> Clang's scan-build-5.0 reports: >> >> linux-user/syscall.c:5581:9: warning: Dereference of undefined pointer value >> if (*host_rt_dev_ptr != 0) { >> ^~~~~~~~~~~~~~~~ >> >> Reported-by: Clang Static Analyzer >> Suggested-by: Laurent Vivier <lvivier@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> linux-user/syscall.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> index fcd20fa276..e79b5baec4 100644 >> --- a/linux-user/syscall.c >> +++ b/linux-user/syscall.c >> @@ -5524,7 +5524,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; >> >> @@ -5570,6 +5570,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)); >> + assert(host_rt_dev_ptr); >> if (*host_rt_dev_ptr != 0) { >> unlock_user((void *)*host_rt_dev_ptr, >> *target_rt_dev_ptr, 0); >> > > I think if you add the assert you should not have to add the "= NULL". > Could you check? Indeed! Thanks :) Phil.
Le 24/07/2017 à 23:26, Peter Maydell a écrit : > On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> Clang's scan-build-5.0 reports: >> >> linux-user/syscall.c:5581:9: warning: Dereference of undefined pointer value >> if (*host_rt_dev_ptr != 0) { >> ^~~~~~~~~~~~~~~~ >> >> Reported-by: Clang Static Analyzer >> Suggested-by: Laurent Vivier <lvivier@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> linux-user/syscall.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> index fcd20fa276..e79b5baec4 100644 >> --- a/linux-user/syscall.c >> +++ b/linux-user/syscall.c >> @@ -5524,7 +5524,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; >> >> @@ -5570,6 +5570,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)); >> + assert(host_rt_dev_ptr); > > There's not much point in this assert because it doesn't > tell us anything we're not about to find out immediately > by dereferencing the pointer... It's just to shut off the warning. What I said in the comment of the previous version of this patch: 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. Thanks, Laurent
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index fcd20fa276..e79b5baec4 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -5524,7 +5524,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; @@ -5570,6 +5570,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)); + assert(host_rt_dev_ptr); if (*host_rt_dev_ptr != 0) { unlock_user((void *)*host_rt_dev_ptr, *target_rt_dev_ptr, 0);
Clang's scan-build-5.0 reports: linux-user/syscall.c:5581:9: warning: Dereference of undefined pointer value if (*host_rt_dev_ptr != 0) { ^~~~~~~~~~~~~~~~ Reported-by: Clang Static Analyzer Suggested-by: Laurent Vivier <lvivier@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- linux-user/syscall.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)