diff mbox

disas/cris.c: Fix Coverity warning about unchecked NULL

Message ID 1483988759-14606-1-git-send-email-peter.maydell@linaro.org
State New
Headers show

Commit Message

Peter Maydell Jan. 9, 2017, 7:05 p.m. UTC
Coverity (CID 1005689) warns that we don't check that
spec_reg_info() returned non-NULL before dereferencing.
Add the check, though as the comment notes this is
a can't-really-happen case because the earlier constraint
matching should have ruled out the "unknown reg" case.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 disas/cris.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

no-reply@patchew.org Jan. 9, 2017, 7:10 p.m. UTC | #1
Hi,

Your series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH] disas/cris.c: Fix Coverity warning about unchecked NULL
Type: series
Message-id: 1483988759-14606-1-git-send-email-peter.maydell@linaro.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1483988759-14606-1-git-send-email-peter.maydell@linaro.org -> patchew/1483988759-14606-1-git-send-email-peter.maydell@linaro.org
Switched to a new branch 'test'
63cfa26 disas/cris.c: Fix Coverity warning about unchecked NULL

=== OUTPUT BEGIN ===
Checking PATCH 1/1: disas/cris.c: Fix Coverity warning about unchecked NULL...
ERROR: code indent should never use tabs
#24: FILE: disas/cris.c:2493:
+^Iif (sregp == NULL || sregp->name == NULL)$

ERROR: suspect code indent for conditional statements (8, 10)
#24: FILE: disas/cris.c:2493:
+	if (sregp == NULL || sregp->name == NULL)
 	  /* Should have been caught as a non-match earlier.  */

ERROR: braces {} are necessary for all arms of this statement
#24: FILE: disas/cris.c:2493:
+	if (sregp == NULL || sregp->name == NULL)
[...]

total: 3 errors, 0 warnings, 8 lines checked

Your patch 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


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
Edgar E. Iglesias Jan. 9, 2017, 8:13 p.m. UTC | #2
On Mon, Jan 09, 2017 at 07:05:59PM +0000, Peter Maydell wrote:
> Coverity (CID 1005689) warns that we don't check that
> spec_reg_info() returned non-NULL before dereferencing.
> Add the check, though as the comment notes this is
> a can't-really-happen case because the earlier constraint
> matching should have ruled out the "unknown reg" case.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  disas/cris.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/disas/cris.c b/disas/cris.c
> index 08161d1..8a1daf9 100644
> --- a/disas/cris.c
> +++ b/disas/cris.c
> @@ -2490,7 +2490,7 @@ print_with_operands (const struct cris_opcode *opcodep,
>  	const struct cris_spec_reg *sregp
>  	  = spec_reg_info ((insn >> 12) & 15, disdata->distype);
>  
> -	if (sregp->name == NULL)
> +	if (sregp == NULL || sregp->name == NULL)
>  	  /* Should have been caught as a non-match earlier.  */
>  	  *tp++ = '?';
>  	else
> -- 
> 2.7.4
>
Peter Maydell Jan. 9, 2017, 9:35 p.m. UTC | #3
On 9 January 2017 at 19:10,  <no-reply@patchew.org> wrote:
> Checking PATCH 1/1: disas/cris.c: Fix Coverity warning about unchecked NULL...
> ERROR: code indent should never use tabs
> #24: FILE: disas/cris.c:2493:
> +^Iif (sregp == NULL || sregp->name == NULL)$
>
> ERROR: suspect code indent for conditional statements (8, 10)
> #24: FILE: disas/cris.c:2493:
> +       if (sregp == NULL || sregp->name == NULL)
>           /* Should have been caught as a non-match earlier.  */
>
> ERROR: braces {} are necessary for all arms of this statement
> #24: FILE: disas/cris.c:2493:
> +       if (sregp == NULL || sregp->name == NULL)
> [...]
>
> total: 3 errors, 0 warnings, 8 lines checked

This is because the whole file is GNU coding standards
style, being a binutils import. Better to stick with it rather
than rework, I think.

thanks
-- PMM
Edgar E. Iglesias Jan. 9, 2017, 10:20 p.m. UTC | #4
On Mon, Jan 09, 2017 at 09:35:16PM +0000, Peter Maydell wrote:
> On 9 January 2017 at 19:10,  <no-reply@patchew.org> wrote:
> > Checking PATCH 1/1: disas/cris.c: Fix Coverity warning about unchecked NULL...
> > ERROR: code indent should never use tabs
> > #24: FILE: disas/cris.c:2493:
> > +^Iif (sregp == NULL || sregp->name == NULL)$
> >
> > ERROR: suspect code indent for conditional statements (8, 10)
> > #24: FILE: disas/cris.c:2493:
> > +       if (sregp == NULL || sregp->name == NULL)
> >           /* Should have been caught as a non-match earlier.  */
> >
> > ERROR: braces {} are necessary for all arms of this statement
> > #24: FILE: disas/cris.c:2493:
> > +       if (sregp == NULL || sregp->name == NULL)
> > [...]
> >
> > total: 3 errors, 0 warnings, 8 lines checked
> 
> This is because the whole file is GNU coding standards
> style, being a binutils import. Better to stick with it rather
> than rework, I think.

Yes, I agree.

Cheers,
Edgar
Michael Tokarev Jan. 12, 2017, 11:03 a.m. UTC | #5
09.01.2017 22:05, Peter Maydell wrote:
> Coverity (CID 1005689) warns that we don't check that
> spec_reg_info() returned non-NULL before dereferencing.
> Add the check, though as the comment notes this is
> a can't-really-happen case because the earlier constraint
> matching should have ruled out the "unknown reg" case.

Applied to -trivial, thanks!

/mjt
diff mbox

Patch

diff --git a/disas/cris.c b/disas/cris.c
index 08161d1..8a1daf9 100644
--- a/disas/cris.c
+++ b/disas/cris.c
@@ -2490,7 +2490,7 @@  print_with_operands (const struct cris_opcode *opcodep,
 	const struct cris_spec_reg *sregp
 	  = spec_reg_info ((insn >> 12) & 15, disdata->distype);
 
-	if (sregp->name == NULL)
+	if (sregp == NULL || sregp->name == NULL)
 	  /* Should have been caught as a non-match earlier.  */
 	  *tp++ = '?';
 	else