[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
- [Qemu-devel] [PATCH 2/5] use qemu_pipe_non_block, (continued)
- [Qemu-devel] [PATCH 3/5] libcacard/vscclient: fix leakage of socket on error paths, Alon Levy, 2013/06/04
- [Qemu-devel] [PATCH 4/5] libcacard/vreader.c: fix possible NULL dereference, Alon Levy, 2013/06/04
- Re: [Qemu-devel] [PATCH 4/5] libcacard/vreader.c: fix possible NULL dereference,
Peter Maydell <=
- [Qemu-devel] [PATCH 5/5] libcacard/vscclient.c: fix use of uninitialized variable, Alon Levy, 2013/06/04
- Re: [Qemu-devel] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block, Peter Maydell, 2013/06/04
- Re: [Qemu-devel] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block, Eric Blake, 2013/06/04
- Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block, Michael Tokarev, 2013/06/05
- Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/5] oslib-posix: add qemu_pipe_non_block, Michael Tokarev, 2013/06/12