Message ID | 1370377419-31788-4-git-send-email-alevy@redhat.com |
---|---|
State | New |
Headers | show |
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
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);
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(-)