1216501 - [CID 1327954][CID 1327953][CID 1327952][CID 1327951] Using uninitialized value secitem.type when calling SECITEM_CopyItem_Util - copying secitem.type only if present

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

SECITEM_CopyItem is often used on objects that are missing the type, i.e. type on those has never been set. Instead of changing all occurrences I'd suggest copying the type only if it was set.

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: CID1327954.patch

I fixed one of those before, but there are at least 4 more. So I think this might be a better solution. (Skipping codereview for these easy patches now.)

Comment 2

[Franziskus Kiefer [:franziskus]]

Attachment: CID1327954.patch

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8676211 [details] [diff] [review]
CID1327954.patch

Review of attachment 8676211 [details] [diff] [review]:
-----------------------------------------------------------------

Not sure what you are trying to do here.

::: lib/util/secitem.c
@@ +269,1 @@
>      if (from->data && from->len) {

If from is NULL, then you get a null-deref on this line instead.  Also, a from->type of siBuffer is falsy.

Comment 4

[Franziskus Kiefer [:franziskus]]

(In reply to Martin Thomson [:mt:] from comment #3)
> Comment on attachment 8676211 [details] [diff] [review]
> CID1327954.patch
> 
> Review of attachment 8676211 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not sure what you are trying to do here.
> 
> ::: lib/util/secitem.c
> @@ +269,1 @@
> >      if (from->data && from->len) {
> 
> If from is NULL, then you get a null-deref on this line instead.  Also, a
> from->type of siBuffer is falsy.

sure, I don't get why none of these functions ever check the pointers they... But I just want to fix one issue here, that is that from->type is not initialised in many cases this is called. To avoid getting undefined behaviour here every time we copy the uninitialised from->type we should simply not copy it in this case. The alternative would be to fix all callsites of SECITEM_CopyItem.

Comment 5

[Martin Thomson [:mt:]]

This isn't going to fix any problems with having from->type uninitialized, it's just going to propagate the problem.

Comment 6

[Franziskus Kiefer [:franziskus]]

Attachment: CID1327954.patch

right, stupid me. I'm just setting the type in those four cases coverity complains. I'm not sure if those are the correct types (I tried my best, let me know if you think they're supposed to be different), but since they're never checked anyway... This will probably pop up at other places as well, we can fix it everywhere or maybe somewhen remove the type from secitem all together ;)

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8676818 [details] [diff] [review]
CID1327954.patch

Review of attachment 8676818 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/projects/nss/rev/ad5687c22dff