333555 - Coverity Crash if item is null [@ sec_asn1d_prepare_for_contents]

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[timeless]

coverity thinks this path is reachable. i'm inclined to agree

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Need a wee bit more info here.
What exactly is "this path" that is reachable?
Can you state a call stack?  
and the line number on which the crash is thought to occur?

Comment 2

[timeless]

990              sec_asn1d_prepare_for_contents
1119                  switch (state->underlying_kind) {

1167                    case SEC_ASN1_BMP_STRING:
1169              	if (state->contents_length % 2) {
make this false
1175              	goto regular_string_type;
1177                    case SEC_ASN1_UNIVERSAL_STRING:
1179              	if (state->contents_length % 4) {
make this false
1185              	goto regular_string_type;
1187                    case SEC_ASN1_SKIP:
1188                    case SEC_ASN1_ANY:
1189                    case SEC_ASN1_ANY_CONTENTS:
1196              regular_string_type:
1197                    case SEC_ASN1_BIT_STRING:
1198                    case SEC_ASN1_IA5_STRING:
1199                    case SEC_ASN1_OCTET_STRING:
1200                    case SEC_ASN1_PRINTABLE_STRING:
1201                    case SEC_ASN1_T61_STRING:
1202 relyea  1.1        case SEC_ASN1_UTC_TIME:
1203                    case SEC_ASN1_UTF8_STRING:
1204                    case SEC_ASN1_VISIBLE_STRING:
1212              	item = (SECItem *)(state->dest);
1226              	if (item == NULL || state->top->filter_only) {
let item = NULL
1227              	    if (item != NULL) {
1230              	    }
1231              	    alloc_len = 0;

1254              	if (alloc_len || ((! state->indefinite)
1255              			  && (state->subitems_head != NULL))) {
let !state->indefinite && state->subitems_head

1259              	    PORT_Assert (item->len == 0 && item->data == NULL);
crash here, item is null.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

taking

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

I think a simpler description of the problem is this:

There are many paths to line 1212, which reads:
        item = (SECItem *)(state->dest);
If item is assigned NULL there, and the code subsequently reaches line 1259,
it will crash there (or at places below in that same basic block) 
dereferencing the null pointer "item". 

This patch should avoid that, and cause the decoder to exit gracefully.
Please review.

Comment 5

[Alexei Volkov]

Comment on attachment 222454 [details] [diff] [review]
patch v1

r=alexei

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222454 [details] [diff] [review]
patch v1

have two reviews now.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Checking in util/secasn1d.c; new revision: 1.33.28.1; previous revision: 1.33
Checking in      secasn1d.c; new revision: 1.34;      previous revision: 1.33

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

CID 298