qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 4/5] libcacard/vreader.c: fix possible NULL dere


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH 4/5] libcacard/vreader.c: fix possible NULL dereference
Date: Tue, 4 Jun 2013 22:06:36 +0100

On 4 June 2013 21:23, Alon Levy <address@hidden> wrote:
> Reported by Coverity:
>
> Error: FORWARD_NULL (CWE-476):
> qemu-1.5.0/libcacard/vreader.c:267: cond_false: Condition "card == NULL", 
> taking false branch
> qemu-1.5.0/libcacard/vreader.c:269: if_end: End of if statement
> qemu-1.5.0/libcacard/vreader.c:272: cond_false: Condition "apdu == NULL", 
> taking false branch
> qemu-1.5.0/libcacard/vreader.c:275: else_branch: Reached else branch
> qemu-1.5.0/libcacard/vreader.c:280: cond_false: Condition "response", taking 
> false branch
> qemu-1.5.0/libcacard/vreader.c:284: if_end: End of if statement
> qemu-1.5.0/libcacard/vreader.c:280: var_compare_op: Comparing "response" to 
> null implies that "response" might be null.
> qemu-1.5.0/libcacard/vreader.c:286: cond_true: Condition "card_status == 
> VCARD_DONE", taking true branch
> qemu-1.5.0/libcacard/vreader.c:287: cond_true: Condition "card_status == 
> VCARD_DONE", taking true branch
> qemu-1.5.0/libcacard/vreader.c:288: var_deref_op: Dereferencing null pointer 
> "response".
>
> Signed-off-by: Alon Levy <address@hidden>
> ---
>  libcacard/vreader.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libcacard/vreader.c b/libcacard/vreader.c
> index 5793d73..60eb43b 100644
> --- a/libcacard/vreader.c
> +++ b/libcacard/vreader.c
> @@ -260,7 +260,7 @@ vreader_xfr_bytes(VReader *reader,
>  {
>      VCardAPDU *apdu;
>      VCardResponse *response = NULL;
> -    VCardStatus card_status;
> +    VCardStatus card_status = VCARD_FAIL;
>      unsigned short status;
>      VCard *card = vreader_get_card(reader);
>

This doesn't make any sense to me as a fix for the quoted coverity
issue. It's complaining because the function both checks
that response isn't NULL (line 280) and uses it without
checking (line 288). If your change makes it stop complaining
it's only because you've managed to confuse it.

You either need to:
 * assume that vcard_make_response() and vcard_process_apdu()
   both guarantee to set response to non-NULL, and drop the
   "if (response)" check
 * don't assume it, and handle NULL response consistently
   in this function (eg by changing the line 287 check to
   "if (card_status == VCARD_DONE && response)"
 * take some middle line, eg "response is guaranteed not to
   be NULL if and only if status is VCARD_DONE" and then
   consistently check card_status in both places.

Also, this sequence:
    assert(card_status == VCARD_DONE);
    if (card_status == VCARD_DONE) {

is nonsensical. Either we should assert that the status
is always DONE, or we have code to handle the DONE and not
DONE cases; not both.

thanks
-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]