[4/5] libcacard/vreader.c: fix possible NULL dereference

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 <alevy@redhat.com>
---
 libcacard/vreader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On 4 June 2013 21:23, Alon Levy <alevy@redhat.com> 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 <alevy@redhat.com>
> ---
>  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