333405 - item cleanup is unused DEADCODE in SECITEM_AllocItem loser

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P4)

Description

[timeless]

found by coverity

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

I'm not sure, but I think this bug suggests that SECITEM_AllocItem is
unused in mozilla and therefore should be removed from NSS.  

The mozilla family of client products are not the only users of NSS. 
The mere fact that mozilla doesn't use a feature doesn't mean the feature
should be removed.  

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Hmm.  On re-review, I guess it is asserting that this statement
 91   	if (item != NULL) {
after the label "loser" can never be true.
I think in the past, SECITEM_AllocItem would "goto loser"
if the allocation for item->data failed, which is how the
above "if" expression could have been true.  

The question in my mind is: shold SECITEM_AllocItem reutrn
a NULL "item", or an item with a null "data" pointer when 
it cannot allocate the data.  

Either way, we won't worry about this until well after NSS 3.11.1
is released.

Comment 3

[Wan-Teh Chang]

Nelson, you asked:
> The question in my mind is: shold SECITEM_AllocItem reutrn
> a NULL "item", or an item with a null "data" pointer when 
> it cannot allocate the data.

This issue is the subject of bug 320336.

Comment 4

[Wan-Teh Chang]

My patch for bug 320336 fixes this bug as a by-product
because it makes the dead code reachable now.  I've
checked in my patch on the NSS trunk (3.12).  So this
bug is fixed in NSS 3.12.

Nelson, if you really want this bug (severity=trivial)
fixed in NSS 3.11.1, do you want me to fix it by removing
the dead code or with my patch for bug 320336?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, I agree that *this* bug is trivial. 
But I think your fix for bug 320336 should be on both trunk and branch.
And since it fixes this bug as a by-product, that's even better.

Comment 6

[Wan-Teh Chang]

OK, this bug is fixed in NSS 3.11.1 (by making the
dead code reachable).  The patch is in bug 320336.

Comment 7

[Wan-Teh Chang]

The patch was backed out because it changed the function's
behavior when the 'len' argument is 0.

Comment 8

[Wan-Teh Chang]

Moved to NSS 3.11.2 because bug 320336, which this bug depends on,
was moved to NSS 3.11.2.

Comment 9

[Wan-Teh Chang]

I only checked in the patch (in bug 320336) on the NSS trunk (3.12),
partly because Nelson is worried about unintended consequences, and
partly because lib/util is used by lib/freebl, and we want to finish
our FIPS algorithm testing this week.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

CID 300